Initialize RepositoryUrl and RepositoryCommit properties from source control info

[tmat]

Initialize RepositoryUrl and RepositoryCommit properties from values set by source control provider package referenced by the project.
See https://github.com/tmat/repository-info/tree/master/docs for details.

The added initialization target depends on InitializeSourceControlInformation target, which is now defined in Microsoft.Common.targets like so:

  <!--
    Target that allows targets consuming source control confirmation to establish a dependency
    on the targets producing this information.
    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 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" />

[tmat]

@emgarten @rohit21agrawal PTAL

[tmat]

Is dev the right branch for VS 15.7 and NET Core 2.1 releases? If not what branch should I move this change to in order to target these?

[rohit21agrawal]

where is the InitializeSourceControlInformation target defined?

[tmat]

@jaredpar @jinujoseph FYI

[tmat]

@rohit21agrawal In (future) Microsoft.Common.targets.

[tmat]

@nguerrera PTAL

[rohit21agrawal]

@tmat in that case, I don't see a reason why this change needs to be made in the Pack targets. The future Microsoft.Common.Targets file can directly define RepositoryUrl and RepositoryCommit properties and pack targets will consume it when set.

[tmat]

@rohit21agrawal The reason is that NuGet is the primary consumer of these properties. The desire of msbuild team is to avoid adding stuff to Microsoft.Common.targets that can be elsewhere.

[rohit21agrawal]

@tmat I disagree. We have worked in the past with the SDK team to define a common set of properties whose defaults will be set in sdk props/targets but be overridden in the csproj through a user. What you are proposing to do will not allow any user to override this information by simply setting a property since you are setting these properties in a target.

I will not accept these changes in pack targets unless there is a better reason.

[nguerrera]

Does pack only work with sdk projects or all projects?

[nguerrera]

If only SDK, we could put it there as it's not unlike other things we setup for pack. If it's for all projects than it needs to go here or common targets and seems both teams are pushing back.

[nguerrera]

Actually one difference with the other SDK defaults for Pack is that this one needs to happen in a target and needs to schedule itself before generate nuspec. The rest is just defaulting properties in evaluation.

[rohit21agrawal]

right now pack targets are designed to work only for sdk projects, however, legacy package reference based projects can also use these targets by referencing the pack nupkg to pack projects.

Additionally, given that these properties also need to be available when a user packs their project using nuget.exe, putting them in pack targets is the wrong approach since nuget.exe does not use pack targets, instead queries msbuild directly for properties.

[tmat]

will not allow any user to override this information by simply setting a property since you are setting these properties in a target.

I don't understand why wouldn't the project be able to override these. The target sets them only if they weren't set. So if the project sets them they will not get changed.

[tmat]

Note also that GenerateNuspec needs to depend on InitializeSourceControlInformation. If it didn't the source control information might not be published when GenerateNuspec runs.

NuGet.exe ... instead queries msbuild directly for properties.

If it does not execute any targets then this feature could not possibly work at all, since the only way to automatically retrieve the information from source control system is to execute a task that does so.
I'm fine with that limitation. Customers who use NuGet.exe will not benefit from this feature but they can continue setting the properties manually. This change will not break their scenario.

The new target is only activated when the project has a package reference to a source control package (a prototype of such package for Git is here: https://github.com/tmat/repository-info/tree/master/src/Microsoft.Build.Tasks.Git). Projects that do not add a reference to this package won't observe any behavior change.

[rohit21agrawal]

that makes the choice even easier, the package should include a target file that includes all the changes you are making here, which in turn will be consumed by the pack target.

[tmat]

@rohit21agrawal That would be backwards. The package can't hard-code all possible consumers of the information it publishes. It has no dependency whatsoever on NuGet. The purpose of the package is to provide source control information for anyone who needs to read it. There are multiple consumers in multiple layers that need this information, including C#, VB, F#, C++ compilers, dotnet SDK and NuGet. Third party targets may also want to read this information. As you can see it does not make sense to hard-code all these into that package.

[rohit21agrawal]

There are many such packages out there that cater to multiple consumers, and yet are able to provide extensibility to each of them. I don't see an issue including an extra targets file in the Microsoft.Build.Tasks.Git nupkg. You can write a target that will execute only if the property IsPackable = true, which would indicate that this project has imported Pack targets already, and thus needs to set this source control information.

I am not convinced that it is a good idea to include any dependency in pack targets which does not come out of the box on installing the sdk.

[tmat]

I am not convinced that it is a good idea to include any dependency in pack targets which does not come out of the box on installing the sdk.

There is no such dependency being added.

[rohit21agrawal]

There is a dependency on a target that doesn’t ship in the SDK

[tmat]

The target is in Microsoft.Common.targets which is integral part of msbuild.

[tmat]

FYI, _GetFrameworkAssemblyReferences target in https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Build.Tasks.Pack/NuGet.Build.Tasks.Pack.targets#L359 has a dependency on ResolveReferences which is also defined in common targets (Microsoft.Common.CurrentVersion.targets). There is no new dependency added.

[rohit21agrawal]

my point is that there is no such target/property in Pack targets that relies on the presence of a particular package which is not a default dependency coming out of SDK. Given how extensible target files are, it just makes logical sense to put this logic in the same package which is supposed to activate this behavior.

If you would like to discuss more, I am happy to meet in person to discuss this.

[jainaashish]

I agree with @rohit21agrawal these properties should be defined by package itself in it's own target file. NuGet is just a consumer of these properties and doesn't care if they are being set either by a incoming package or the project itself.

[tmat]

@rohit21agrawal Can you point me to any existing tests that I can mimic to test this feature?

[rohit21agrawal]

PackCommandTests in Dotnet.Integration.Test project if you want to do an end to end dotnet.exe based test.