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

[release/7.0] Disable NativeAOT subset for source-build. #76206

Merged
merged 2 commits into from
Oct 10, 2022

Conversation

crummel
Copy link
Contributor

@crummel crummel commented Sep 26, 2022

Follow-up to #71853.
Tracking issue in source-build is dotnet/source-build#2885.
If merged obsoletes #74505.

The source-build understanding is:

I would like to make the minimal change necessary to disable the dependency on llvm-project in runtime without breaking any end-user functionality and this subset seems like it should fit that bill - please let me know if I'm wrong.

@ghost ghost assigned crummel Sep 26, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@crummel
Copy link
Contributor Author

crummel commented Sep 26, 2022

cc @tmds @omajid @jkotas @dsplaisted

@ghost
Copy link

ghost commented Sep 26, 2022

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

Follow-up to #71853.
Tracking issue in source-build is dotnet/source-build#2885.
If merged obsoletes #74505.

The source-build understanding is:

I would like to make the minimal change necessary to disable the dependency on llvm-project in runtime without breaking any end-user functionality and this subset seems like it should fit that bill - please let me know if I'm wrong.

Author: crummel
Assignees: crummel
Labels:

area-Infrastructure-coreclr

Milestone: -

@carlossanlop carlossanlop changed the title Disable NativeAOT subset for source-build. [release/7.0] Disable NativeAOT subset for source-build. Sep 26, 2022
@jkotas
Copy link
Member

jkotas commented Sep 26, 2022

We are currently using the Microsoft-built NativeAOT bits in the final SDK when an end-user wants to publish a NativeAOT project. These aren't shipped with the SDK but are pulled down when an end-user wants to publish a NativeAOT project.

This is true except for the Microsoft.DotNet.ILCompiler package with build integration that is bundled into the SDK (look for Sdks\Microsoft.DotNet.ILCompiler). The way things are setup today this package has to bundled into the SDK to allow pulling down the NativeAOT compiler.

Is the Microsoft.DotNet.ILCompiler package still getting built with the right content with this change?

@jkotas
Copy link
Member

jkotas commented Sep 26, 2022

make the minimal change necessary to disable the dependency on llvm-project

This dependency is in clr.tools subset here: https://github.com/dotnet/runtime/blob/main/src/coreclr/tools/aot/ILCompiler/ILCompiler.props#L40-L47. How is disabling clr.nativeaotlibs for source build suppressing this dependency? Have you validated that this change is actually suppressing this dependency?

@crummel
Copy link
Contributor Author

crummel commented Sep 28, 2022

You're right, I need to look at this more. I'll get this updated. Thanks!

@carlossanlop carlossanlop added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 28, 2022
@carlossanlop carlossanlop marked this pull request as draft September 28, 2022 20:09
@crummel
Copy link
Contributor Author

crummel commented Oct 5, 2022

After discussion with @LakshanF and the source-build team, we will need the ObjWriter bits eventually in 7.0 even if not for NativeAOT, so I'll close this PR.

@crummel crummel closed this Oct 5, 2022
@jkotas
Copy link
Member

jkotas commented Oct 5, 2022

@crummel Could you please explain the reasoning more? As far as I know, we should not need ObjWriter in 7.0. I would like to understand why you believe that it is not the case.

@LakshanF
Copy link
Member

LakshanF commented Oct 6, 2022

That was a misunderstanding, @crummel and I chatted again and understood we don't need ObjWriter in 7.0

@crummel
Copy link
Contributor Author

crummel commented Oct 7, 2022

I've updated the PR to only remove the ObjWriter bits. I've verified runtime builds in the source-build context with this change and no longer restores the ObjWriter package. I have a build going to test that everything works in the rest of the SDK and will verify NativeAOT publishing from the end-user's perspective after that's done. Does this look like a better change @LakshanF and @jkotas ? Any other testing that seems advisable?

@jkotas
Copy link
Member

jkotas commented Oct 7, 2022

Yes, this looks better.

@crummel crummel marked this pull request as ready for review October 7, 2022 22:56
@crummel
Copy link
Contributor Author

crummel commented Oct 10, 2022

Hi @jkotas, I kicked the CI and everything is green now. I don't have merge permissions so this is good to go whenever you're ready.

@carlossanlop carlossanlop removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 10, 2022
@carlossanlop
Copy link
Member

CI is green. Removed the no-merge label since it has been signed-off by @jkotas. This is an infra-only change so no Tactics approval needed.

Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 51758ee into dotnet:release/7.0 Oct 10, 2022
@crummel
Copy link
Contributor Author

crummel commented Oct 10, 2022

Thanks!

@MichaelSimons
Copy link
Member

@crummel - did this get backported to main?

crummel added a commit to crummel/dotnet_runtime that referenced this pull request Oct 24, 2022
* Disable NativeAOT subset for source-build.

* More surgical fix - remove only the ObjWriter dependency for source-build.
@crummel
Copy link
Contributor Author

crummel commented Oct 24, 2022

I believe we worked out with @LakshanF and @jkotas that additional pieces of runtime including crossgen2 (which source-build requires) would need ObjWriter/llvm in 8.0 so we didn't want to backport this since we'll have the bits anyway. Unless I misunderstood the 8.0 plans, correct me if I'm wrong please Lakshan and Jan.

@jkotas
Copy link
Member

jkotas commented Oct 24, 2022

Yes, we will need that for the eventual unified VMR build in .NET 8/9.

This is very simple change to port and undo later. If we want to have a source build working in .NET 8 sooner rather than later, it may be worth it to port it.

jkotas pushed a commit that referenced this pull request Nov 9, 2022
…8060)

* Disable NativeAOT subset for source-build.

* More surgical fix - remove only the ObjWriter dependency for source-build.
@ghost ghost locked as resolved and limited conversation to collaborators Nov 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants