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.

Sign up for GitHub

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
Draft

design spec for version property in project reference #12348

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7299fd9
design spec for version property in project reference
martinrrm Jan 6, 2023
4a4c924
pr comments feedback, multiple options
martinrrm Apr 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 107 additions & 0 deletions proposed/2023/allow-version-property-in-projectreference.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
# Allow users to define version ranges for ProjectReferences

- [Martin Ruiz](https://github.com/martinrrm)
- Start Date (2023-01-01)
- [5556](https://github.com/NuGet/Home/issues/5556)

# 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.

Copy link
Contributor

@kartheekp-ms kartheekp-ms Apr 25, 2023 •
edited
Loading

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.


# 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?


# Explanation

Currently when adding a `ProjectReference` to a project, there is no property to specify which version(s) of it to be used when doing a pack.
When doing a pack to a package with a `ProjectRefernce` it will always be added as a range, where the minumum version will be the ProjectReference version and with an open maximum version.
martinrrm marked this conversation as resolved.
Show resolved Hide resolved

```
<ItemGroup>
<ProjectReference Include="../MyReferencedPackage/MyReferencedPackage.csproj" />
</ItemGroup>
```

Add a `Version` property to `ProjectReference` and store that value in the assets file when doing a restore so we can retrieve that information when doing a `pack command.

If there is no `Version` information then the behavior should be the current one.

## Example

### .CSPROJ file
```
<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.

</ItemGroup>
```

### assets file
```
"frameworks": {
"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": "[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.

}
}
}
},
```

### nuspec file
```
<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2012/06/nuspec.xsd">
<metadata>
<id>ConsoleApp1</id>
<version>1.2.4</version>
<authors>ConsoleApp1</authors>
<description>Package Description</description>
<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?

</group>
</dependencies>
</metadata>
<files>
<file src="C:\Users\mruizmares\source\repos\ConsoleApp1\ConsoleApp1\bin\Debug\net6.0\ConsoleApp1.runtimeconfig.json" target="lib\net6.0\ConsoleApp1.runtimeconfig.json" />
<file src="C:\Users\mruizmares\source\repos\ConsoleApp1\ConsoleApp1\bin\Debug\net6.0\ConsoleApp1.dll" target="lib\net6.0\ConsoleApp1.dll" />
</files>
</package>
```

### nupkg
```
Metadata:
id: ConsoleApp1
version: 1.2.4
authors: ConsoleApp1
description: Package Description
Dependencies:
net6.0:
MyReferencedPackage: '>= 1.0.0 && < 2.0.0'
Newtonsoft.Json: '>= 9.0.0 && < 13.0.2'

Contents:
- File: _rels/.rels
- File: [Content_Types].xml
- File: ConsoleApp1.nuspec
- File: lib/net6.0/ConsoleApp1.dll
- File: lib/net6.0/ConsoleApp1.runtimeconfig.json
- File: package/services/metadata/core-properties/a638a18cb3b1449185ce67e16a13ebaf.psmdcp
```

# Drawbacks

I don't think there are drawbacks to this implementation when doing a `pack` command. For restore I'm not sure if adding a new propert to the assets file can affect the performance.

# Rationale and alternatives

# Unresolved Questions

# Future Possibilities