Add extensibility points for source link #2960

tmat · 2018-02-06T19:18:05Z

Adds a few extensibility points to common targets that enable source control and SourceLink package features and fully deterministic compilation.

See https://github.com/tmat/repository-info/blob/master/docs/Readme.md for details.

TODO:

Follow up change in dotnet SDK to include revision id in AssemblyInformationalVersion
Follow up change in C#, VB and F# targets files to use source roots to generate PathMap (Implements targets that calculate PathMap from SourceRoots roslyn#25325)
Follow up change in C++ compiler to implement deterministic path mapping and use source roots to generate it
Follow up change in NuGet targets to populate RepositoryUrl from PrivateRepositoryUrl
Create source control and source link packages using these extensibility points
Create Microsoft.Build.StandardCI package

tmat · 2018-02-06T19:18:14Z

@nguerrera

tmat · 2018-02-06T21:02:32Z

@rainersigwald Please review.

Not sure why the tests are failing with

System.InvalidOperationException : Task attempted to log before it was initialized. Message was: The value of SourceRoot.ContainingRoot not found in SourceRoot items: 'c:\\MyProjects\\MyProject\\'

They work on my box. Is log initialization different on Linux/OSX?

tmat · 2018-02-06T22:04:58Z

@AndyGerlicher This PR adds the minimal amount of extensibility points to the common targets that are needed for linking PDBs and packages to the originating source (via SourceLink, commit sha in AssemblyInformationalAttribute, Repository Url in nuget packages, etc.) and for compilation with deterministic source paths.

tmat · 2018-02-06T22:24:45Z

@jinujoseph @richlander FYI

AndyGerlicher · 2018-02-06T22:29:45Z

@tmat not sure if I'm just missing something at the moment, haven't tried it out. But why couldn't all this all come from a package reference?

tmat · 2018-02-06T22:32:26Z

@AndyGerlicher Because NuGet targets, dotnet SDK and the compiler targets will have a dependency on these properties and targets.

tmat · 2018-02-06T22:33:38Z

@AndyGerlicher I did reduce the set of targets and properties that go to common targets to minimum. I don't think there is anything more that can be reasonably removed without breaking layering or introducing hacks.

nguerrera · 2018-02-06T23:37:24Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+    </ItemGroup>
+
+    <Error Text="SourceRoot paths are required to end with a slash or backslash: @(_InvalidSourceRoot, ', ')"
+           Condition="'@(_InvalidSourceRoot)' != ''" />


Why can't we just add them? That is what common targets normally does for this.

Do we know which separator to add on Windows (\ or /)? The source root path needs to end with exactly the same separator that the full paths of source files that flow into the compiler use.
Is it possible that the compiler will see a source path with / in it on Windows?

There's some magic that changes backslash to slash for ToolTask invocation on Unix

Take a deep breath:

https://github.com/Microsoft/msbuild/blob/cc9cc0c54454d427b12d73255b950908894ed10e/src/Utilities/ToolTask.cs#L1725

So I think appending \ will give the compiler \ on Windows and / on Unix.

I think it is possible for the compiler to see / on Windows, but not \ on Unix.

Why can't/shouldn't the compiler understand that there's no difference between / and \ on Windows?

Source paths in the PDB are matched to the patterns in SourceLink by simple prefix match when interpreted by the debugger.

I don't think requiring the path to end with slash is an issue in practice. SourceRoot items will usually be generated by another build task. Writing SourceRoots manually should be very rare.

I don't think it's much of an issue. I asked because the append-trailing-slash-if-not-already-there idiom is pervasive in common targets. If there's a good reason to do it differently here, that's fine. Might be worth a comment.

Will add a comment.

nguerrera · 2018-02-06T23:44:43Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+
+  <Target Name="SetSourceRevisionId">
+    <PropertyGroup>
+      <SourceRevisionId>$(STANDARD_CI_SOURCE_REVISION_ID)</SourceRevisionId>


Shouldn't this allow the user to override $(SourceRevisionId) just like $(PrivateRepositoryUrl) below?

Fixed. Using the same pattern as InitializePrivateRepositoryUrl

nguerrera · 2018-02-06T23:45:22Z

src/Tasks/Microsoft.Common.props

+      Build servers that comply with Standard CI specification set STANDARD_CI_REPOSITORY_URL and STANDARD_CI_SOURCE_REVISION_ID variables.
+      See https://github.com/dotnet/designs/blob/86f4ed0e39fc1b1ab5c4128990b17c4aead4420f/proposed/standard-ci-env-variables.md.
+    -->
+    <ContinuousIntegrationBuild Condition="'$(DesignTimeBuild)' != 'true' and '$(STANDARD_CI_REPOSITORY_URL)' != ''">true</ContinuousIntegrationBuild>


We should check if ContinuousIntegrationBuild is already set, no?

nguerrera · 2018-02-06T23:47:27Z

src/Tasks/MapSourceRoots.cs

+        public ITaskItem[] SourceRoots { get; set; }
+
+        [Output]
+        public ITaskItem[] MappedSourceRoots { get; set; }


private set for Output

nguerrera · 2018-02-07T00:59:45Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+      <_InvalidSourceRoot Include="@(SourceRoot)" Condition="!HasTrailingSlash('%(SourceRoot.Identity)')" />
+    </ItemGroup>
+
+    <Error Text="SourceRoot paths are required to end with a slash or backslash: @(_InvalidSourceRoot, ', ')"


Localization?

How do I localize an error message in .targets file?

In dotnet/sdk we have a custom task that we pass resource name and format arguments.

But it seems there is precedent for unlocalized errors in common targets:

https://github.com/Microsoft/msbuild/blob/356e43229e0ea0ba7bf4e8cade7ec3fe2ede7967/src/Tasks/Microsoft.Common.CurrentVersion.targets#L1003

I would think this is against the letter of the World Ready tenet. Given that we've already said that these are generally automatically provided by tooling and not typed in by users, the error would be rare, so it's probably OK in spirit.

@cdmihai Thoughts?

Given that there's other error tasks in the file and I searched the visual studio drops for all occurrences of Microsoft.Common.CurrentVersion.targets but did not find anything, it's probably safe to say that, because it's always been not localized, it can remain not localized.

tmat · 2018-02-14T02:40:31Z

@rainersigwald All tests fixed. Thanks for help. Does the change look good now?

tmat · 2018-02-21T01:34:15Z

@rainersigwald ping

tmat · 2018-02-21T01:34:32Z

test Ubuntu16.04 Build for CoreCLR please

rainersigwald

This seems like a straightforward addition of some properties, targets, and items. I still don't understand why this can't be done in a package. What information is required where? What properties and items must exist, where their absence cannot be interpreted as not opting in to the new behavior?

rainersigwald · 2018-02-26T16:53:01Z

To be clear, I'm thinking of a package like SourceLink.Base that:

Delivers the MapSourceRoots task
Defines a bunch of default properties in its .props
Defines the targets here
Is depended upon by specific plugins, like the Git provider

Someone could still override InitializeSourceRevisionId (define it in a NuGet package that depends on this package, to say nothing of adding to $(InitializeSourceRevisionIdDependsOn)) and PrivateRepository too.

tmat · 2018-02-27T03:27:59Z

I still don't understand why this can't be done in a package.

The properties and targets defined in this change are intended to be used at least in the following targets:

NuGet.Build.Tasks.Pack.targets
So that Pack task can embed revision id (e.g. commit SHA for git repos) and repository URL into the package nuspec.
Microsoft.CppBuild.targets
So that C++ compiler can use SourceRoots to generate deterministic path map.
Microsoft.CSharp.Core.targets, Microsoft.VisualBasic.Core.targets, Microsoft.FSharp.Core.targets
So that C#, VB and F# compilers can use SourceRoots to generate deterministic path map.
Microsoft.NET.GenerateAssemblyInfo.targets
So that .NET SDK can include revision id in AssemblyInformationalVersion attribute.

If the targets added by this PR were in a package than all of the above targets would need to depend on that package somehow, which is impossible as far as I can tell.

tmat · 2018-02-27T03:46:39Z

package than all of the above targets would need to depend on that package somehow

I guess that's not strictly true. We could alternatively copy these targets into all of the above targets files and make a few copies of the MapSourceRoots task. That would probably work. But then what are common targets good for if not for sharing targets that are common?

eerhardt · 2018-02-27T14:35:16Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

Should _InitializeSourceRevisionIdFromStandardCI go after $(InitializeSourceRevisionIdDependsOn)? That way other extensions can hook into this target, and they don't have to blindly overwrite the property?

As it is coded now, if an extension wants to write to <SourceRevisionId>, it can't check for empty, because _InitializeSourceRevisionIdFromStandardCI will set it first.

Same applies to InitializePrivateRepositoryUrl below.

That depends on whether STANDARD_CI_SOURCE_REVISION_ID should be the default value that should be used if available or a fallback value that should be used if there is no extension setting SourceRevisionId, right?

The extension could decide to

add itself to InitializeSourceRevisionIdDependsOn, or

chain itself before InitializeSourceRevisionIdFromStandardCI target to initialize to a different default value like so:

<Target Name="MyInitializeSourceRevisionIdFromStandardCI" BeforeTargets="InitializeSourceRevisionIdFromStandardCI"> <PropertyGroup> <SourceRevisionId Condition="'$(SourceRevisionId)' == ''">MyRevisionId</SourceRevisionId> </PropertyGroup> </Target>

Think about if you were writing an extension to this target and you wanted to set <SourceRevisionId>. How would you do it?

Maybe instead of using a Target to set this property, you should just set the property in a normal <PropertyGroup>. Then the typical MSBuild ordering applies. An extension can set it either before OR after you. Here they only get 1 option - after.

I have a working extension that does it: https://github.com/tmat/repository-info/blob/master/src/Microsoft.Build.Tasks.SourceControl/build/Microsoft.Build.Tasks.SourceControl.targets#L62-L67

It can't be set in normal <PropertyGroup> since a task has to be called to retrieve this value.

I guess we could also have InitializeSourceRevisionIdDependsOn initialized to _InitializeSourceRevisionIdFromStandardCI and then let extensions to prepend or append their target name to this property.

@rainersigwald Is there any guidelines for how to write these extension points?

No, no formal guidelines that I'm aware of. I added some comments about the structure I'd expect, which I think is closer to what Eric expected too.

In general, I'd rather avoid creating new xDependsOn properties. They were required for MSBuild 2, but BeforeTargets/AfterTargets is a much better extensibility mechanism.

OK. It would be nice to have these guidelines written down somewhere, so we don't need to spend time figuring this out every time.

By the way, this

AfterTargets doesn't work for the consumption. If I need to use this value in another target I add InitializeSourceRevisionId to my target's DependsOnTargets. AfterTargets of InitializeSourceRevisionId won't necessarily run.

Isn't true. It should be (targets should make a DAG), but it's not. The details of the traversal are https://docs.microsoft.com/en-us/visualstudio/msbuild/target-build-order#determining-the-target-build-order, and I don't see any way to change them to a DAG approach without breaking the whole world, so they can be relied upon.

eerhardt · 2018-02-27T14:36:50Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

'$(ContinuousIntegrationBuild)' != '' seems strange. What if it is set to false?

It seems that the condition should be '$(ContinuousIntegrationBuild)' == 'true'.

Yes! Good catch.

And assert success/failure explicitly.

rainersigwald · 2018-02-27T16:31:36Z

If the targets added by this PR were in a package than all of the above targets would need to depend on that package somehow, which is impossible as far as I can tell.

Why? Why do those target have direct dependencies on this? Can't they just use properties/items? If so, the properties/items can just not be set by default, and the other targets can treat that as "don't opt into this".

tmat · 2018-02-27T16:44:46Z

@rainersigwald How will the targets above make sure the properties are initialized before the targets run?

tmat · 2018-02-27T16:46:32Z

The properties can't be initialized during evaluation phase. Tasks need to be called to retrieve the values.

rainersigwald · 2018-02-27T16:55:11Z

It would be the responsibility of the package that delivers these targets to ensure they happen at the right time by setting the right BeforeTargets. I suspect that BeforeTargets="BeforeBuild" would do the trick though you could also mention CoreCompile and friends to be sure.

tmat · 2018-02-27T17:03:12Z

How should that package know what are all the consumers of these properties? It seems backwards.

rainersigwald · 2018-02-27T17:14:56Z

I'd be open to the idea of adding a new stub target in common targets to act as your hook point. Then any target (not Roslyn because of downlevel compat, but everybody else) could DependsOn it directly, and the package could inject targets before it. How's that sound?

tmat · 2018-02-27T17:17:04Z

@rainersigwald What do you mean by a stub target? Isn't that what this change is adding?

rainersigwald

Comments on the actual task/targets.

rainersigwald · 2018-02-27T16:56:47Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+      The usage of SourceRoot might be sensitive to what kind of separator is used (e.g. in SourceLink where it needs
+      to match the corresponding separators used in paths given to the compiler).
+    -->
+    <Error Text="SourceRoot paths are required to end with a slash or backslash: @(_InvalidSourceRoot, ', ')"


Why not do this validation in the task and get a localized error?

The task is only invoked in deterministic build. I think it's better do the validation for all builds.

To address the localization: is there a localized version of Error task for predefined resource strings?

rainersigwald · 2018-02-27T16:58:05Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+
+  <Target Name="_InitializeSourceRevisionIdFromStandardCI">
+    <PropertyGroup>
+      <SourceRevisionId Condition="'$(SourceRevisionId)' == ''">$(STANDARD_CI_SOURCE_REVISION_ID)</SourceRevisionId>


You expect STANDARD_CI_SOURCE_REVISION_ID to be set outside the build process, right? Can't this just be the default setting in the evaluation phase? Then you can eliminate this target and have only a stub for InitializeSourceRevisionId that's overridable.

rainersigwald · 2018-02-27T16:58:24Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+
+  <Target Name="_InitializePrivateRepositoryUrlFromStandardCI">
+    <PropertyGroup>
+      <PrivateRepositoryUrl Condition="'$(PrivateRepositoryUrl)' == ''">$(STANDARD_CI_REPOSITORY_URL)</PrivateRepositoryUrl>


rainersigwald · 2018-02-27T16:59:41Z

src/Tasks/Microsoft.Common.props

+  <PropertyGroup>
+    <!-- 
+      ContinuousIntegrationBuild is set to true when building on a CI server. The variable is then used to enable settings that only 
+      apply to official builds, as opposed to local builds on developer machine.


I don't think I like the name here. Even in this comment you're describing the difference between "official" builds and something else . . . why call it ContinuousIntegration?

Have a better name? Many repos call this variable CIBuild. STANDARD_CI_XXX variables include the "CI" in their name and this variable is tied to those (when available). So I think that's consistent naming.

rainersigwald · 2018-02-27T17:00:30Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+    -->
+
+    <ItemGroup Condition="'@(_MappedSourceRoot)' != ''">
+      <_MappedSourceRoot Remove="@(_MappedSourceRoot)" />


Is this just defensive? That's not something we generally do.

Yes, it's defensive since <Output> with ItemName adds to the list. Shouldn't we do it more generally? What if someone has a temporary item group of the same name?

rainersigwald · 2018-02-27T17:01:23Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+    If the project references a package that provides source control information (such as Microsoft.Build.Tasks.Git, Microsoft.Build.Tasks.TFVC, etc.)
+    the revision id becomes available in local build environments as well.
+    
+    Extensibility: Add target to InitializeSourceRevisionIdDependsOn to initialize SourceRevisionId.


Why not just override this target?

That's one way to do it. The disadvantage is that the extension can't fall back to the default (like a base call in overridden method).

rainersigwald · 2018-02-27T17:02:52Z

src/Tasks/MapSourceRoots.cs

+                MappedSourceRoots = SourceRoots;
+            }
+
+            return success;


In our experience, the best practice for returning from Task.Execute is

return !Log.HasLoggedErrors;

That helps avoid the confusing situation where you fail a task but have no error in the log about why.

rainersigwald · 2018-02-27T17:08:51Z

src/Tasks/MapSourceRoots.cs

+            }
+
+            string EndWithSlash(string path)
+                => (path[path.Length - 1] == '/') ? path : path + '/';


If this stays here you should have access to FileUtilities.EnsureTrailingSlash.

rainersigwald · 2018-02-27T17:12:19Z

src/Tasks/MapSourceRoots.cs

+            string EndWithSlash(string path)
+                => (path[path.Length - 1] == '/') ? path : path + '/';
+
+            // assign mapped paths to process source control roots first:


What's the intention behind this ordering?

In most cases there will be a single SourceRoot that corresponds to the repository root, plus some additional SourceRoots for source packages and other external source roots.

The first SourceRoot in the item group is mapped to /_/ path prefix. Since the mapped paths are visible to the user it's better if /_/ prefix is commonly used for all the files in the repo (as opposed to /_N/ prefix based on the order the targets added their SourceRoots).

I'll add a comment.

rainersigwald · 2018-02-27T17:38:27Z

No, this PR adds real targets. I'm proposing a new target like

<Target Name="GetSourceMetadata" />

Then things could DependsOnTargets="GetSourceMetadata", and InitializePrivateRepositoryUrl could BeforeTargets="GetSourceMetadata".

tmat · 2018-02-27T17:47:08Z

Sounds good for PrivateRepositoryUrl and SourceRevisionId, if we also initialize the properties from STANDARD_CI_XXX values in evaluation phase.

What about InitializeSourceRoot? Currently this target gathers the data from the extension target, validates that the gathered data are valid (paths ending with slash) and then calculates the deterministic path mapping, so that all consumers of InitializeSourceRoot have those mapped paths available.

rainersigwald · 2018-02-27T18:08:48Z

In that case I'd expect:

This package provides the InitializeSourceRoot target
The extension target is in another package that has a NuGet dependency on this one
The extension target has BeforeTargets="InitializeSourceRoot"

tmat · 2018-02-27T18:15:56Z

InitializeSourceRoot needs to be called from C#, VB, F# and C++ targets. These targets consume the mapped paths (the output of the MapSourceRoot task).

rainersigwald · 2018-02-27T18:17:23Z

I don't understand how that impacts the dependency graph above. Can you elaborate?

tmat · 2018-02-27T18:17:30Z

Currently each compiler has their own build task assembly. So to remove the MapSourceRoot from common tasks we would need 4 copies of that task.

rainersigwald · 2018-02-27T18:17:59Z

It would ship in what I'm calling FancyNew.Common.nupkg, as only one copy.

tmat · 2018-02-27T18:18:30Z

Ok, so how do I add a FancyNew.Common.nupkg dependency to C++ targets?

rainersigwald · 2018-02-27T18:22:28Z

You'd add it at the project level, like all NuGet references. Referencing a package would serve as the opt-in for this feature set.

For C++ specifically, this might require jumping through a couple of hoops until vcxproj is taught to handle PackageReference in the project, but I think it could be done with (shudder) an install.ps1.

tmat · 2018-02-27T18:23:26Z

So all projects in the world will have this NuGet reference? It shouldn't be opt-in. The intent is to make it the default.

rainersigwald · 2018-02-27T18:23:57Z

I thought that when we discussed this a while ago we all agreed that it might be default later but must be opt-in now.

tmat · 2018-02-27T18:25:13Z

Not determinism. Deterministic build is on by default for .NET Core projects. We agreed on making SourceLink opt-in. This is not about SourceLink.

The SourceRoot item group is used for both deterministic build and for SourceLink.

tmat · 2018-03-07T23:19:04Z

Superseded by #3063

tmat changed the title ~~WIP: Add extensibility points for source link~~ Add extensibility points for source link Feb 6, 2018

tmat force-pushed the SourceLinkCore branch from e227a11 to 4982fe8 Compare February 6, 2018 21:40

nguerrera reviewed Feb 6, 2018

View reviewed changes

nguerrera reviewed Feb 7, 2018

View reviewed changes

tmat force-pushed the SourceLinkCore branch from a533005 to 9c13daf Compare February 14, 2018 18:46

rainersigwald requested changes Feb 26, 2018

View reviewed changes

eerhardt reviewed Feb 27, 2018

View reviewed changes

tmat and others added 9 commits February 27, 2018 08:26

Add extensibility points for source link

9a6e9bf

Add doc comments

0a4bf57

Fix paths

4e4fb35

PR feedback

e81dc99

Update reference assemblies

b84e1c9

Log test output

74f088c

And assert success/failure explicitly.

Correct ContainingRoot

6f4d3a8

Fix path when matching ContainingRoot to ItemSpec

8d4ef07

Fix DeterministicSourcePaths condition

0e36540

tmat force-pushed the SourceLinkCore branch from 2e0fb13 to 0e36540 Compare February 27, 2018 16:29

rainersigwald requested changes Feb 27, 2018

View reviewed changes

This was referenced Mar 7, 2018

Implements targets that calculate PathMap from SourceRoots dotnet/roslyn#25325

Merged

Add source control information extensibility point #3063

Merged

tmat closed this Mar 7, 2018

Add extensibility points for source link #2960

Add extensibility points for source link #2960

Uh oh!

Conversation

tmat commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmat commented Feb 6, 2018

Uh oh!

tmat commented Feb 6, 2018

Uh oh!

tmat commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmat commented Feb 6, 2018

Uh oh!

AndyGerlicher commented Feb 6, 2018

Uh oh!

tmat commented Feb 6, 2018

Uh oh!

tmat commented Feb 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nguerrera Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat commented Feb 14, 2018

Uh oh!

tmat commented Feb 21, 2018

Uh oh!

tmat commented Feb 21, 2018

Uh oh!

rainersigwald left a comment

Choose a reason for hiding this comment

Uh oh!

rainersigwald commented Feb 26, 2018

Uh oh!

tmat commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmat commented Feb 27, 2018

Uh oh!

tmat commented Feb 6, 2018 •

edited

Loading

tmat commented Feb 6, 2018 •

edited

Loading

nguerrera Feb 7, 2018 •

edited

Loading

tmat Feb 7, 2018 •

edited

Loading

tmat commented Feb 27, 2018 •

edited

Loading

tmat Feb 27, 2018 •

edited

Loading

tmat commented Feb 27, 2018 •

edited

Loading

tmat commented Feb 27, 2018 •

edited

Loading