fscheck / FsCheck

Random Testing for .NET
https://fscheck.github.io/FsCheck/
BSD 3-Clause "New" or "Revised" License
1.15k stars 154 forks source link

Reduce restriction on xUnit dependency #645

Closed tomrijnbeek closed 4 months ago

tomrijnbeek commented 9 months ago

This PR changes the dependency restriction on xUnit for the FsCheck.Xunit package to be less strict. The previous version would require downstream consumers of this library to deal with version overrides since FsCheck was asking for a version <2.5.0 (while other recent dependencies will ask 2.5.0 or higher).

Since xUnit follows semver, this PR should be safe and should cause dependencies to be happy with any version >=2.4.0 and <3.0.0


Below is the old description that's no longer relevant.

This PR updates the xUnit dependencies from 2.4.x to 2.5.x. This was done by running the following command:

dotnet paket update xunit* --filter

This command also removed netcoreapp3.1 from the supported versions list. I believe this should not be a major concern, since netcoreapp3.1 has been out of support for 9 months now. My guess is that xUnit dropped support for it, though their release notes don't explicitly mention it, they just speak of dropping support for UWP (which may be related, but I'm not well versed enough in .NET versioning to understand whether these might be related).

The reason this update is desirable is that FsCheck is currently requesting a version <2.5.0, whereas most other dependencies our project use are slowly all requesting >=2.5.0, so this PR would unblock quite a few dependency updates for us.

ploeh commented 9 months ago

Is this going to prevent users who are still on xUnit.net 2.4.x from using new versions of FsCheck?

kurtschelfthout commented 9 months ago

Following @ploeh 's comment, would it be better to update https://github.com/fscheck/FsCheck/blob/master/src/FsCheck.Xunit/paket.template to have a more relaxed xunit dependency?

tomrijnbeek commented 9 months ago

Yeah, after @ploeh's comment I was indeed thinking the same, I just didn't get around to answering yet.

On my own repository I forced an update to xunit and so far I haven't experienced any problems, so relaxing the requirement to allow both ~2.4.0 and ~2.5.0 should be fine. When I have a spare minute I'll update the PR to reflect this change.

BennieCopeland commented 4 months ago

Version 2.7.0 just dropped today.

bartelink commented 4 months ago

UPDATED: Argh, didn't see Kurt's comment, and the rest of this message is a longer version of that basic point https://github.com/fscheck/FsCheck/pull/645#issuecomment-1725204194

Bottom line is this approach is a bad approach and the PR needs to be changed to make the dependency more relaxed (or abandoned).

I'd do it myself but ultimately I don't believe in using paket for libraries so don't have the energy/interest to fix it as it should be. (For comparison, this is how you trigger such a constraint via a PackageReference)


Given xunit adheres to semver, and various solutions will have a wide mix of xunit versions, I don't think this is a good solution to the overall problem, which I've documented in #647

@tomrijnbeek What are the chances of revising the PR to do this properly and instead make the dependency be looser in the paket.dependencies, i.e. >= 2.4, <3

The resulting impact on the lock file would be similar, but doesn't force a perpetual busywork cycle of updating the package to keep it in sync with the latest xunit


For paket.dependencies, I believe the syntax should be nuget xunit.extensibility.execution -> 2.4 to get the >=2.4, <3 versioning

Have not actually tried; for libraries, I use PackageReference, and that Just Works, i.e. the generated packages specify >= constraints on their dependencies

You can verify the outcome has been achieved by inspecting the generated package from a loval build by uploading it to https://nuget.info/packages (or look in the zip of course)

tomrijnbeek commented 4 months ago

Ah, I missed the update to your latest comment (I went by what GitHub sent me over email originally).

I've gone ahead and reverted my changes to paket.dependencies and paket.lock which, based on @kurtschelfthout's should fix the problem.

I am not huge on paket either, but this should be an easy enough fix that at least makes it a bit easier to manage xunit dependencies.

bartelink commented 4 months ago

Did this yield a correct package, ie. can you confirm the version constraint in the generated nupkg is correct after a build?

tomrijnbeek commented 4 months ago

This is the generated nuspec file:

<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd">
  <metadata>
    <id>FsCheck.Xunit</id>
    <version>1.0.0</version>
    <authors>Kurt Schelfthout and contributors</authors>
    <requireLicenseAcceptance>false</requireLicenseAcceptance>
    <license type="expression">BSD-3-Clause</license>
    <licenseUrl>https://licenses.nuget.org/BSD-3-Clause</licenseUrl>
    <projectUrl>https://fscheck.github.io/FsCheck/</projectUrl>
    <description>Package Description</description>
    <copyright>Copyright 2008</copyright>
    <tags>F# fsharp test random</tags>
    <repository type="git" url="https://github.com/Fscheck/fscheck" />
    <dependencies>
      <group targetFramework=".NETStandard2.0">
        <dependency id="FsCheck" version="1.0.0" exclude="Build,Analyzers" />
        <dependency id="FSharp.Core" version="[5.0.2, 5.1.0)" exclude="Build,Analyzers" />
        <dependency id="xunit.extensibility.execution" version="2.4.1" exclude="Build,Analyzers" />
      </group>
    </dependencies>
  </metadata>
</package>

So no, it seems like just changing paket.template is not enough. My guess is that it falls back on the lock file somehow, but I don't know enough about paket to know the exact intricacies. I spent a bunch of time experimenting, and the current configuration (the one I just pushed) seems to generate what we want for the nuspec file:

<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd">
  <metadata>
    <id>FsCheck.Xunit</id>
    <version>1.0.0</version>
    <authors>Kurt Schelfthout and contributors</authors>
    <requireLicenseAcceptance>false</requireLicenseAcceptance>
    <license type="expression">BSD-3-Clause</license>
    <licenseUrl>https://licenses.nuget.org/BSD-3-Clause</licenseUrl>
    <projectUrl>https://fscheck.github.io/FsCheck/</projectUrl>
    <description>Package Description</description>
    <copyright>Copyright 2008</copyright>
    <tags>F# fsharp test random</tags>
    <repository type="git" url="https://github.com/Fscheck/fscheck" />
    <dependencies>
      <group targetFramework=".NETStandard2.0">
        <dependency id="FsCheck" version="1.0.0" exclude="Build,Analyzers" />
        <dependency id="FSharp.Core" version="[5.0.2, 5.1.0)" exclude="Build,Analyzers" />
        <dependency id="xunit.extensibility.execution" version="[2.4.1, 3.0.0)" exclude="Build,Analyzers" />
      </group>
    </dependencies>
  </metadata>
</package>

The direct dependency being added to paket.dependencies is needed, even if there is already an indirect dependency, because without it dotnet pack returns the following error:

  Checking dependency status for package Some(xunit.extensibility.execution), version Some(>= 2.4.1)
  Couldn't find a version range for package 'xunit.extensibility.execution' in group 'Main', is this package in your pa
  ket.dependencies file?

Updating paket.lock didn't seem necessary.

bartelink commented 4 months ago

Nice work

That paket.dependencies listing requirement sounds like a decent intentional feature (forcing you to list it to be able to put it in a nuspec template)

Aside: This disgusted me but "[2.4.1, 3.0.0)" matches 3.0.0-rc.1, which is why this abomination came to be! (It's possible that later versions of NuGet version matching changed o make it do what you would hope instead, and not match previews of V3)

kurtschelfthout commented 4 months ago

Cheers