-
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
Crossgen Task #2997
Crossgen Task #2997
Conversation
@nguerrera Here's the crossgen task. It's still WIP since I haven't tested on OSX yet (tested on Linux and Windows). |
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.TargetingPackResolution.targets
Outdated
Show resolved
Hide resolved
Tested on OSX |
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 am not finished reviewing everything, but didn't want to hold back these comments.
src/Tasks/Microsoft.NET.Build.Tasks/ResolveFrameworkReferences.cs
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/ResolveFrameworkReferences.cs
Outdated
Show resolved
Hide resolved
@nguerrera Doesn't look like I have access to merge this. Could you please take another look at the new changes I posted, and merge if it looks good? |
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
Show resolved
Hide resolved
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.
Sorry for the delay in doing a full review. I have some more concerns noted above.
Also, tests need to be added.
@nguerrera I changed the platform validation logic a bit to consume the runtime graph as we discussed, to validate target/host platform compatibility. It now does so by comparing the best matching RID of the target to the best matching RID of the host. Tested it on Ubuntu, OSX, Alpine, and RHEL.6. I need some pointers on how to add a test to the CI. I can add it as a separate PR. |
75b4b85
to
a7933fa
Compare
For the tests, look a GuvenThatWeWantToCrossPublish for simple example. You probably want to mix this with PackageReferences and ProjectReferences to get more than one file to crossgen and to test more cases. GivenThatWeWantDesignerSupport shows this. You can enable ReadyToRun via TestProject.AdditionalProperties. It would be nice to have negative coverage for the mismatched host, and coverage of all the options (PDBs, exclude list). |
We always include tests in the original PR. I've put some pointers above. Feel free to grab some time with me if you want to pair on this or have more questions about it. |
src/Tests/Microsoft.NET.TestFramework/ProjectConstruction/TestProject.cs
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToRunCrossgen.cs
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
Outdated
Show resolved
Hide resolved
c30e095
to
5a5dad2
Compare
… targets to invoke the task. Added new condition to download runtime packs: when the ReadyToRun property is set. Adding R2R exclusion list capability
…ring to have a ToolTaskBase) Fixing logic with RuntimeIdentifier values used by the task (now it's read from the RuntimePack)
…ferenceAssemblyAttribute
… validation to use the runtime graph data
…that calls ExecuteTool multiple times. The split also enables msbuild to automatically skip entries that are already up to date. Removed ToolTaskBase since it's not really adding much value
5a5dad2
to
5e43db7
Compare
5e43db7
to
ec53a2a
Compare
….1 (#2997) - Microsoft.DotNet.Cli.Runtime - 5.0.100-alpha1.19480.1
Intitial implementation of the crossgen task, and plumbing in the SDK targets to invoke the task. The crossgen task invokes crossgen from the runtime pack.