Use implicit package reference for ILC build package#28690
Merged
sbomer merged 11 commits intodotnet:mainfrom Oct 26, 2022
Merged
Use implicit package reference for ILC build package#28690sbomer merged 11 commits intodotnet:mainfrom
sbomer merged 11 commits intodotnet:mainfrom
Conversation
ghost
added
the
- With implicit package reference, the bundled SDK is unused - The target-RID package will be downloaded for the implicit package reference
sbomer
requested review from
LakshanF and
agocke
and removed request for
LakshanF
LakshanF
approved these changes
Oct 25, 2022
Contributor
LakshanF
left a comment
LakshanF
left a comment
There was a problem hiding this comment.
Thanks for doing this!
Its a great start and we should add the test scenarios for major version changes once the repo is ready. There are 4 different scenarios, and we currently only test the 1st one thoroughly.
- Default: User only specifies PublishAot at publish. All package versions (both ILC build and runtime packages and the .NET Core App Runtime) match and there should not be any versioning issues. We have existing test scenarios for this.
- User explicitly overrides the .NET Core App Runtime version. We need to ensure that all 3 package versions match as above. This is a critical version requirement since previously (without this fix), the bundled ILC build package targets and properties will be used causing versioning issues. This fix should address that, and we need a test case once the repo is ready.
- User explicitly specifies an ILC build package version. We discourage this scenario and only allow it for narrow cases (for example, pick a bug fix in the NativeAOT packages) where the version mismatch with the .NET Core App Runtime version is expected to be minor. We do have a test scenario currently for this but do not comprehensively validate the versions.
- User explicitly overrides the .NET Core App Runtime version and also explicitly specifies an ILC build package version. Not sure what the user scenario here is but treating this as scenario 2 seems reasonable to me. We don’t have a test scenario for this currently.
Member
Author
|
Thanks @LakshanF. I added some validation of the explicit versions for (3). |
2 tasks
LakshanF
approved these changes
Oct 26, 2022
Contributor
LakshanF
left a comment
LakshanF
left a comment
There was a problem hiding this comment.
Thanks for adding the explicit ilc path check to the test!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This addresses parts of dotnet/runtime#75468 by removing the bundled ILCompiler bits from the SDK, and instead using implicit PackageReference to import targets that correctly match the ILCompiler package version.
This seems to work fine for all of the 7.0 scenarios we are testing in ci and for my manual tests. I can't add tests targeting 8.0 yet because the repo hasn't updated to enable targeting 8.0 (and I think that is blocked on the templates updating to 8.0).