Add source control information extensibility point #3063

tmat · 2018-03-07T23:18:45Z

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 (Include SourceRevisionId in InformationalVersion sdk#2028)
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 (Initialize RepositoryUrl and RepositoryCommit properties from source control info NuGet/NuGet.Client#2081)
Create source control and source link packages using these extensibility points
Create Microsoft.Build.StandardCI package

Supersedes #2960

tmat · 2018-03-07T23:21:38Z

@rainersigwald @AndyGerlicher @eerhardt @jeffkl @nguerrera PTAL
Implements the proposal based on our last meeting discussion.

eerhardt · 2018-03-08T00:10:22Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+    Any target that reads SourceRevisionId, PrivateRepositoryUrl, and other source control related
+    properties and items should depend on this target.
+
+    Source control information provider that sets these properties and items shall override this target


Should they override this target? or BeforeTargets it?

The issue is there can be only one override, so if 2 things want to set these properties/items...

There can only be one source control provider for a project. Having multiple doesn't make much sense.

I'll change it to BeforeTargets. There might potentially be scenario when one target fills in some information and another target (using the same source control) fills in more info.

eerhardt · 2018-03-08T00:14:29Z

src/Tasks/Microsoft.Common.props

+    <!--
+      PrivateRepositoryUrl is the URL of the repository supplied by the CI server or retrieved from source control manager.
+
+      This value is not directly embedded in build outputs to avoid inadvertently publishing links to private repositories.


I don't think I follow what this property is actually used for. Here we say what it is NOT used for, but never what it IS used for.

The next line explains. I'm open to suggestions to word it better.

The next line says ‘PublishRepositoryUrl’ property. Is that a type-o or really a different property? I thought it was a different prop

It's different property. Let me reword this to be clearer.

eerhardt

Just some nits on the comments.

tmat · 2018-03-08T17:05:37Z

@richlander FYI

tmat · 2018-03-08T17:19:24Z

@eerhardt Updated the comments. Let me know if it's clearer now.

eerhardt · 2018-03-08T17:33:18Z

src/Tasks/Microsoft.Common.props

+      PrivateRepositoryUrl is the URL of the repository supplied by the CI server or retrieved from source control manager.
+
+      Targets consuming this property shall not publish its value implicitly as it might inadvertently reveal an internal URL.
+      Instead, they shall only do so if the project sets PublishRepositoryUrl property to true. For example, the NuGet Pack target


they shall only do so if the project sets PublishRepositoryUrl property to true

Ok - this helped me understand what is going on. Thanks!

tmat · 2018-03-08T22:34:04Z

@rainersigwald I'd like to get this into VS 15.7 and the next dotnet SDK (whatever the version number is). Should I retarget the PR to vs15.7 branch?

AndyGerlicher

Looks good to me. If @rainersigwald is ok with it we can merge. master is correct for 15.7.

rainersigwald · 2018-03-13T18:54:21Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+    this target (by including InitializeSourceControlInformation in its BeforeTargets) and set
+    source control properties and items that haven't been initialized yet.
+  -->
+  <Target Name="InitializeSourceControlInformation" />


Do we need to insert the DependsOn property we talked about somewhere so folks can inject into this but still have a package compatible with VS2015?

Yes, i'll add the property.

I also have one more piece of feedback coming from NuGet team to look into before we merge.

tmat · 2018-03-13T22:52:10Z

@AndyGerlicher @rainersigwald Based on feedback from NuGet team I have conditioned reading the STANDARD_CI variables based on a new UseStandardCIVariables property. We want to avoid potential (build or test) breaks that initializing these properties from the Standard CI variables might cause.

For example, if Deterministic is on the compiler emits deterministic paths when running on CI build (ContinuousIntegrationBuild is true). This might affect unit tests running on the CI that read paths via CallerFilePathAttribute.

UseStandardCIVariables property will be switched on by the SourceLink package, so that customers who use that package and build deterministically (the default in .NET SDK projects) get all the deterministic goodness as well. Customers not using SourceLink can switch it on themselves.

tmat · 2018-03-13T22:52:18Z

@nguerrera FYI

tmat · 2018-03-14T16:01:40Z

On second thought, since we need to make the usage of Standard CI opt-in and thus they are not defaults anymore it seems it would be better to look at Standard CI as another source information provider and move the properties to a separate package (Microsoft.Build.StandardCI). Dropping in this package opts into reading source control info from Standard CI env variables. SourceLink package would then depend on this package.

Updated PR to only include the target that serves as a sync point for consumers and producers of the source control info.

tmat · 2018-03-14T16:30:07Z

@rainersigwald Could you please merge?

rainersigwald · 2018-03-14T18:20:58Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+    Source control information provider that sets these properties and items shall execute before this target (by including 
+    InitializeSourceControlInformation in its BeforeTargets) and set source control properties and items that haven't been initialized yet.
+  -->
+  <Target Name="InitializeSourceControlInformation" />


Did you decide that we didn't need a DependsOn property here? I thought we wanted that for some packages that wanted to support downlevel.

I have added SourceControlInformationFeatureSupported so that they can condition on that. Let me double check that this property is actually reasonable to use.

It looks good:
https://github.com/dotnet/sdk/pull/2028/files#diff-6746d5c3e16c263867ca48a7b3a962bfR52

tmat · 2018-03-14T20:11:41Z

Thanks everyone!

tmat mentioned this pull request Mar 7, 2018

Add extensibility points for source link #2960

Closed

6 tasks

eerhardt reviewed Mar 8, 2018

View reviewed changes

eerhardt approved these changes Mar 8, 2018

View reviewed changes

This was referenced Mar 8, 2018

Include SourceRevisionId in InformationalVersion dotnet/sdk#2028

Merged

Add spec for Standardized CI Environment Variables dotnet/designs#19

Closed

eerhardt reviewed Mar 8, 2018

View reviewed changes

eerhardt approved these changes Mar 8, 2018

View reviewed changes

tmat mentioned this pull request Mar 9, 2018

Initialize RepositoryUrl and RepositoryCommit properties from source control info NuGet/NuGet.Client#2081

Merged

AndyGerlicher approved these changes Mar 13, 2018

View reviewed changes

rainersigwald reviewed Mar 13, 2018

View reviewed changes

tmat force-pushed the SourceInformationExtensibility branch 2 times, most recently from 9bfd5ef to c34c7fe Compare March 13, 2018 22:47

Add source control information extensibility point

5a1c178

tmat force-pushed the SourceInformationExtensibility branch from c34c7fe to 5a1c178 Compare March 14, 2018 15:58

rainersigwald reviewed Mar 14, 2018

View reviewed changes

rainersigwald merged commit b4805af into dotnet:master Mar 14, 2018

natemcmaster mentioned this pull request Mar 22, 2018

Use built-in SDK features for commit hash attributes and source-link aspnet/BuildTools#618

Closed

kzu mentioned this pull request Sep 24, 2020

Integrate better with SourceLink and built-in support in the SDK devlooped/GitInfo#128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add source control information extensibility point #3063

Add source control information extensibility point #3063

tmat commented Mar 7, 2018 •

edited

Loading

tmat commented Mar 7, 2018

eerhardt Mar 8, 2018

eerhardt Mar 8, 2018

tmat Mar 8, 2018

tmat Mar 8, 2018 •

edited

Loading

eerhardt Mar 8, 2018

tmat Mar 8, 2018

eerhardt Mar 8, 2018

tmat Mar 8, 2018 •

edited

Loading

eerhardt left a comment

tmat commented Mar 8, 2018

tmat commented Mar 8, 2018

eerhardt Mar 8, 2018

tmat commented Mar 8, 2018

AndyGerlicher left a comment

rainersigwald Mar 13, 2018

tmat Mar 13, 2018

tmat commented Mar 13, 2018

tmat commented Mar 13, 2018

tmat commented Mar 14, 2018 •

edited

Loading

tmat commented Mar 14, 2018

rainersigwald Mar 14, 2018

tmat Mar 14, 2018

tmat Mar 14, 2018

tmat commented Mar 14, 2018

Add source control information extensibility point #3063

Add source control information extensibility point #3063

Conversation

tmat commented Mar 7, 2018 • edited Loading

tmat commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

tmat commented Mar 8, 2018

tmat commented Mar 8, 2018

Choose a reason for hiding this comment

tmat commented Mar 8, 2018

AndyGerlicher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat commented Mar 13, 2018

tmat commented Mar 13, 2018

tmat commented Mar 14, 2018 • edited Loading

tmat commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat commented Mar 14, 2018

tmat commented Mar 7, 2018 •

edited

Loading

tmat Mar 8, 2018 •

edited

Loading

tmat Mar 8, 2018 •

edited

Loading

tmat commented Mar 14, 2018 •

edited

Loading