Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

design spec for version property in project reference #12348

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

martinrrm
Copy link
Contributor

@martinrrm martinrrm commented Jan 6, 2023

spec for: #5556

Rendered Proposal

Please 👍 or 👎 this comment to help us with the direction of this feature & leave as much feedback/questions/concerns as you'd like on this issue itself and we will get back to you shortly.

Thank You 🎉

@martinrrm martinrrm requested a review from a team as a code owner January 6, 2023 23:30

# Motivation

When using `ProjectReference` there is no option to define a Version to the reference like in `PackageReference` and the version will always be defined as `>= ProjectVersion`, this results in customers manually modifying the nuspec file if they want to declare a different version than the project version.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you be able to add little bit more details on exactly what is the project version here?
What happens if I change different branch which have same version but with different content?
For packages we have dedicated global packages and for same id+version content doesn't mutate. Here it mutates, so what happens now for same project version with different content?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For project version I mean the value of <Version></Version> in the CSPROJ.

About what happens now for same project version with different content? I'll do more testing but I;m not sure if I understand your concern.

For example if ProjectA has a ProjectReference to MyReferencedProject like this: <ProjectReference Include="MyReferencedProject" Version="[9.0.0, 13.0.2)" /> and you change the content in MyReferencedProject but not the version, you mean the package content should not change?

"projectReferences": {
"C:\\Users\\mruizmares\\source\\repos\\ConsoleApp1\\MyReferencedPackage\\MyReferencedPackage.csproj": {
"projectPath": "C:\\Users\\mruizmares\\source\\repos\\ConsoleApp1\\MyReferencedPackage\\MyReferencedPackage.csproj",
"version": "[1.0.0, 2.0.0)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this change be compatible with old version tools?
That is, will old tool be able to build the solution/project with the new format assets file?
You may refer to NuGet/NuGet.Client#4654 (comment) for similar checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested doing a dotnet build and it works, you know if there are more ways to test if it is compatible?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you use --no-restore? Otherwise dotnet build will auto-restore and overwrite your manually edited assets file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that because it's additive it should work.

<dependencies>
<group targetFramework="net6.0">
<dependency id="MyReferencedPackage" version="[1.0.0, 2.0.0)" exclude="Build,Analyzers" />
<dependency id="Newtonsoft.Json" version="[9.0.0, 13.0.2)" exclude="Build,Analyzers" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I know if there is any behavior change for package reference? Or will this change only impact project reference? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, we consider ProjectReferences as packages when doing pack, so this code is shared. Since PackageReference's can handle ranges, I think the behavior is going to be the same.

Currently we use dependency.VersionRange.ToLegacyShortString() to represent the version string value and I want to investigate if we can use dependency.VersionRange.ToShortString() or if it's a breaking change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to switch from legacy short string to short string?

```
<ItemGroup>
<PackageReference Include="Newtonsoft.Json" Version="[9.0.0, 13.0.2)" />
<ProjectReference Include="../MyReferencedPackage/MyReferencedPackage.csproj" Version="[1.0.0, 2.0.0)" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the minimum version should not be allowed to be specified on the ProjectReference, and NuGet should always automatically fill in the min version from the project's version.

This might not be popular with customers who want this feature, but this is for the protection of package consumers, especially since NuGet only selected minimum version in its dependency resolution graph.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this I think we can take the customer suggestion and do something like this:

<ProjectReference Include="..\MyReferencedPackage\MyReferencedPackage.csproj" >
    <MaximumVersion Inclusive="false">2</MaximumVersion>
</ProjectReference>

With this in mind, you think saving this as a version range in the assets file (like the initial proposal) or change it to be different properties?

Option 1 (I like this one more) :

"C:\\Users\\mruizmares\\source\\repos\\ConsoleApp1\\MyReferencedPackage\\MyReferencedPackage.csproj": {
  "projectPath": "C:\\Users\\mruizmares\\source\\repos\\ConsoleApp1\\MyReferencedPackage\\MyReferencedPackage.csproj",
  "version": "[1.0.0, 2.0.0)"
}

Option 2:

"C:\\Users\\mruizmares\\source\\repos\\ConsoleApp1\\MyReferencedPackage\\MyReferencedPackage.csproj": {
  "projectPath": "C:\\Users\\mruizmares\\source\\repos\\ConsoleApp1\\MyReferencedPackage\\MyReferencedPackage.csproj",
  "maxVersion": "2.0.0",
  "inclusive": true,
}

I currently have an mvp for this in branch dev-martinrrm-project-reference I have also tested with dotnet build --no-restore and it works, I'm going to investigate why is not breaking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently I forgot to reply to this 😕

With this I think we can take the customer suggestion and do something like this:
<MaximumVersion Inclusive="false">2</MaximumVersion>

That's not valid MSBuild syntax. While project files are XML, MSBuild has a strict syntax, and diverging from it will require changes to MSBuild if we feel stongly enough that it's needed to support a customer scenario (in which case you'd also need to consider what potential bugs it could introduce to non-NuGet scenarios if the syntax was extended). Anyway, MSBuild items are basically Dictionary<string, string>, where using XML attributes and child elements are equivalent. So any syntax proposal needs to be key-value pairs.

NuGet's VersionRange already supports values without a minimum value, for example (, 2.0.0-0), so we could just have <ProjectReference Include="..\path\to\project.csproj" Version="(,2.0.0)" />, and then at pack time, NuGet figures out the actual version of project.csproj, let's say 1.2.3, and then makes it a [1.2.3, 2.0.0) dependency.

However, I think another scenario that some customers may want, is to limit the package dependency to the exact version of the project when it's being packed. VersionRange defines this as [1.2.3] (which is equivalent to [1.2.3, 1.2.3]). However, [] is not valid, neither is [,]. I don't currently have any ideas for syntax we can use, unless we special case [] in the relevant code before it gets passed to VersionRange.Parse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zivkan I like the idea of doing <ProjectReference Include="..\path\to\project.csproj" Version="(,2.0.0)" /> and change the version range to always have the lower version to the project version, just like your example. But at the same time I'm concern that this will only work if they are using ranges, otherwise it will look weird, for example:

<ProjectReference Include="..\path\to\project.csproj" Version="2.0.0" /> make it a [1.2.3, 2.0.0) or [1.2.3, 2.0.0].

What if we do something like <ProjectReference Include="..\path\to\project.csproj" MaxVersion="2.0.0" Inclusive="true" />? Is that a valid MSBuild syntax? Whit this we can also make the scenario for [1.2.3,1.2.3] work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per our docs on version ranges, Version="2.0.0" means Version="[2.0.0, )" (>= 2.0). My proposal here is that any version range with a min version errors our, so Version="2.0.0" would result in an error. (maybe I forgot to write this explicitly in my previous comment?)

What if we do something like <ProjectReference Include="..\path\to\project.csproj" MaxVersion="2.0.0" Inclusive="true" />? Is that a valid MSBuild syntax?

Yes, that works with MSBuild's syntax. Something to keep in mind, although it's very unlikely, is that customers might have build custom build scripts, so if any customer already uses Inclusive= or MaxVersion= for something in their build script, introducing this might break them. Inclusive in particular sounds version generic, so if you're not already in the mental context of thinking about NuGet dependency versions, I don't think that Inclusive would be obvious to customers what it's related to. Imagine being a new .NET dev, looking at a csproj for a project you just joined, and trying to guess what <ProjectReference Include="..\projb\projb.csproj" MaxVersion="2.0.0" Inclusive="true" /> means. The MaxVersion might help with the guess about Inclusive. Also, what happens if Inclusive has a value, but MaxVersion does not? Do we silently ignore, print a warning or an error? This is also an additional edge case that needs tests (and ideally should be covered by the spec). I think using Version= with a range, and erroring out when a min version is specified, has few test cases, although we shouldn't be designing for that. What's easiest for customers to understand and work with?

Customers are more likely to already be familiar with Version= for PackageReference, although I imagine not many customers are familiar with NuGet's range syntax. Maven uses the same syntax, but I'm not aware of other package managers using this syntax. I think this syntax is used in mathematics, but not many people have studied maths to the extend to have learned this syntax, and even if they have, who will expect these math conventions to be used in a project file?

All this to say, I don't have an obvious answer/solution. Get more feedback from others. Perhaps my Version="range" suggestion is not popular. I'm mostly thinking out loud, hoping that something will jump out as an "obvious" best choice, but it hasn't.

Whit this we can also make the scenario for [1.2.3,1.2.3] work.

How? What would the syntax be for "use the referenced project's version as the max" be? MaxVersion="???". It can't be $(Version), because that's the version of the current project, not the referenced project. Although, I expect the most common scenario for wanting to use exact version match is when there are multiple projects packed into packages that all use the same version, in which case $(Version) will work from a pragmatic point of view.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic word maybe?

[X,1.2.3] where X is the autofilled version?
Likewise, [X].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today, the Version for project references get recalculated when pack is run, so maybe that's another argument for some sort of a magic word/letter.

I don't love it though. It feels like the most completely way without misuse risk, but it's kind of ugly.

"projectReferences": {
"C:\\Users\\mruizmares\\source\\repos\\ConsoleApp1\\MyReferencedPackage\\MyReferencedPackage.csproj": {
"projectPath": "C:\\Users\\mruizmares\\source\\repos\\ConsoleApp1\\MyReferencedPackage\\MyReferencedPackage.csproj",
"version": "[1.0.0, 2.0.0)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you use --no-restore? Otherwise dotnet build will auto-restore and overwrite your manually edited assets file.

@martinrrm martinrrm requested a review from zivkan January 11, 2023 16:17
Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we're comfortable with a direction, then we should make sure we share the design in the issue and get customer feedback.

Good idea to solicit feedback from the msbuild/SDK group, potentially in one of the syncs.

"projectReferences": {
"C:\\Users\\mruizmares\\source\\repos\\ConsoleApp1\\MyReferencedPackage\\MyReferencedPackage.csproj": {
"projectPath": "C:\\Users\\mruizmares\\source\\repos\\ConsoleApp1\\MyReferencedPackage\\MyReferencedPackage.csproj",
"version": "[1.0.0, 2.0.0)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that because it's additive it should work.

```
<ItemGroup>
<PackageReference Include="Newtonsoft.Json" Version="[9.0.0, 13.0.2)" />
<ProjectReference Include="../MyReferencedPackage/MyReferencedPackage.csproj" Version="[1.0.0, 2.0.0)" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today, the Version for project references get recalculated when pack is run, so maybe that's another argument for some sort of a magic word/letter.

I don't love it though. It feels like the most completely way without misuse risk, but it's kind of ugly.

<dependencies>
<group targetFramework="net6.0">
<dependency id="MyReferencedPackage" version="[1.0.0, 2.0.0)" exclude="Build,Analyzers" />
<dependency id="Newtonsoft.Json" version="[9.0.0, 13.0.2)" exclude="Build,Analyzers" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to switch from legacy short string to short string?

@ghost ghost added the Status:No recent activity No recent activity. label Mar 3, 2023
@ghost
Copy link

ghost commented Mar 3, 2023

This PR has been automatically marked as stale because it has no activity for 30 days. It will be closed if no further activity occurs within another 15 days of this comment, unless it has a "Status:Do not auto close" label. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

@ghost ghost closed this Mar 19, 2023
@martinrrm martinrrm reopened this Apr 17, 2023
@martinrrm martinrrm marked this pull request as draft April 17, 2023 19:57
Minimum version should not be open to users, and NuGet should always automatically fill in the min version from the project's version, this is for the protection of package consumers.

---
## Option 1 - Version with VersionRange
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After doing more investigation (Nikolche also commented about this in #12348 (comment)), I think this option is not possible.

I don't think we are aware of the project version during restore, meaning that we will have different information in the assets file and the .nupkg

Example:

MyReferencedPackage -> v.1.0.9

<ProjectReference Include="../MyReferencedPackage/MyReferencedPackage.csproj" Version="[2.0.0, 4.0.0)" />

Will write this the assets file:

"net6.0": {
          "targetAlias": "net6.0",
          "projectReferences": {
            "C:\\Users\\mruizmares\\source\\repos\\ConsoleApp1\\MyReferencedPackage\\MyReferencedPackage.csproj": {
              "projectPath": "C:\\Users\\mruizmares\\source\\repos\\ConsoleApp1\\MyReferencedPackage\\MyReferencedPackage.csproj",
              "version": "[2.0.0, 4.0.0)"
            }
          }

And this can cause confusion when pack resolves to another min version.

@martinrrm martinrrm removed the Status:No recent activity No recent activity. label Apr 19, 2023

# Summary

Add a `Version` to `ProjectReference` tag in CSPROJ, to allow customers to specify the referenced project version in the `.nupkg` and `nuspec` files when doing a pack command.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, I think Version attribute doesn't belong to ProjectReference because it refers to the package version range. Looking at the schema for ProjectReference element https://github.com/dotnet/msbuild/blob/main/src/MSBuild/MSBuild/Microsoft.Build.CommonTypes.xsd#L643-L722, I think PackageVersion might be more appropriate. The schema has Package as sub element for ProjectReference but there are no comments to understand more about its usage. I noticed that there is also SpecificVersion attribute for ProjectReference element to specify whether the exact version of the assembly should be used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI - There is an MSBuild property PackageVersion allowing customers to specify package version in the csproj file if the project is packed as nupkg.

https://learn.microsoft.com/nuget/create-packages/package-authoring-best-practices#package-metadata

Visual Studio property name Project file/ MSBuild property name Nuspec property name Description
Package version PackageVersion version NuGet package version.


# Summary

Add a `Version` to `ProjectReference` tag in CSPROJ, to allow customers to specify the referenced project version in the `.nupkg` and `nuspec` files when doing a pack command.
Copy link
Contributor

@kartheekp-ms kartheekp-ms Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding another PackageReference entry in the project file when customers would like to specify upper limit version number for a project reference.

As you can see in the below example, ClassLibrary2 is mentioned as both ProjectReference and PacakageReference.

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net7.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="ClassLibrary2" Version="[1.0.0, 2.0.0)" />
  </ItemGroup>

  <ItemGroup>
    <ProjectReference Include="..\ClassLibrary2\ClassLibrary2.csproj" />
  </ItemGroup>

</Project>

I didn't think about the pros/cons or possibilities of this approach.

@JonDouglas
Copy link
Contributor

Thanks for the review yesterday.

My general comments are that we should consider the duality between ProjectReference and PackageReference. Thus if one has a piece of metadata and a developer were to transform from one way to the other, that the metadata matches and functions as appropriate.

The only other thing I have to add is that we should comment on the issue in question to invite more feedback from the community and ensure this is the right direction to solve their challenges. I've added some additions to the parent comment to help invite others.

Finally, do feel free to take this out of draft when you are ready to get reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants