Handle ref assemblies in up-to-date check #2414

panopticoncentral · 2017-06-09T19:05:48Z

Customer scenario

Given three projects that have a dependency as such: ClassLibrary1 -> ClassLibrary2 -> ConsoleApp1, if the two class libraries use the new ref assembly feature, then if ClassLibrary1's implementation is changed but not its ref assembly, the up to date check will think that ConsoleApp1 does not need to be rebuilt. It does, though, so it can copy an updated ClassLibrary1 implementation.

Without this change building projects with reference assemblies will be broken in the IDE. For Roslyn.sln we're seeing around 70% reduction in incremental build times with reference assemblies. So we want to take this change to enable such productivity enhancements.

Bugs this fixes:

Fixes #2254

Workarounds, if any

The user would have to realize that one of their assemblies is out of date and do a rebuild.

Risk

Low. The marker file we depend on is only produced in the new ref assembly case, so it doesn't affect existing projects. Fast up to date checks can be turned off if needed.

Performance impact

Low. We check timestamps on a handful of extra files, small compared to the list of files we already check.

Is this a regression from a previous update?

No.

panopticoncentral · 2017-06-09T19:07:26Z

This changes how the up-to-date check collects references because ResolveProjectReferences is too early. We need to wait until project references have been redirected to ref assemblies, which happens later. It seemed easiest to just pick up the list of actual references that Compile task will use. Let me know if there seems to be a problem with this.

panopticoncentral · 2017-06-09T19:08:29Z

cc: @rainersigwald, @jcouv

basoundr · 2017-06-09T20:42:32Z

Are there no tests for this component?

panopticoncentral · 2017-06-09T21:05:48Z

Unfortunately, not yet. We need integration tests, so as soon as they are running, I'm going to add some.

jcouv · 2017-06-09T23:17:47Z

Thanks!
I can't say that I fully understand the change though, so I'll let reviews the project-system team.

I'll stop by Monday to discuss the testing strategy (I'm not familiar with the integration tests). A few scenarios come to mind with the 3 project setup (ref assemblies turned on):
(1) build from scratch,
(2) build with no change (skips all),
(3) add comment into lowest-level library and build,
(4) build with no change,
(5) make a significant change in lowest level library and build.
Scenarios with ref assemblies turned on only in some projects coud be interesting as well. I'm assuming that old scenarios with ref assemblies turned off are already covered.

panopticoncentral · 2017-06-12T16:26:35Z

Ping @dotnet/project-system

rainersigwald

Mostly concerned about the implementation-assembly-of-references case we discussed in dotnet/msbuild#2180 (comment).

rainersigwald · 2017-06-12T16:35:02Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/BuildUpToDateCheck.cs

+                }
+                logger.Verbose("Found output marker '{0}'.", _markerFile);
+
+                var latestInputMarker = GetLatestInput(_referenceMarkerFiles, timestampCache);


Possible future improvement: you don't actually need the latest here, since the only question is "is any reference newer than the output". You might want to look at MSBuild's IsAnyOutOfDate for ideas.

rainersigwald · 2017-06-12T16:38:51Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/BuildUpToDateCheck.cs

                {
-                    _references[referenceSchema] = new HashSet<string>(changes.After.Items.Select(item => item.Value[ResolvedPath]));
+                    _compilationReferences.Add(item.Value[ResolvedPath]);


And also

_compilationReferences.Add(item.Value[OriginalPath]);

if it exists?

jmarolf · 2017-06-12T17:55:06Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/BuildUpToDateCheck.cs

+            if (e.ProjectChanges.TryGetValue(ResolvedAnalyzerReference.SchemaName, out var changes) &&
+                changes.Difference.AnyChanges)
+            {
+                _analyzerReferences = new HashSet<string>(changes.After.Items.Select(item => item.Value[ResolvedPath]));


Which sort of string comparison do we want to use for this HashSet?

rainersigwald · 2017-06-12T21:00:54Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/BuildUpToDateCheck.cs

@@ -118,12 +120,27 @@ private void OnProjectChanged(IProjectSubscriptionUpdate e)
            _isDisabled = disableFastUpToDateCheckString != null && string.Equals(disableFastUpToDateCheckString, TrueValue, StringComparison.OrdinalIgnoreCase);

            _msBuildProjectFullPath = e.CurrentState.GetPropertyOrDefault(ConfigurationGeneral.SchemaName, ConfigurationGeneral.MSBuildProjectFullPathProperty, _msBuildProjectFullPath);
-            foreach (var referenceSchema in ReferenceSchemas)
+            _markerFile = e.CurrentState.GetPropertyOrDefault(ConfigurationGeneral.SchemaName, ConfigurationGeneral.CopyUpToDateMarkerProperty, null);


In dotnet/msbuild#2213 I'd like to change this to be an item instead of a property. Reasoning is embedded there. Is that a problem?

That might actually make it easier i think. #2416 is making it so that we just query the projectitemschemaservice for all item types and we can add CopyUpToDateMarker item to the schema. If we did that, do we still need the CheckMarkers method below? Ideally if we can avoid any special code here for the markers that would be ideal.

Unfortunately, we still need CheckMarkers. The markers have to be checked independently of the project's inputs/outputs because they are independent of one another (i.e. a dependent assembly might have an implementation-only change that won't cause the outputs of the project to be rebuilt).

panopticoncentral · 2017-06-16T00:01:50Z

OK, I've updated this review to handle CopyUpToDate as an item. I had to change slightly how we handle non-build items like CopyUpToDate, UpToDateCheckInput, and UpToDateCheckOutput to make the UI work correctly (i.e. not have them show up in Solution Explorer).

srivatsn · 2017-06-20T17:52:25Z

Is this ready now? @jcouv @panopticoncentral ?

jcouv · 2017-06-20T18:01:15Z

I have been using it yesterday and this morning. Seems to work on Roslyn.
I'll run some additional pointed validation this afternoon (Jason's scenarios dotnet/roslyn#18612 (comment))

srivatsn · 2017-06-20T18:05:39Z

Tagging @MattGertz for Preview4

panopticoncentral · 2017-06-21T06:30:14Z

@jcouv Can you test some more? I added two fixes:

We could get into a state where a project loses track of it's own marker file. I think this was what was causing the check to fail after a while but succeed when you closed and reopened the project.
The algorithm we came up with had to be slightly modified. A project's marker file needs to be compared against the marker files AND the implementation DLLs of any projects it references that produce ref assemblies. We can't just compare against marker files because the project we reference might not itself reference projects that have ref assemblies, in which case it won't have a marker file. If this doesn't make sense, we can talk about it tomorrow.

Regardless, the scenario we were testing today now seems to work correctly.

jcouv · 2017-06-21T15:59:21Z

I'll try this iteration in the morning.
As for the algorithm, what you describe seems correct according to #2254 (see the diagram) if we consider that projects without refout don't produce a marker file.
If the upstream project's primary output is fresher than my output marker, I need to build.

Handle ref assemblies

fb019db

dnfclas added the cla-already-signed label Jun 9, 2017

rainersigwald reviewed Jun 12, 2017

View reviewed changes

jmarolf reviewed Jun 12, 2017

View reviewed changes

rainersigwald mentioned this pull request Jun 12, 2017

Late bind CopyUpToDateMarker dotnet/msbuild#2213

Merged

rainersigwald reviewed Jun 12, 2017

View reviewed changes

panopticoncentral mentioned this pull request Jun 13, 2017

Up-to-date checks: UpToDateCheckInput and UpToDateCheckOutput #2416

Merged

panopticoncentral added 2 commits June 14, 2017 13:10

Merge upstream/master

bfc28f4

Move to item rather than property

a839b33

panopticoncentral added 2 commits June 16, 2017 09:23

Fix issue.

2d0f843

Fix marker check

32b9b66

jcouv mentioned this pull request Jun 20, 2017

Assertion when opening Build settings page on CSharpCompilerSymbolTest project dotnet/roslyn#20331

Closed

srivatsn mentioned this pull request Jun 20, 2017

Assertion when opening Build settings page on CSharpCompilerSymbolTest project #2471

Closed

srivatsn approved these changes Jun 20, 2017

View reviewed changes

MattGertz added Pending director approval Approved to merge and removed Pending director approval labels Jun 20, 2017

panopticoncentral added 2 commits June 20, 2017 13:45

Fix problem with non-existent marker files

9355c9e

Some more fixes

e88ebd5

panopticoncentral merged commit d2d39fc into dotnet:master Jun 21, 2017

panopticoncentral deleted the refassem branch June 21, 2017 19:52

jcouv mentioned this pull request Jun 22, 2017

VS deadlocked during up-to-date check #2474

Closed

This was referenced Oct 1, 2018

Default ProduceReferenceAssembly to true dotnet/sdk#2521

Closed

Legacy project system fast up-to-date check does not work with ProduceReferenceAssembly=true #4079

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle ref assemblies in up-to-date check #2414

Handle ref assemblies in up-to-date check #2414

panopticoncentral commented Jun 9, 2017 •

edited by srivatsn

Loading

panopticoncentral commented Jun 9, 2017

panopticoncentral commented Jun 9, 2017

basoundr commented Jun 9, 2017

panopticoncentral commented Jun 9, 2017

jcouv commented Jun 9, 2017

panopticoncentral commented Jun 12, 2017

rainersigwald left a comment

rainersigwald Jun 12, 2017

rainersigwald Jun 12, 2017

jmarolf Jun 12, 2017

rainersigwald Jun 12, 2017

srivatsn Jun 13, 2017

panopticoncentral Jun 13, 2017

panopticoncentral commented Jun 16, 2017

srivatsn commented Jun 20, 2017

jcouv commented Jun 20, 2017

srivatsn commented Jun 20, 2017

panopticoncentral commented Jun 21, 2017

jcouv commented Jun 21, 2017

Handle ref assemblies in up-to-date check #2414

Handle ref assemblies in up-to-date check #2414

Conversation

panopticoncentral commented Jun 9, 2017 • edited by srivatsn Loading

panopticoncentral commented Jun 9, 2017

panopticoncentral commented Jun 9, 2017

basoundr commented Jun 9, 2017

panopticoncentral commented Jun 9, 2017

jcouv commented Jun 9, 2017

panopticoncentral commented Jun 12, 2017

rainersigwald left a comment

Choose a reason for hiding this comment

rainersigwald Jun 12, 2017

Choose a reason for hiding this comment

rainersigwald Jun 12, 2017

Choose a reason for hiding this comment

jmarolf Jun 12, 2017

Choose a reason for hiding this comment

rainersigwald Jun 12, 2017

Choose a reason for hiding this comment

srivatsn Jun 13, 2017

Choose a reason for hiding this comment

panopticoncentral Jun 13, 2017

Choose a reason for hiding this comment

panopticoncentral commented Jun 16, 2017

srivatsn commented Jun 20, 2017

jcouv commented Jun 20, 2017

srivatsn commented Jun 20, 2017

panopticoncentral commented Jun 21, 2017

jcouv commented Jun 21, 2017

panopticoncentral commented Jun 9, 2017 •

edited by srivatsn

Loading