Skip to content
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 set MSBuildAllProjects for MSBuild 16.0+ #2853

Merged
merged 3 commits into from Jan 22, 2019

Conversation

@drewnoakes
Copy link
Member

commented Jan 21, 2019

microsoft/msbuild#1299 discusses a change whereby MSBuild prepends the newest project file path to MSBuildAllProjects automatically. Therefore, so long as consumers of this property only use this property for up-to-date checks, it's possible to reduce both the size of this property and corresponding allocations.

This reduction applies equally to consumers, such as for the case discussed in dotnet/project-system#3744.

For backwards compatibility the property is still set for MSBuild versions prior to 16.0.

See also microsoft/msbuild#3605.

//cc: @jeffkl @rainersigwald @davkean

Don't set MSBuildAllProjects
microsoft/msbuild#1299 discusses a change whereby MSBuild prepends the newest input file path to this property automatically. Therefore, so long as consumers of this property only use this list for up-to-date checks, it's possible to reduce the size of this property, and reduce corresponding allocations.

This reduction applies equally to consumers, such as in the case discussed in dotnet/project-system#3744

For backwards compatibility the property is still set for MSBuild versions prior to 16.0.
@jeffkl

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

This looks good, here was my commit that I hadn't had a change to submit yet:

jeffkl@0510dc1

I'm surprised you didn't have to update the unit test like I had to.

@drewnoakes

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2019

here was my commit that I hadn't had a change to submit yet

If it's safe to remove them outright then that's a better approach, and given your commit we can close this PR.

I'm surprised you didn't have to update the unit test like I had to.

It's marked as Skip. It does look like it should be removed, as you did.

@jeffkl

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

My commit is from August and I haven't had time to send a PR so feel free to merge this PR instead.

@drewnoakes

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2019

In that case I'll remove the skipped unit test and remove the < 16.0 checks altogether.

drewnoakes added some commits Jan 22, 2019

Remove adding to MSBuildAllProjects altogether
Usages of these files will at least run MSBuild 16.0 so the backwards
compatibility is not required.
@rainersigwald
Copy link
Contributor

left a comment

🎉

I closed #1810 since this will make it super obsolete.

@davkean

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

The CLI will require Dev16, which means if someone tries to use this in Dev15, they will have to do hacks to enable it and we don't support it.

How is that enforced? ie What will prevent this from "just working" if I run MSBuild.exe (v15) against a project that only has 3.0 installed.

@livarcocc

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

@davkean the 3.0 CLI has a file that establishes 16.0 as the minimum MSBuild version it requires. This file is then honored by the SDK Resolver that msbuild uses to find the .NET Core SDK props/targets. Users would have to edit this file inside the SDK installation to get around that. At that point, I would say all bets are OFF.

@davkean

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

👍

@nguerrera nguerrera merged commit 2eb6c54 into dotnet:master Jan 22, 2019

3 checks passed

WIP Ready for review
Details
license/cla All CLA requirements met.
Details
public-CI (3) #20190122.1 succeeded
Details

@drewnoakes drewnoakes deleted the drewnoakes:msbuild-all-projects branch Jan 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.