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

Use csproj information for NuGet pack #1189

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

lahma
Copy link
Contributor

@lahma lahma commented Apr 14, 2024

Types of Changes

Prerequisites

Please make sure you can check the following two boxes:

  • I have read the CONTRIBUTING document
  • My code follows the code style of this project

Contribution Type

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue, please reference the issue id)
  • New feature (non-breaking change which adds functionality, make sure to open an associated issue first)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Description

It seems I was able to drop NET6/7/8 targets with my earlier changes to .nuspec so here's something to make amends. This in my opinion simplifies the packaging logic quite a bit. All is in propertyes/csproj and dotnet pack produces the output as expect

Here's the produced nuspec in package file, you can see <owners> as empty but it's deprecated and now replaced with proper <authors>.

<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2012/06/nuspec.xsd">
  <metadata>
    <id>AngleSharp</id>
    <version>1.2.0</version>
    <authors>Florian Rappl</authors>
    <owners></owners>
    <requireLicenseAcceptance>false</requireLicenseAcceptance>
    <license type="expression">MIT</license>
    <icon>logo.png</icon>
    <readme>README.md</readme>
    <projectUrl>https://anglesharp.github.io/</projectUrl>
    <description>AngleSharp is the ultimate angle brackets parser library. It parses HTML5, CSS3, and XML to construct a DOM based on the official W3C specification.</description>
    <releaseNotes>https://github.com/AngleSharp/AngleSharp/blob/main/CHANGELOG.md</releaseNotes>
    <copyright>Copyright 2013-2024, AngleSharp.</copyright>
    <tags>html html5 css css3 xml dom dom4 parser engine hypertext markup language query selector attributes linq angle bracket web internet text headless browser</tags>
    <repository type="git" url="https://github.com/AngleSharp/AngleSharp" commit="bb66302f7ae6ef38d58596873ba85deb3799e726" />
    <dependencies>
      <group targetFramework=".NETFramework4.6.2">
        <dependency id="System.Text.Encoding.CodePages" version="8.0.0" exclude="Build,Analyzers" />
      </group>
      <group targetFramework=".NETFramework4.7.2">
        <dependency id="System.Text.Encoding.CodePages" version="8.0.0" exclude="Build,Analyzers" />
      </group>
      <group targetFramework="net6.0" />
      <group targetFramework="net7.0" />
      <group targetFramework="net8.0" />
      <group targetFramework=".NETStandard2.0">
        <dependency id="System.Text.Encoding.CodePages" version="8.0.0" exclude="Build,Analyzers" />
      </group>
    </dependencies>
  </metadata>
</package>

Files in package:

image

@FlorianRappl
Copy link
Contributor

Here's the produced nuspec in package file, you can see as empty but it's deprecated and now replaced with proper .

Yeah but that is what I feared. We distinguish here, because authors are meant to be the contributors, which is everyone (incl. you) who contributed - but owners are the core maintainers / persons responsible for deciding when / what to release.

I'm fine if this is the only change here; but then we should maybe change "Florian Rappl" to either "AngleSharp contributors" or "Florian Rappl et al." (or something else that seems to reflect this).

Rest seems fine to me.

Final question: How to make the dependency version to be a range (this would be needed for the other AngleSharp repositories, e.g., https://github.com/AngleSharp/AngleSharp.Js/blob/devel/src/AngleSharp.Js.nuspec#L20). If this cannot be reflected without a nuspec then this approach should not be taken.

@lahma
Copy link
Contributor Author

lahma commented Apr 14, 2024

Here's the produced nuspec in package file, you can see as empty but it's deprecated and now replaced with proper .

Yeah but that is what I feared. We distinguish here, because authors are meant to be the contributors, which is everyone (incl. you) who contributed - but owners are the core maintainers / persons responsible for deciding when / what to release.

Well if you look at the NuGet page https://www.nuget.org/packages/Quartz you can see that the owner is me and the Quartz organization, however the NuGet package metadata term is author https://nuget.info/packages/Quartz/3.8.1 . So NuGet experience considers author the owner.

I'm fine if this is the only change here; but then we should maybe change "Florian Rappl" to either "AngleSharp contributors" or "Florian Rappl et al." (or something else that seems to reflect this).

So changing this would also change (at least) the NuGet web UI to show contributors as owners. I get the idea if distinction, but not sure if it's worth the hassle. I haven't seen packages listing contributors on NuGet pages, see NUnit for example https://www.nuget.org/packages/NUnit . Only specific people are listed as authors/owners, but the copyright notice includes the contributors.

Rest seems fine to me.

👍🏻

Final question: How to make the dependency version to be a range (this would be needed for the other AngleSharp repositories, e.g., AngleSharp/AngleSharp.Js@devel/src/AngleSharp.Js.nuspec#L20). If this cannot be reflected without a nuspec then this approach should not be taken.

This is definitely something hard to achieve with just the PackageReference definition. I'm not entirely sure if the range is needed though. If I consume AngleSharp.Js it would give dotnet restore requirement that Jint 3.1 is needed and restore can decide the latest minor/patch but it won't bring Jint 4.0 unless I specifically add reference to Jint and to that version. So the restore process has its smarts and you won't get the new major version if you don't specifically ask for it. In my opinion it's usually gives more trouble to have a package prohibiting the usage of new major (Microsoft doesn't do this), if it won't work just needs a PR to make consuming package compatible with new version.

@campersau
Copy link
Contributor

@lahma
Copy link
Contributor Author

lahma commented Apr 14, 2024

The range is restore guidance for library's own compilation that won't reflect to produced nuget package/its internal nuspec definition.

@FlorianRappl
Copy link
Contributor

Well if you look at the NuGet page https://www.nuget.org/packages/Quartz you can see that the owner is me and the Quartz organization, however the NuGet package metadata term is author https://nuget.info/packages/Quartz/3.8.1 . So NuGet experience considers author the owner.

Yes I know this - I was not talking about how NuGet interprets it, but how we use these fields (and that for me its important that the contributors are mentioned in some way in the metadata; how that is interpreted / represented on the page then is secondary.

So changing this would also change (at least) the NuGet web UI to show contributors as owners.

But authors are not owners; NuGet made that clear IIRC. The owner (deprecated owners field) is associated with the token / publisher account(s) and all other accounts that have a right to publish new versions. Previously, we also used the authors field to indicate "AngleSharp" and my point was to keep doing that.

If I consume AngleSharp.Js it would give dotnet restore requirement that Jint 3.1 is needed and restore can decide the latest minor/patch but it won't bring Jint 4.0 unless I specifically add reference to Jint and to that version. So the restore process has its smarts and you won't get the new major version if you don't specifically ask for it.

I am not sure here; my experience with restore is that a range such as [1.0, 2.0) is not automatically used when a dependency on 1.1 was specified. So if you (as a user) would want to go for the lower end (1.0) you'd get in trouble here. I mean, that makes sense as NuGet does not know that the 1.1 can be downleveled to a 1.0; so either one would need to work with an outdated version locally when publishing (not great) or need to remain at an implicit [1.1, 2.0).

<PackageReference> supports version ranges https://learn.microsoft.com/en-us/nuget/concepts/package-versioning?tabs=semver20sort#references-in-project-files-packagereference , does this help?

It would, if it would be considered for package creation. AFAIK it is not.

@lahma
Copy link
Contributor Author

lahma commented Apr 14, 2024

I am not sure here; my experience with restore is that a range such as [1.0, 2.0) is not automatically used when a dependency on 1.1 was specified. So if you (as a user) would want to go for the lower end (1.0) you'd get in trouble here. I mean, that makes sense as NuGet does not know that the 1.1 can be downleveled to a 1.0; so either one would need to work with an outdated version locally when publishing (not great) or need to remain at an implicit [1.1, 2.0).

For me the default logic feels quite right. Say we have Jint with version 3.0.0, 3.0.1 and 3.1.0. Have library A which refers to 3.0.1 and library B which refers to version 3.1.0.

  • If I add NuGet package A to my project I will get Jint 3.0.1 which is the lowest that will work
  • I could update Jint to 3.0.2 with explicit own reference and thus mandating the version
  • Now that I add NuGet package B to my project package resolution will point to 3.1.0 (this is why we use package lock files too)
  • Package maintainer for B decides to upgrade reference to Jint 4.0.0 and releases new version
  • I upgrade my B package in my consuming project to new version and I start to get Jint 4.0.0 as it's the lowest accepted now
  • My package A might be broken on runtime due to changed APIs etc, this is expected and I need to make sure package A will be made compliant with Jint 4.0.0 or I just need to downgrade back older version of package B until issues are being resolved - I don't need explicit prohibitors for version ranges

The fixed version ranges with DotNetZip were a real PITA due to Paket's handling and it requiring these explicit ranges, always needed a new release when dependency versions didn't match anymore to other consumed libraries, even though there even were no runtime breaking changes.

@FlorianRappl
Copy link
Contributor

The described cases are not the ones that we are worried about. It's the case where you have an explicit dependency to, e.g., Jint 3.0.2 but you now add a dependency to lib B - making this a 3.1.0 which should be a 3.0.2 still.

@lahma
Copy link
Contributor Author

lahma commented Apr 14, 2024

IMO if you add the dependency to B it's an informed decision that you cannot no longer use older version as you want to use a library that needs a newer version. Basically you would want to prevent using B?

@FlorianRappl
Copy link
Contributor

IMO if you add the dependency to B it's an informed decision that you cannot no longer use older version as you want to use a library that needs a newer version. Basically you would want to prevent using B?

Well, first of all not everyone is aware of the dependencies of a lib, but even with that awareness - why should we limit the combinations if we know that the lower one is supported, too? It does not make much sense. We should deliver the library in a way that it can be consumed in the broadest / most flexible way. That's why it is a library. A package manager should help with this. The version range has been introduced to help with this - by giving the library means for telling the package manager what is supported.

@lahma
Copy link
Contributor Author

lahma commented Apr 14, 2024

IMO if you add the dependency to B it's an informed decision that you cannot no longer use older version as you want to use a library that needs a newer version. Basically you would want to prevent using B?

Well, first of all not everyone is aware of the dependencies of a lib, but even with that awareness - why should we limit the combinations if we know that the lower one is supported, too? It does not make much sense. We should deliver the library in a way that it can be consumed in the broadest / most flexible way. That's why it is a library. A package manager should help with this. The version range has been introduced to help with this - by giving the library means for telling the package manager what is supported.

I think we just have different viewpoints on this, and I respect your view. My look on the matter is more about me as a software developer making the decision if the functionality works with the combination of libraries I've chosen. This is one things I've worked recently on - package lock files and reporting used versions and their licenses for auditing purposes.

So I guess the difference is that while I'm leaning towards getting the lowest supported version and taking steps from that, you would like to opt out from supporting something that might not work. Major versions of course are the ones here to worry about with their freedom to break things.

Locking the upper bound is just a sure way to get issues opened about inability to use the library as NuGet won't allow the update 🙂

@lahma
Copy link
Contributor Author

lahma commented Apr 14, 2024

Also as a sidenote, we did intentionally break public API surface with Jint 3.1 for some advanced cases that I had vetted for against the public libraries using Jint (like yours). As Jint doesn't promise semantic versioning it would be correct to lock against very specific version (3.0.1 allowed, not 3.0.2).

Of course in ideal world it would be semantic versioning but in this case we paved the road for v4 changes - where I would expect that your integration would be actually binary compatible with the API surface you are using even though we are replacing the whole JS parsing library. I try my best check downstream consumers when they are public OSS libraries.

@FlorianRappl
Copy link
Contributor

Yeah I fully agree - the thing is that only the version a lib was developed against would therefore work ("work" in the sense of "works as intended / covered by the test cases"). Because any other version might break; even if semver is tried to be followed there could be something in that would break API or even ABI compatibility.

That being written it's a hard problem without any good solution. The direction we want to take with AngleSharp is to allow the broadest set. All libs of AngleSharp (e.g., AngleSharp.Io, AngleSharp.Xml, ...) are released with support for the same major version of AngleSharp (AngleSharp.Io 1.x requires / supports AngleSharp 1.x etc.). To reflect this support on NuGet install we use the semver ranges.

I know that this is a discussion that cannot be solved / where the POV really depends on what your target / desire / mindset / whatever (taste?) is. I fully get your point, too - I just want that we can continue on semver ranges moving forward.

Maybe a post-build step that modifies the NuGet package could be introduced? On the other side that somewhat counters the simplification that we aim for with this PR.

(Sure, in this repo the PR would make sense, but usually we use the same infrastructure in all repos, which means this change cannot be transported; counter-acting the simplification)

@lahma
Copy link
Contributor Author

lahma commented Apr 14, 2024

Maybe a post-build step that modifies the NuGet package could be introduced? On the other side that somewhat counters the simplification that we aim for with this PR.

(Sure, in this repo the PR would make sense, but usually we use the same infrastructure in all repos, which means this change cannot be transported; counter-acting the simplification)

Maybe it boils down to whether you would be willing to have specific project (repos) to have the nuspec in use to achieve the goal you want. If the library you are publishing only needs core MS dependencies I would argue that it's safe to allow any range without upper bound. So if the JS integration is just "exception that confirms the rule" it would justifiable (maybe) to have specific packaging logic there.

I know it's a pain to know that any specific repo needs nuspec file updates when dependencies change, but in the other hand I feel the automation from csproj very compelling.

@FlorianRappl
Copy link
Contributor

Well it's all the repos except this one; as all the repos have at least a dependency to AngleSharp - and (as explained) this one is major-version only.

The JS-one is IIRC the only with with a non-AngleSharp dependency (Jint); MS-dependencies excluded.

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.

None yet

3 participants