-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add back _RequiresILLinkPack override #34429
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
Conversation
EnableSingleFileAnalyzer is set by default for PublishSingleFile. _RequiresILLinkPack must be computed late enough to see the inferred value.
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.
Thanks!
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
Outdated
Show resolved
Hide resolved
|| IsTrimmable || EnableTrimAnalyzer | ||
|| EnableSingleFileAnalyzer; | ||
if (requiresILLinkPack) | ||
if (RequiresILLinkPack) |
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.
Would it make sense to generate the errors outside of the ProcessFrameworkReferences
task? Then I tihnk we could remove all of these additional task parameters that were added.
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.
That seems reasonable, I'll look into it in a follow-up.
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 started trying this, but I'm not sure what is the best way to write the conditional logic in MSBuild. I could create another task for the error reporting, but at that point it seems like it would be simpler to report the errors from ProcessFrameworkReferences. Currently I think doing it from PFR is the simpler approach, but let me know if you have any ideas. Here's the WIP: 5e22d0a
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.
Thanks for looking at it. If it seems like there's not an obvious way to simplify it well, then we can leave it as it is.
#34077 removed the ability to download the ILLink pack by setting
_RequiresILLinkPack
. This is needed for macios scenarios where the ILLink pack should be restored, but it's not known ahead-of-time whether trimming will be enabled. See dotnet/macios#17227 for more context.This adds back the ability to set
_RequiresILLinkPack
to download the ILLink pack even if none of the other relevant settings are enabled.