-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Don't strip default items from build-related Targets when building runnable apps #50942
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
Don't strip default items from build-related Targets when building runnable apps #50942
Conversation
…ps and/or compute their run arguments we include default items This is load-bearing for some project types, so we instead just make sure that restore-only pathways keep the old default-item exclude
| public static Dictionary<string, string> GetGlobalPropertiesFromArgs(MSBuildArgs msbuildArgs) | ||
| { | ||
| var globalProperties = msbuildArgs.GlobalProperties?.ToDictionary() ?? new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); | ||
| globalProperties[Constants.EnableDefaultItems] = "false"; // Disable default item globbing to improve performance |
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.
Consider adding some tests that would reproduce the issue which this is fixing.
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 will probably add a test upstream in dotnet/android, I don't know how you'd reproduce the problem without the Android workload:
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 think it should be fairly easy to repro in dotnet/sdk by adding <CallTarget Targets="Build" /> or something similar in a custom target that runs before/after ComputeRunArguments.
|
I want to get this in for RC2 and hopefully not wrestle with CI failures more than necessary, so I'll log a follow-up issue to create a test case to pin the behavior. |
Potential fix for dotnet/android#10500.
Historically we've included
EnableDefaultItems=falsein purely-evaluation or Restore scenarios as a free performance bump, since doing MSBuild glob expansion is a performance hit that isn't generally necessary in those cases.However, we were including this flag even when running run-related Targets that in android's case involve actual building, so this assumption no longer holds. Due to this, we remove the optimization.