Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move tools to use LKG ILC #107772

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Move tools to use LKG ILC #107772

wants to merge 12 commits into from

Conversation

agocke
Copy link
Member

@agocke agocke commented Sep 12, 2024

Starts with crossgen2 and moves shared helpers into a central targets file.

Starts with crossgen2 and moves shared helpers into a central targets file.
@jkoritzinsky
Copy link
Member

Are we going to build the non-NativeAOT crossgen2 (the singlefilehost version) against the LKG bits?

If so, could we instead just update/use the crossgen2.csproj project and have 2 projects instead of continuing to have the 3 separate projects?

@agocke
Copy link
Member Author

agocke commented Sep 12, 2024

Are we going to build the non-NativeAOT crossgen2 (the singlefilehost version) against the LKG bits?

By that you mean, are we going to pull the single-file host from the LKG as well? I hadn't thought of that, but it makes sense.

@jkoritzinsky
Copy link
Member

Yep that's what I was thinking of. I think however we ship crossgen2, we should ship it on equivalent runtime bits.

The alternative would likely be more expensive in infra maintenence for minimal gain, unless it's required by source-build or something.

@@ -1,14 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<CrossHostArch Condition="'$(CrossBuild)' == 'true' or '$(TargetArchitecture)' != '$(BuildArchitecture)' or '$(HostOS)' != '$(TargetOS)' or '$(EnableNativeSanitizers)' != ''">$(BuildArchitecture)</CrossHostArch>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For ilc, this is set unconditionally in the crossarch project

<OutputPath>$(RuntimeBinDir)/$(CrossHostArch)/ilc/</OutputPath>

Also, we should make the crossgen and ilc project files use the same naming scheme. I like the naming scheme for ilc project files (ILCompiler.csproj, ILCompiler_crossarch.csproj).

@agocke
Copy link
Member Author

agocke commented Sep 13, 2024

I've been experimenting locally, and I don't think we should get rid of the publish project either.

The problem is that the SDK is architected such that _IsPublishing needs to be set when doing a publish operation.

I can't think of a way to do that without a different project if we also want things to use the non-published copy. If I set _IsPublishing unconditionally than the CoreCLR copy has the wrong settings. If I don't set it, we don't have the right publish settings. There's no condition I can use to set it otherwise.

@jkotas
Copy link
Member

jkotas commented Sep 13, 2024

ilc has two projects (native and cross-arch). It does not have any special handling of publishing. Can crossgen be on the same plan?

@agocke
Copy link
Member Author

agocke commented Sep 13, 2024

I think ILC is the incorrect one here -- while not setting _IsPublishing doesn't actively break anything right now, it definitely could in the future. As we've been conditioning more things in the SDK to have different behavior during publish (to specialize for Native AOT and other form factors in combination with multiple architectures), _IsPublishing is becoming more common.

Thus far ILC has not depended on any of those behaviors, but I think that's accidental.

@am11
Copy link
Member

am11 commented Sep 14, 2024

Maybe I am missing the bigger picture. Coverage-wise, both approaches are eventually testing what is being shipped:

  • With live build, we will be testing every version before it is shipped and as the changes are being made.

  • With LKG, we will be testing every version after it has made it to some public/semi-public nuget feed.

Live build is better IMO, and it gives us control over enabling nativeaot for new platforms without waiting for the entire update cycle, runtime->sdk->runtime (which takes a month or two). End to end tests in dotnet/sdk repo are covering the nuget feed scenario.

@agocke
Copy link
Member Author

agocke commented Sep 16, 2024

With live build, we will be testing every version before it is shipped and as the changes are being made.

But it's also testing every version, as you're developing it on a local machine and in CI. So in addition to the complexity in the build system needed to produce the right phase ordering, you also make local development and CI uniquely unstable by changing Native AOT. Add to all of this, debug builds become unusably slow because the debug JIT is incredibly slow.

Optimizing for live builds is optimizing for release validation, while using LKG builds is optimizing for infra simplicity and developer experience.

@am11
Copy link
Member

am11 commented Sep 16, 2024

Yup, @jkotas explained it to me about the release vs. debug performance penalty and I agree that live build (for non-testing) should be on release plan.

BTW, are you looking to incorporrate the scenario #105004 is covering? I can close that one. Basically it's this workflow https://github.com/am11/CrossRepoCITesting/actions/runs/10847119420/workflow (also covered in runtime CI, see eng/pipelines changes in the PR).

@agocke
Copy link
Member Author

agocke commented Sep 18, 2024

BTW, are you looking to incorporrate the scenario #105004 is covering? I can close that one

In theory, yes. In practice, there isn't an LKG apphost for FreeBSD right? I haven't thought of what we should do in that case. Perhaps the answer is "require an LKG apphost just like we require an LKG runtime"

@jkotas
Copy link
Member

jkotas commented Sep 18, 2024

I haven't thought of what we should do in that case. Perhaps the answer is "require an LKG apphost just like we require an LKG runtime"

I think we should introduce a new build option that builds everything that comes from LKG (including LKG apphost if necessary) and require invoking build with that option before invoking regular build if there is no LKG available for the target platform.

#105004 (comment) has details.

Introducing this build option should be a separate PR from this one.

@build-analysis build-analysis bot mentioned this pull request Sep 19, 2024
3 tasks
@am11
Copy link
Member

am11 commented Sep 19, 2024

In theory, yes. In practice, there isn't an LKG apphost for FreeBSD right?

Yes, it's about the else case when LKG is not available. These PRs are overlapping it's best to do it one at a time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants