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

buildDotnetModule: add support for installing pre-release tools #374663

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

Conversation

gaelj
Copy link

@gaelj gaelj commented Jan 17, 2025

Things done

Added an optional allowPreRelease argument to buildDotnetModule that will set the --prerelease flag to the dotnet tool install command.

This is because some useful Nuget packages have a tendency to stay in alpha release, so it's nice to be able to package them regardless.

We could also add the --prerelease tag unconditionally if that would be acceptable.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@corngood
Copy link
Contributor

We could also add the --prerelease tag unconditionally if that would be acceptable.

I can't think of a good reason not to do this in the install. The version is pinned in the derivation.

Would specifying the version (--version ${version}) explicitly avoid the need for --prerelease?

Explicitly allowing prerelease versions could still be relevant for the update script:

# always skip prerelease versions for now
version=$(curl -fsSL "https://api.nuget.org/v3-flatcontainer/$nugetName/index.json" |
    jq -er '.versions | last(.[] | select(match("^[0-9]+\\.[0-9]+\\.[0-9]+$")))')

Copy link
Contributor

@corngood corngood left a comment

Choose a reason for hiding this comment

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

The fixup commit should be squashed.

@gaelj
Copy link
Author

gaelj commented Jan 18, 2025

Would specifying the version (--version ${version}) explicitly avoid the need for --prerelease?

No, specifying an alpha version explicitly isn't enough. The pre release flag needs to be there to install a non official version or you get an error.

I'll do some more checks. Just noticed the command errors out if we pass both version and pre release flags...

But I agree we should pass the pinned version to the DotNet tool installation command.

I'll also update the regex in the install script.

@gaelj gaelj changed the title buildDotnetModule: add support for installing pre-release tools Dratf: buildDotnetModule: add support for installing pre-release tools Jan 18, 2025
@gaelj gaelj force-pushed the nuget-allow-prerelease branch from f4a0a1e to 5edf781 Compare January 18, 2025 10:30
@gaelj gaelj changed the title Dratf: buildDotnetModule: add support for installing pre-release tools buildDotnetModule: add support for installing pre-release tools Jan 18, 2025
@gaelj gaelj force-pushed the nuget-allow-prerelease branch from 5edf781 to 4568254 Compare January 18, 2025 10:34
@gaelj
Copy link
Author

gaelj commented Jan 18, 2025

You are absolutely correct, @corngood : replacing --prerelease with --version [...] does the trick.

I modified the regex in install.sh. I tested it by running the curl command in a terminal and it works as expected. Not sure how I could test the whole script though.

All commits are now squashed.

@gaelj gaelj requested review from corngood and lostmsu January 18, 2025 11:57
@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 374663


x86_64-linux

✅ 10 packages built:
  • csharp-ls
  • csharpier
  • csharprepl
  • dotnet-ef
  • dotnet-repl
  • fable
  • fantomas
  • gitversion
  • pbm
  • upgrade-assistant

aarch64-linux

✅ 10 packages built:
  • csharp-ls
  • csharpier
  • csharprepl
  • dotnet-ef
  • dotnet-repl
  • fable
  • fantomas
  • gitversion
  • pbm
  • upgrade-assistant

x86_64-darwin

✅ 8 packages built:
  • csharp-ls
  • csharpier
  • csharprepl
  • dotnet-ef
  • dotnet-repl
  • fantomas
  • gitversion
  • upgrade-assistant

aarch64-darwin

✅ 7 packages built:
  • csharpier
  • csharprepl
  • dotnet-ef
  • dotnet-repl
  • fantomas
  • gitversion
  • upgrade-assistant

version=$(curl -fsSL "https://api.nuget.org/v3-flatcontainer/$nugetName/index.json" |
jq -er '.versions | last(.[] | select(match("^[0-9]+\\.[0-9]+\\.[0-9]+$")))')
jq -er '.versions | last(.[] | select(match("^[0-9]+\\.[0-9]+\\.[0-9]+(-[0-9A-Za-z.-]+)?(\\+[0-9A-Za-z.-]+)?$")))')
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to do this by default. This will cause all the existing packages to update to pre-release automatically.

If we're going to do this it should be gated by something like allowPrerelease, but it may not be worth the trouble. We may not want things doing automatic updates to prerelease versions at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my suggestion would be to just leave this out for now.

@gaelj gaelj force-pushed the nuget-allow-prerelease branch from 4568254 to 149eb99 Compare January 20, 2025 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants