-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Corresponding PublishAOT changes to match the runtime cleanup changes #27159
Conversation
src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAnAotApp.cs
Outdated
Show resolved
Hide resolved
var testProject = CreateHelloWorldTestProject(targetFramework, projectName, true); | ||
|
||
// This will add a reference to a package that will also be automatically imported by the SDK | ||
testProject.PackageReferences.Add(new TestPackageReference("Microsoft.DotNet.ILCompiler", "7.0.0-*")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you have a chance to think about how we could remove this source of non-determinism (#27098 (comment))?
I see two options:
- Make it so that we know the version here. Pass it from the build somehow.
- Make it so that we know the version at test build time. AFAIK this gets spilled into a csproj, so the version string could also be
$(TheILCompilerPackageVersion)
and it will expand then at the time the test project is built. We just need something in the SDK to tell us what ILCompiler package it's including.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Maybe this doesn't have to be addressed here, just asking to make sure you saw it)
// This will add a reference to a package that will also be automatically imported by the SDK | ||
testProject.PackageReferences.Add(new TestPackageReference("Microsoft.DotNet.ILCompiler", "7.0.0-*")); | ||
testProject.PackageReferences.Add(new TestPackageReference("runtime.win-x64.Microsoft.DotNet.ILCompiler", "7.0.0-*")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect crossbuild with PublishAot not to require an ILCompiler PackageReference.
People won't know what package version to include. The SDK should include all the necessary packages for them. The experience of managing ILCompiler packages version is subpar - we only want it for hotfixes, not as a way for people to consume PublishAot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. This requires the runtime changes to be present to set the runtime package directory to get the platform link,
a85ed8e
to
a182e3b
Compare
Supporting
PublishAOT
scenarios as described in dotnet/runtime#72415 specifically, in the comments here.