add version-suffix to P2P references for dotnet pack

[rohit21agrawal]

Bug

Link: NuGet/Home#4337
Regression: No
If Regression then when did it last work: N/A
If Regression then how are we preventing it in future: N/A

Fix

Details: This fix adds the passed in version suffix to the project references if they don't already have it. These project references get added to the resulting nupkg as package references with the given version suffix.

Testing/Validation

Tests Added: Yes
Reason for not adding tests: Tests added
Validation done: Added automated functional tests, and verified locally.

[dasMulli]

This only adds the version suffix and does not really evaluate PackageVersion (/ PackageId) from the referenced projects.. this would not really help when using custom msbuild logic (BuildNumber, GitCommitCount, …) to construct the version

[rohit21agrawal]

@dasMulli i don't see how that would be any useful to do it again as PackageVersion and PackageId are already being read by restore into the assets file.

[clairernovotny]

@rohit21agrawal the problem is that it needs a double-restore then. It should not read from the assets file to pack, that's the problem.

[dasMulli]

The idea is if you had a logic like:

<VersionPrefix>1.2.3</VersionPrefix>
<ReleaseLabel>beta2</ReleaseLabel>
<Version>$(VersionPrefix)-$(ReleaseLabel)-$(BuildNumber.PadLeft(5,'0'))</Version>

(where ReleaseLabel and VersionPrefix could be in a Directory.Build.props file)

Then dotnet pack /p:BuildNumber=25 should correctly set the version on the package references to 1.2.3-beta2-00025.

[rohit21agrawal]

@dasMulli / @onovotny the problem you describe can also be solved using --version-suffix ( which is the same as msbuild property $(VersionSuffix):
Set this in the project being packed:

<VersionPrefix>1.2.3</VersionPrefix>
<ReleaseLabel>beta2</ReleaseLabel>
<VersionSuffix>$(ReleaseLabel)-$(BuildNumber.PadLeft(5,'0'))</VersionSuffix>

[clairernovotny]

@rohit21agrawal not if the version itself is calculated. It's not just the suffix, but the whole version that may change. I don't have any hard-coded versions anywhere. They are all generated on-the-fly as part of the MSBuild targets.

[dasMulli]

I believe it is dangerous to assume that all project references share the same VersionSuffix generation logic. Yes I can make sure that all the targets set VersionSuffix explicitly but if that varies by project (example: some packages are stable and some are preview2-full), this will just be incorrect and hard to diagnose.

[rohit21agrawal]

@onovotny what you describe is different issue, and a much bigger task at hand. this PR fixes the problem as described by the OP. The ask was to bring back the behavior which preview2 tooling had with project.json, and this is what this PR is aiming to fix.
we will have to file a different issue to calculate whole version for P2P using msbuild and triage it accordingly

[clairernovotny]

This is related to and should be the same fix as NuGet/Home#4790
4790 is scheduled for 4.4 and fixing that fixes this implicitly.

[emgarten]

Support for a VersionSuffix to fix up versions at pack time is unrelated to allowing packages contribute to restore inputs I would say.

the problem is that it needs a double-restore then. It should not read from the assets file to pack, that's the problem.

Reading the project to project graph and finding all of the information needed for pack again in a different for pack is non-trival. The current approach is that restore captures the dependency graph, and pack packs up everything to match that which keeps everything consistent.

To solve the double restore problem restore really needs a way to load new props/targets during the restore. Currently this doesn't work due to limitations with msbuild.

[clairernovotny]

@emgarten I'm not suggesting they need to contribute restore inputs. Rather, that pack should not be reading the assets file at all since it's stale. Sometimes the version is calculated per commit. Could be time of day. Point is, the only way to get the version is to evaluate the versions during the pack step.

[clairernovotny]

/cc @AArnott

[dasMulli]

I think the "fix" should be one of:

1. Re-Evaluate PackageId / PackageVersion from all project references (e.g. introducing a new GetPackageVersion target).
2. Check if the current version suffix is different than what was used during restore and generate a warning / error on a mismatch to indicate this to the user.

I believe 2 is actually okay since dotnet pack now calls restore implicitly (and I hope changing an MSBuild property that impacts the version does work on incremental restore) and MSBuild has a new /restore option in the making to do the same.

[dasMulli]

Still project A:

<VersionPrefix>1.2.3</VersionPrefix>
<VersionSuffix>beta2</VersionSuffix>

with a pkg ref to project B:

<Version>2.0.0</Version>

would then incorrectly emit a dependency on project B using 2.0.0-beta2 as Version. This should not happen.

[emgarten]

Re-Evaluate PackageId / PackageVersion from all project references (e.g. introducing a new GetPackageVersion target).

Reading only PackageVersion again based on the project references already found in the assets file could work. PackageId would be a problem if for example the id was changed to conflict with something else in the graph.

[clairernovotny]

@emgarten I'm completely ok with not reevaluating the PackageId. That would be messy and I'd expect a build that does that to be more complicated.

[clairernovotny]

@emgarten if evaluating on pack works, then I don't think $(ExcludeRestorePackageImports)='true' from #4790 is as important for this scenario since the version wouldn't need to be calculated during the restore phase.

[dasMulli]

I also think PackageId is likely a very edge-case use (still, a warning / error on change might be nice).

Context: I ran into it while taking some time do diagnose dotnet/msbuild#2463 - the idea was that a project would generate both signed and unsigned versions based on an MSBuild property with the package id adding .Signed for the versions containing signed assemblies.
There are quite a few .Signed / .StrongName packages out there who may want to do something similar if they changed to the new csproj system.

[emgarten]

if evaluating on pack works, then I don't think $(ExcludeRestorePackageImports)='true' is as important for this scenario since the version wouldn't need to be calculated during the restore phase.

Evaluating on pack is the only way I see that issue getting fixed 😄

ExcludeRestorePackageImports protects against needing potentially endless restores, and with no one way to know you are done until the last two results are the same, along with a host of other problems such as a package adding a reference to a project inside the package to override something else.

[emgarten]

@dasMulli that is an interesting scenario for PackageId, I could see using that.

[emgarten]

It seems reasonable to directly re-evaluate these properties, which would allow git versioning to work. My main concern was re-walking projects in msbuild which is difficult since some projects are empty or do not contain the expected targets. These projects paths are in the asset file and could be used to reduce differences between restore and pack.

[AArnott]

Point is, the only way to get the version is to evaluate the versions during the pack step.

Agreed. If you're packing a dependency tree of projects, each project will pack its own version based on the current MSBuild properties -- not the assets file. So for a referencing pack to use the assets file means you're using the wrong data. If you want the package to reference another project's package, ask the project what the version of the package will be rather than assuming some stale cache has it right. :)

I understand that this PR was focused on a smaller problem. But as @dasMulli said:

I believe it is dangerous to assume that all project references share the same VersionSuffix generation logic.

I believe fixing this subset may be hazardous. Also, since fixing the full problem (NuGet/Home#4790) will also fix this one, and in what I believe is a safe way, it seems prudent to focus on fixing that issue.

[emgarten]

Closing based on the earlier discussion on this.