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

Prepend the last modified project to MSBuildAllProjects #3605

Merged
merged 5 commits into from Aug 14, 2018

Conversation

@jeffkl
Copy link
Contributor

commented Aug 14, 2018

During evaluation, keep track of the last modified project in the import graph.
Prepend the path to the last modified project to MSBuildAllProjects
Stop setting MSBuildAllProjects in our targets
Add unit tests

Closes #1299

jeffkl added some commits Aug 14, 2018

Prepend to MSBuildAllProjects with the last modified project
During evaluation, keep track of the last modified project in the import graph.
Prepend the path to the last modified project to `MSBuildAllProjects`
Stop setting `MSBuildAllProjects` in our targets
Add unit tests

Closes #1299

@jeffkl jeffkl closed this Aug 14, 2018

@jeffkl jeffkl reopened this Aug 14, 2018

@@ -1997,7 +1998,7 @@ public void AllEvaluatedPropertiesAndImports()
// the whole list.
// We have to dump it into a dictionary because AllEvaluatedProperties contains duplicates, but we're preparing to Properties,
// which doesn't, so we need to make sure that the final value in AllEvaluatedProperties is the one that matches.
foreach (ProjectProperty property in project.AllEvaluatedProperties.TakeWhile(property => property.Xml == null))
foreach (ProjectProperty property in project.AllEvaluatedProperties.Where(property => property.Xml == null))

This comment has been minimized.

Copy link
@rainersigwald

rainersigwald Aug 14, 2018

Contributor

Why this change? If it's needed, please update the comment to remove the "don't need to scan the whole list" bit.

This comment has been minimized.

Copy link
@jeffkl

jeffkl Aug 14, 2018

Author Contributor

This code assumes that all reserved properties are at the beginning of the list. Originally I had added a new reserved property but it was at the end so this test broke. Should I update the comment or revert this change?

This comment has been minimized.

Copy link
@rainersigwald

rainersigwald Aug 14, 2018

Contributor

I'd just update the comment. It's test code; the perf benefit of an early stop doesn't strike me as worth much.

File.SetLastWriteTime(project2.ProjectFile, DateTime.Now);
File.SetLastWriteTime(primaryProject.ProjectFile, DateTime.Now.AddHours(-1));


This comment has been minimized.

Copy link
@rainersigwald

rainersigwald Aug 14, 2018

Contributor

nit: extra blank line


Project project = new Project(primaryProject.ProjectFile, null, null);

project.GetPropertyValue(Constants.MSBuildAllProjectsPropertyName).ShouldStartWith(project2.ProjectFile);

This comment has been minimized.

Copy link
@rainersigwald

rainersigwald Aug 14, 2018

Contributor

Validate no project1 path?

Constants.MSBuildAllProjectsPropertyName,
oldValue == null
? _lastModifiedProject.FullPath
: String.Format(CultureInfo.CurrentCulture, "{0};{1}", _lastModifiedProject.FullPath, oldValue.EvaluatedValue), isGlobalProperty: false, mayBeReserved: false);

This comment has been minimized.

Copy link
@rainersigwald

rainersigwald Aug 14, 2018

Contributor

Do we want CurrentCulture? Shouldn't it be invariant?

This comment has been minimized.

Copy link
@jeffkl

jeffkl Aug 14, 2018

Author Contributor

All other places in this file use CurrentCulture...

This comment has been minimized.

Copy link
@rainersigwald

rainersigwald Aug 14, 2018

Contributor

Consistency is good but why?

¯_(ツ)_/¯

Meh.

This comment has been minimized.

Copy link
@AndyGerlicher

AndyGerlicher Aug 14, 2018

Member

I'd change it. And just use string interpolation rather than string.Format.

jeffkl added some commits Aug 14, 2018

@jeffkl jeffkl merged commit d42d350 into microsoft:master Aug 14, 2018

6 of 8 checks passed

Windows_NT Build for Full Build finished.
Details
Windows_NT Build for CoreCLR Triggered. (1/4 on Windows.10.Amd64.ClientRS3.DevEx.Open)
Details
OSX10.13 Build for CoreCLR Build finished.
Details
RHEL7.2 Build for CoreCLR Build finished.
Details
Ubuntu16.04 Build for CoreCLR Build finished.
Details
VSTS: msbuild-pr 20180814.16 succeeded
Details
WIP ready for review
Details
license/cla All CLA requirements met.
Details

@jeffkl jeffkl deleted the jeffkl:lastmodifiedproject branch Aug 14, 2018

jeffkl added a commit to jeffkl/NuGet.Client that referenced this pull request Aug 15, 2018

Set MSBuildAllProjects only for MSBuild 15.0 or below in nuget.g.props.
In MSBuild 16.0, MSBuildAllProjects is automatically the last modified project so that each import does not have to add to the list.  We found that the property was being set 35+ times during project evaluation so this feature was added.  Now we need to stop setting it to make a performance impact.

To maintain back-compat, it is still set for older versions of MSBuild.

microsoft/msbuild#3605

drewnoakes added a commit to drewnoakes/project-system that referenced this pull request Sep 14, 2018

drewnoakes added a commit to drewnoakes/project-system that referenced this pull request Sep 17, 2018

sidiesen added a commit to sidiesen/msbuild that referenced this pull request Oct 4, 2018

Prepend the last modified project to MSBuildAllProjects (microsoft#3605)
During evaluation, keep track of the last modified project in the import graph.
Prepend the path to the last modified project to `MSBuildAllProjects`
Stop setting `MSBuildAllProjects` in our targets
Add unit tests

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