Skip to content

Conversation

@gepbird
Copy link
Contributor

@gepbird gepbird commented Jan 18, 2025

There are ~100 dotnet packages in nixpkgs, some of them can't be automatically updated, while others have a hacked together update scripts with potential issues that nix-update could already take care of, let's change that!

This PR makes it possible to also run the nuget fetch-deps script using the following snippet in a package with buildDotnetModule:

  passthru.updateScript = nix-update-script { };

I wasn't sure about putting this feature behind the --generate-lockfile flag, as most contributors will probably try nix-update-script { }, which won't update the deps.json file. OTOH, this options is more appropriate for nix-update, as it technically generates a lockfile (but not in the same sense as it generates npm or cargo lockfiles). We should mention this flag in the dotnet docs to make it clear for contributors. I'm curious about your opinions on this, should it work without --generate-lockfile? This feature being locked behind --generate-lockfile was scrapped, it will work without extra arguments.

I did some extra testing outside of nix-update: on https://github.com/gepbird/nixpkgs/tree/nix-update-dotnet-test, I updated 3 packages (osu-lazer, opentabletdriver, celeste64) and made sure they still build.

Documentation on how to update nuget dependencies: https://github.com/NixOS/nixpkgs/blob/b0a6b4fe4aa08c883fecb4b06d8d9a32e91f370c/doc/languages-frameworks/dotnet.section.md#generating-and-updating-nuget-dependencies-generating-and-updating-nuget-dependencies

cc @anpin @corngood @GGG-KILLER @JamieMagee from the dotnet team

@Mic92
Copy link
Owner

Mic92 commented Jan 18, 2025

Afaik we added the --generate-lock-file option for cargo because there is an option to have both the lock file and use only a checksum. However if this only works with the lock file anyway, we can also just generate it unconditionally.

@GGG-KILLER
Copy link

GGG-KILLER commented Jan 18, 2025

Given that updating the deps file is a required step for updating the dotnet packages, I think it's bad UX to lock a required step behind a flag.

It should do that unconditionally since it's part of the update process and doesn't produce a working package without it.

@GGG-KILLER
Copy link

Everything looks good to me now, thanks for this!

@Mic92
Copy link
Owner

Mic92 commented Jan 19, 2025

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Jan 19, 2025

queue

☑️ The pull request will automatically be requeued

Merge queue reset: an external action moved the base branch head to 7bf6549

@corngood
Copy link
Contributor

What's going to happen to packages that already fetch deps in their update scripts? Will it do it twice? It's extremely expensive for some packages.

@Mic92 Mic92 merged commit cea66cb into Mic92:main Jan 19, 2025
4 checks passed
@GGG-KILLER
Copy link

GGG-KILLER commented Jan 19, 2025

What's going to happen to packages that already fetch deps in their update scripts? Will it do it twice? It's extremely expensive for some packages.

This would only happen to packages that call nix-update in their update scripts, which can be remediated by replacing it with the global nix-update script. I don't really see it as too big of an issue nor a reason that we shouldn't improve nix-update with fetch-deps support.

@Mic92
Copy link
Owner

Mic92 commented Jan 19, 2025

Yes. It's either a custom update script or this. Not both.

@gepbird gepbird deleted the dotnet-support branch January 19, 2025 18:59
@corngood
Copy link
Contributor

I believe (as @GGG-KILLER mentioned) there are some packages with update scripts that call nix-update and then call fetch-deps afterward. It's what I've advised people to do recently.

I'm not sure I like the idea of having this run unconditionally in nix-update. Why not just have addNugetDeps wrap the update script for the package to fetch dependencies afterward? That would make it composable with things like fetching npm deps (see avalonia which has both), and with custom update scripts.

@GGG-KILLER
Copy link

The objective of nix-update is to entirely replace update scripts. Making dotnet packages not fully updateable with it goes against its objective.
The mistake in this case was calling nix-update from an update script instead.

@corngood
Copy link
Contributor

The mistake in this case was calling nix-update from an update script instead.

I don't see why packages should be discouraged from calling nix-update and then doing other work. README just says:

Nix-update updates versions/source hashes of nix packages.

This change causes nix-update to make assumptions about the relationship between nugetDeps and fetch-deps, etc. Why would we want tools outside of nixpkgs to be aware of those details?

We already have maintainers/scripts/update-dotnet-lockfiles.nix which uses the existence of fetch-deps rather than nugetDeps to determine when a package uses fetch-deps. This is subtly different.

fetch-deps is essentially just postUpdateScript. Maybe we should standardise on something like that, but I don't know if there's anything to gain over wrapping updateScript?

@gepbird
Copy link
Contributor Author

gepbird commented Jan 20, 2025

I believe (as @GGG-KILLER mentioned) there are some packages with update scripts that call nix-update and then call fetch-deps afterward. It's what I've advised people to do recently.

Good catch! We should replace those update scripts with just passthru.updateScript = nix-update-script { } (if it's sufficient, otherwise just remove the fetch-deps part). There are 9 such dotnet packages found by rg -l fetch-deps $(rg -l nix-update).

This change causes nix-update to make assumptions about the relationship between nugetDeps and fetch-deps, etc. Why would we want tools outside of nixpkgs to be aware of those details?

It already makes assumptions for cargoDeps, pnpmDeps and many more.

We already have maintainers/scripts/update-dotnet-lockfiles.nix which uses the existence of fetch-deps rather than nugetDeps to determine when a package uses fetch-deps. This is subtly different.

I don't mind changing has_nuget_deps = pkg ? nugetDeps; to has_nuget_deps = pkg ? fetch-deps; (or passthru.fetch-deps), but both looked stable for me, and I don't see much benefits other than consistency with a script in nixpkgs.

@GGG-KILLER
Copy link

This change causes nix-update to make assumptions about the relationship between nugetDeps and fetch-deps, etc. Why would we want tools outside of nixpkgs to be aware of those details?

Users themselves already need to be aware of them to update their own packages.

Having fetch-deps is already part of our public API and not something we can change easily. Tools outside of nixpkgs depending on it wouldn't be anything new.

fetch-deps is essentially just postUpdateScript. Maybe we should standardise on something like that, but I don't know if there's anything to gain over wrapping updateScript?

I'd argue that fetch-deps should be part of the update script but since we don't have a way to compose it like NixOS options then we are stuck with the lesser option of having users manually trigger it in their own update scripts or manually by building it and then running it.

@gepbird
Copy link
Contributor Author

gepbird commented Jan 20, 2025

I'd argue that fetch-deps should be part of the update script but since we don't have a way to compose it like NixOS options then we are stuck with the lesser option of having users manually trigger it in their own update scripts or manually by building it and then running it.

We have a way to combine update scripts, for example here: https://github.com/NixOS/nixpkgs/blob/c2699e091cf348ff848132d2f278713b5990aeb7/pkgs/by-name/fa/famistudio/package.nix#L155-L158. But given that it's called experimental and not documented (outside of its source file), we should consider other solutions first.

And just calling nix-update-script { } (for the majority of cases) is more concise, more stable, and contributors who don't want to dig deep into update scripts can reuse their knowledge from their previous non-dotnet packages where nix-update-script { } just worked out of the box.

@corngood
Copy link
Contributor

Users themselves already need to be aware of them to update their own packages.

They don't need to know about nugetDeps. Also some things like dotnetCorePackages.dotnet_8.vmr have fetch-deps, but not nugetDeps, and some packages have something like make-deps.

We have a way to combine update scripts, for example here: https://github.com/NixOS/nixpkgs/blob/c2699e091cf348ff848132d2f278713b5990aeb7/pkgs/by-name/fa/famistudio/package.nix#L155-L158. But given that it's called experimental and not documented (outside of its source file), we should consider other solutions first.

This is exactly what I was imagining addNugetDeps/buildDotnetModule would do. I wonder if it limits calling fetch-deps to when the source has actually changed.

And just calling nix-update-script { } (for the majority of cases) is more concise, more stable, and contributors who don't want to dig deep into update scripts can reuse their knowledge from their previous non-dotnet packages where nix-update-script { } just worked out of the box.

You'd still specify nix-update-script { }, but fetch-deps would be called via wrapper, automatically.

I don't mind changing has_nuget_deps = pkg ? nugetDeps; to has_nuget_deps = pkg ? fetch-deps; (or passthru.fetch-deps), but both looked stable for me, and I don't see much benefits other than consistency with a script in nixpkgs.

I think this would be better than using nugetDeps, but if we do this then we've essentially just made postUpdateScript. There's nothing dotnet specific about it.

I'd still personally rather do composition in nixpkgs. Take a look at https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/av/avalonia/update.bash. It does:

  • nix-update
  • prefetch-npm-deps (3x for different packages, in different directories)
  • fetch-deps

If nix-update calls fetch-deps automatically, it'll likely break because the npm deps need to be updated first. I'd like to get to a point where that can be accomplished by something like _experimental-update-script-combinators.sequence, but for now the shell script is probably the best option.

@gepbird
Copy link
Contributor Author

gepbird commented Jan 20, 2025

They don't need to know about nugetDeps. Also some things like dotnetCorePackages.dotnet_8.vmr have fetch-deps, but not nugetDeps, and some packages have something like make-deps.

Every contributor who wants to make a dotnet package in nixpkgs needs to know about the existance of nugetDeps. nix-update wouldn't be able to help with updating dotnet_8.vmr because it's too complex. It's update script is 114 lines long. A bit off topic, but r-ryantm failed to update it recently: https://r.ryantm.com/log/dotnetCorePackages.dotnet_8.vmr/2025-01-09.log.

You'd still specify nix-update-script { }, but fetch-deps would be called via wrapper, automatically.

Yes, because to update the majority of dotnet packages, you'd need to call fetch-deps. Similarly to update the majority of pnpm packages, you'd need to prefetch the pnpmDeps hash. Both depend on some sort of code from nixpkgs, but both are common use cases. You could argue that to update pnpmDeps, you don't need to generate/run shell commands and for dotnet you do. However to update a package that has a cargo lockfile, manually you'd need to run some cargo commands in the shell, but with nix-update it's done automatically with some assumptions of the build tool and some assumptions of nixpkgs.

I'd still personally rather do composition in nixpkgs. Take a look at NixOS/nixpkgs@master/pkgs/by-name/av/avalonia/update.bash. It does:

For complex packages like avalonia, it's perfectly fine to use composition. In this case we should replace the nix-update to update-source-version that doesn't call fetch-deps and keep manually calling fetch-deps as before. Thanks for also catching this! It's one of that 9 packages (rg -l fetch-deps $(rg -l nix-update)) of which we need to review the update script when this change hits nixpkgs.

I can't emphasize this enough, but nix-update is best suited for automating the entire update process of simple packages. What I mean by simple, is the package uses only 1 ecosystem (eg. dotnet, rust or javascript) and it's update process is fairly straightforward (eg. either fetching nuget deps or updating cargo hashes/lockfiles or updating npm deps hashes) . In some situations it can also handle more complex packages like autobrr that needs a go vendorHash and pnpmDeps.hash update. nix-update is not suited for more complex packages, where it needs to fetch data from multiple resources, especially when the order matters like with avalonia.

@corngood
Copy link
Contributor

corngood commented Jan 20, 2025

Every contributor who wants to make a dotnet package in nixpkgs needs to know about the existance of nugetDeps.

I was thinking of passthru.nugetDeps, which is not the same as the nugetDeps attribute. The former is what it would have been checking I believe. At the very least it's confusing.

In this case we should replace the nix-update to update-source-version that doesn't call fetch-deps

I'm not familiar with update-source-version. Can it handle all the things nix-update can handle in terms of source repositories, etc?

I would still like to have addNugetDeps automatically call fetch-deps after any updateScript passed to it. If some of those are a nix-update, which calls fetch-deps automatically, that's going to be a problem. In that case nix-update wouldn't need to know about fetch-deps.

A bit off topic, but r-ryantm failed to update it recently: https://r.ryantm.com/log/dotnetCorePackages.dotnet_8.vmr/2025-01-09.log.

Thanks. I actually fixed this one recently, but I've been meaning to figure out how to track failed updates.

@gepbird
Copy link
Contributor Author

gepbird commented Jan 20, 2025

I'm not familiar with update-source-version. Can it handle all the things nix-update can handle in terms of source repositories, etc?

It only updates the version, and the src hash, (and can do seperately for each arch-platform combination), it does much less than nix-update, but I think that's what you want for avalonia. I'm not super familiar with it either, but used it a few times and mainly learnt it by looking at how other packages use it (example usecase in a dotnet package: https://github.com/NixOS/nixpkgs/blob/1c7635ce775fae7afd430ef029897c836ebfbcdf/pkgs/by-name/ca/cavalier/update.sh). It has some docs in the source file: https://github.com/NixOS/nixpkgs/blob/1c7635ce775fae7afd430ef029897c836ebfbcdf/pkgs/common-updater/scripts/update-source-version

I would still like to have addNugetDeps automatically call fetch-deps after any updateScript passed to it. If some of those are a nix-update, which calls fetch-deps automatically, that's going to be a problem. In that case nix-update wouldn't need to know about fetch-deps.

Can you explain the reason for this and postUpdateScript (I couldn't find much info on them except 3 packages using addNuGetDeps, and it mostly containing the fetch-deps script)? I probably misunderstand it, but you want to automatically run the fetch-deps script after the package's update script?

Thanks. I actually fixed this one recently, but I've been meaning to figure out how to track failed updates.

Go on either https://nixpkgs-update-logs.nix-community.org/ or https://r.ryantm.com/log/ (the trailing slash is important as it doesn't have a redirect), then select a package and an update attempt. https://nix-community.org/update-bot/ has some interesting docs and a queue, so you can estimate how much you need to wait until your package gets an update attempt :)

@corngood
Copy link
Contributor

Can you explain the reason for this and postUpdateScript (I couldn't find much info on them except 3 packages using addNuGetDeps, and it mostly containing the fetch-deps script)?

postUpdateScript was just a name I invented to describe what what nix-update is doing with fetch-deps. It's an attribute that contains a reference to a script to be executed after a successful update. postUpdateScript just fits in with the naming convention of updateScript etc.

I probably misunderstand it, but you want to automatically run the fetch-deps script after the package's update script?

Yeah, I'd like addNugetDeps (which is used by buildDotnetModule) to wrap the incoming updateScript. That way it'll work regardless of the updateScript. It's where fetch-deps gets added, so it makes sense for it to be responsible for adding it to the update script IMO.

@gepbird
Copy link
Contributor Author

gepbird commented Jan 21, 2025

postUpdateScript was just a name I invented to describe what what nix-update is doing with fetch-deps. It's an attribute that contains a reference to a script to be executed after a successful update. postUpdateScript just fits in with the naming convention of updateScript etc.

Thanks for clarifying! I consider fetch-deps it to be part of the update script (for the majority) of dotnet packages, similarly to how updating packages from other ecosystems is just part of the update script, as I previously mentioned.

Yeah, I'd like addNugetDeps (which is used by buildDotnetModule) to wrap the incoming updateScript. That way it'll work regardless of the updateScript. It's where fetch-deps gets added, so it makes sense for it to be responsible for adding it to the update script IMO.

That would break (or at least run the fetch-deps twice) for almost every existing dotnet packages' update script. Which is not that hard to fix, and I wouldn't opposite this change for this reason.

A rare edge case would be a package similar to avalonia: it needs to first update npm deps, and only then update nuget deps. What if a package needs to do the opposite and update nuget deps first?

Let's suppose this "wrapping the update script with a package ecosystem's postUpdateScript" pattern gets popular and it works for many other languages too. What if you want to package an application using multiple languages, who's postUpdateScript will run first? Will it have some kind of priority, can the user change that, or you just need to disable the update script wrapping functionality for a package? I'm already not a fan of including wrapGAppsHook3 in the the package, then having to write dontWrapGAppsHook = true because it conflicts with qt6.wrapQtAppsHook (see https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/gnome.section.md?plain=1#L193-L204), I imagine it would be something similar with dontWrapDotnetUpdateScript = true. However this is a rare use case, but still has to be implemented.

The only benefit I see currently is a easier maintenance, because the update applying code would live in the same nixpkgs repo.

This would introduce some inconsistencies (at least at the beginning): most packages are updated from nix-update or a custom update script, it's explicit. If a package doesn't have an update script, IIRC nixpkgs-update would implicitly try nix-update or their own update scripts. But with dotnet even after you explcitily define a passthru.updateScript, it would implicitly change that. This should be documented and it's another thing contributors may need to be aware of.

Overall I'm either still misunderstanding/missing something fundamental, or this sounds doesn't sound like a good idea. I prefer keeping the update running code in nix-update, and just use nix-update-script in nixpkgs packages where it works. If it doesn't work because a package is too complex, you can still manually call fetch-deps at the end of a custom update script or with the experimental update script combinator.

@corngood
Copy link
Contributor

corngood commented Jan 21, 2025

Thanks for clarifying! I consider fetch-deps it to be part of the update script (for the majority) of dotnet packages

This is mostly true, but it's still occasionally useful to call it directly. It can pull in updates for dependencies in some cases, or the results can change after a dependency change (e.g. sdk/runtime update).

What if a package needs to do the opposite and update nuget deps first?

I imagine it would be something similar with dontWrapDotnetUpdateScript = true.

Yeah, it would have to either be something like this, or done with overrides on the output of addNugetDeps.

Let's suppose this "wrapping the update script with a package ecosystem's postUpdateScript" pattern gets popular and it works for many other languages too. What if you want to package an application using multiple languages, who's postUpdateScript will run first?

This is essentially why I'd rather not have nix-update try to do it at all, and instead do something like the "experimental update script combinator".

If a package doesn't have an update script, IIRC nixpkgs-update would implicitly try nix-update or their own update scripts. But with dotnet even after you explcitily define a passthru.updateScript, it would implicitly change that.

This is a legit problem if we want to support updating packages without updateScript, but I'm not sure how important that is. We could have addNugetDeps default to nix-update, but that feels wrong.

If it doesn't work because a package is too complex, you can still manually call fetch-deps at the end of a custom update script or with the experimental update script combinator.

To me this is the only practical problem we have right now (avalonia). It could be solved by:

  1. allow fetch-deps to be disabled with e.g. nix-update --no-fetch-deps
  2. replace nix-update with update-source-version

@GGG-KILLER
Copy link

This is essentially why I'd rather not have nix-update try to do it at all, and instead do something like the "experimental update script combinator".

I feel like magically adding fetch-deps to the update script behind the scenes I swhat would be confusing. The .NET ecosystem already has the overrides system for nuget packages which is also obscure and not obvious and I'd rather not introduce more.

To me this is the only practical problem we have right now (avalonia). It could be solved by:

If we only have an issue with a single package then there's no issue, it should just be fixed to not use nix-update as it's supposed to be a tool that can automatically update 90% of packages since they don't need complex/custom steps.
The usage of the tool is what's wrong in this case, I don't think there's much more to discuss. As @Mic92 pointed out earlier, it's either nix-update or a custom update script, not both.

@corngood
Copy link
Contributor

corngood commented Jan 21, 2025

There are loads of custom update scripts that call nix-update.

Avalonia is the one that's going to be broken, but there are others that will fetch deps twice.

@corngood
Copy link
Contributor

corngood commented Feb 6, 2025

replace nix-update with update-source-version

I was going to try this, but I misunderstood what update-source-version does. nix-update is used because it knows how to deal with releases on the source repository (github). In order to avoid duplicating that logic, I think we need to do:

allow fetch-deps to be disabled with e.g. nix-update --no-fetch-deps

@gepbird
Copy link
Contributor Author

gepbird commented Feb 6, 2025

nix-update is used because it knows how to deal with releases on the source repository (github).

If you need to get the latest version number and update the source version and hash, you can use something like this:

new_version="$(curl -s "https://api.github.com/repos/AvaloniaUI/Avalonia/releases/latest" | jq --raw-output '.tag_name')"
update-source-version avalonia "$new_version"

It's a little longer than just calling nix-update, but many packages already use this.

I can't say it enough times, but nix-update is designed to replace the whole update process of simple packages, and most of the time it shouldn't be used in complex packages. I'm not too opposed to adding a --no-fetch-deps flag, but just for avalonia (or other very few edge cases) it seems overkill.

@corngood
Copy link
Contributor

corngood commented Feb 6, 2025

The part of nix-update which understands source repos and is able to fetch releases is a very useful abstraction. Moving that logic back into individual package scripts seems like a mistake.

I can't say it enough times, but nix-update is designed to replace the whole update process of simple packages

The readme just says:

Nix-update updates versions/source hashes of nix packages.

It's nice that it can do certain other helpful things, but right now I'm stuck if I just want it to do that without also running fetch-deps.

It's useful enough in update scripts that it's used in at least all of these:

pkgs/applications/networking/cluster/sonobuoy/update.sh:7:nix-update "${UPDATE_NIX_ATTR_PATH}"
pkgs/applications/networking/instant-messengers/ferdium/update.sh:11:nix-update --version "$latestVersion" --system aarch64-linux --override-filename "$dirname/default.nix" ferdium
pkgs/applications/networking/instant-messengers/ferdium/update.sh:12:nix-update --version skip --system x86_64-linux --override-filename "$dirname/default.nix" ferdium
pkgs/applications/networking/remote/teamviewer/update-teamviewer.sh:6:nix-update --version "$TEAMVIEWER_VER" --system x86_64-linux teamviewer
pkgs/applications/networking/remote/teamviewer/update-teamviewer.sh:7:nix-update --version "skip" --system aarch64-linux teamviewer
pkgs/applications/science/misc/openrefine/update.sh:6:nix-update "${UPDATE_NIX_ATTR_PATH}"
pkgs/applications/science/misc/openrefine/update.sh:7:nix-update "${UPDATE_NIX_ATTR_PATH}.npmPkg" --version=skip
pkgs/development/compilers/abcl/update.sh:5:nix-update abcl --version "$new_version"
pkgs/development/libraries/vulkan-headers/update.sh:20:nix-update glslang --version-regex '(\d+\.\d+\.\d+)' --commit
pkgs/development/python-modules/types-aiobotocore/update.sh:8:nix-update python312Packages.types-aiobotocore --commit --build
pkgs/development/tools/continuous-integration/buildbot/update.sh:5:nix-update buildbot
pkgs/development/tools/continuous-integration/buildbot/update.sh:6:nix-update --version=skip buildbot-plugins.www
pkgs/development/tools/continuous-integration/buildbot/update.sh:7:nix-update --version=skip buildbot-plugins.console-view
pkgs/development/tools/continuous-integration/buildbot/update.sh:8:nix-update --version=skip buildbot-plugins.waterfall-view
pkgs/development/tools/continuous-integration/buildbot/update.sh:9:nix-update --version=skip buildbot-plugins.grid-view
pkgs/development/tools/continuous-integration/buildbot/update.sh:10:nix-update --version=skip buildbot-plugins.wsgi-dashboards
pkgs/development/tools/continuous-integration/buildbot/update.sh:11:nix-update --version=skip buildbot-plugins.badges
pkgs/servers/web-apps/plausible/update.sh:6:nix-update plausible
pkgs/servers/web-apps/plausible/update.sh:11:nix-update --url "$source_url" --version "$version" plausible.tracker
pkgs/servers/web-apps/plausible/update.sh:12:nix-update --url "$source_url" --version "$version" plausible.assets
pkgs/servers/home-assistant/custom-lovelace-modules/zigbee2mqtt-networkmap/update.sh:9:nix-update --override-filename "$dirname/default.nix" home-assistant-custom-lovelace-modules.zigbee2mqtt-networkmap
pkgs/servers/home-assistant/custom-lovelace-modules/atomic-calendar-revive/update.sh:42:nix-update --version "$LATEST_VERSION" "$ATTR"
pkgs/tools/misc/ntfy-sh/update.sh:22:nix-update ntfy-sh --version $version
pkgs/tools/package-management/nix/update-all.sh:34:nix-update --override-filename "$SCRIPT_DIR/default.nix" --version branch --build --commit "nixVersions.git"
pkgs/tools/security/metasploit/update.sh:20:nix-update metasploit --version "$latest"
pkgs/by-name/av/avalonia/update.bash:8:nix-update "$package"
pkgs/by-name/bu/butterfly/update.sh:18:nix-update butterfly
pkgs/by-name/co/codebuff/update.sh:15:nix-update codebuff --version "$version"
pkgs/by-name/go/gopeed/update.sh:18:nix-update gopeed.libgopeed
pkgs/by-name/ka/kafka-cmak/update.sh:4:nix-update kafka-cmak
pkgs/by-name/ka/kazumi/update.sh:17:nix-update kazumi
pkgs/by-name/km/kminion/update.sh:4:nix-update kminion
pkgs/by-name/ma/magento-cloud/update.sh:6:nix-update magento-cloud --version "$new_version"
pkgs/by-name/ne/nexusmods-app/update.bash:8:nix-update "$package"
pkgs/by-name/no/node-red/update.sh:21:nix-update node-red --version "$tag"
pkgs/by-name/oc/ocis/update.sh:21:nix-update -vr 'v(.*)' --version=$OCIS_WEB_VERSION ocis.web
pkgs/by-name/oc/ocis/update.sh:22:nix-update -vr 'v(.*)' --version=$UPDATE_NIX_NEW_VERSION ocis
pkgs/by-name/op/opentabletdriver/update.sh:6:nix-update opentabletdriver
pkgs/by-name/po/pomerium/updater.sh:23:nix-update pomerium --version $version
pkgs/by-name/pr/protoc-gen-connect-es/update.sh:30:nix-update protoc-gen-connect-es --version "$version"
pkgs/by-name/pr/protoc-gen-es/update.sh:30:nix-update protoc-gen-es --version "$version"
pkgs/by-name/sh/shopify-cli/update.sh:37:nix-update shopify-cli --version $version
pkgs/by-name/sl/slimevr/update.sh:4:nix-update slimevr
pkgs/by-name/st/stirling-pdf/update.sh:4:nix-update stirling-pdf
pkgs/by-name/su/sunshine/updater.sh:23:nix-update sunshine --version $version
pkgs/by-name/to/tone/update.sh:6:nix-update tone
pkgs/by-name/ui/uiua/update-unstable.sh:4:nix-update --override-filename pkgs/by-name/ui/uiua/unstable.nix --version unstable uiua-unstable
pkgs/by-name/ui/uiua/update-stable.sh:4:nix-update --override-filename pkgs/by-name/ui/uiua/stable.nix --version-regex '^(\d*\.\d*\.\d*)$' uiua
pkgs/by-name/vi/viddy/update.sh:35:nix-update viddy --version "$latestVersion"
pkgs/by-name/we/webcord/update.sh:13:nix-update --version "$latestVersion" webcord
pkgs/by-name/kr/kryptor/update.sh:5:nix-update kryptor
pkgs/by-name/ps/ps3-disc-dumper/update.sh:6:nix-update ps3-disc-dumper
pkgs/by-name/n8/n8n/update.sh:6:nix-update n8n --version "$new_version"
pkgs/by-name/vr/vrcadvert/update.sh:6:nix-update vrcadvert
pkgs/by-name/v2/v2rayn/update.sh:6:nix-update v2rayn

I do think it's a bit confusing that nix-update can call update scripts that call itself, and it works in two different ways:

  • high level (--commit, --use-update-script, --test, etc)
  • low level (how it's used in the update scripts above)

You'd never want to use the 'high level' interface recursively.

So maybe it should be two different tools?

In any case, I appreciate having a tool that can "updates versions/source hashes of nix packages" without me having to worry about whether it comes from github, gitlab, or something else. For now I'll have to duplicate that logic in avalonia, and I think that's a shame.

@Mic92
Copy link
Owner

Mic92 commented Feb 6, 2025

Do you have more examples beyond this a single package that might not work with nix-update? Nix-update is not a magic bullet. There might be always situations where it breaks, i.e the version replacement algorithm is just a stupid heuristic. Also to me it sounds like the workaround will be just slower in this case compared to the optimal solution.

@corngood
Copy link
Contributor

corngood commented Feb 6, 2025

Do you have more examples beyond this a single package that might not work with nix-update?

I haven't found anything else that's actually broken, but most of the dotnet packages will now run fetch-deps twice.

@Mic92 what are your thoughts on all those update scripts above that call nix-update? That's what I've been advising people to do, because it seems way more abstract and less fragile than the alternative, .e.g:

nix-update avalonia

vs

new_version="$(curl -s "https://api.github.com/repos/AvaloniaUI/Avalonia/releases/latest" | jq --raw-output '.tag_name')"
update-source-version avalonia "$new_version"

I don't want to abuse nix-update for something it wasn't intended to do, but it seems valuable for packages not to have to worry about the details of fetching the latest version (especially for github).

If you'd rather nix-update wasn't used that way (from update scripts), then I think there's an opportunity for another tool to fill that gap.

@gepbird
Copy link
Contributor Author

gepbird commented Feb 6, 2025

but most of the dotnet packages will now run fetch-deps twice.

When bumping nix-update in nixpkgs, we should replace the previous update scripts (apart from avalonia's) with just nix-update-script { }. It can be done in a few minutes, there are 9 such cases: rg -l fetch-deps $(rg -l nix-update). I'm happy to do it when a new version is released :)

I don't want to abuse nix-update for something it wasn't intended to do, but it seems valuable for packages not to have to worry about the details of fetching the latest version (especially for github).

I agree that writing low level curl commands to the github API is not the nicest, and there could be a tool to abstract that, but I don't think that tool should be nix-update. For one-off cases like avalonia this code duplication is acceptable IMO. After this PR, most dotnet packages should work out of the box with nix-update-script { }, removing many duplicated update script code, and enabling auto updates for more dotnet packages. Overall these benefits outweigh that 3 line diff that needs to be done to avalonia.

To make everyone happy, we could add the --no-fetch-deps flag, but given the lack of situations where it could be used, this feature mostly just bloats nix-update. If we agree on this idea, I'd be happy to implement it.

@corngood
Copy link
Contributor

corngood commented Feb 6, 2025

we should replace the previous update scripts (apart from avalonia's) with just nix-update-script { }.

This is a good idea. All of them except avalonia look trivial, and a bunch of them make the mistake of calling fetch-deps even when the source hasn't changed.

To make everyone happy, we could add the --no-fetch-deps flag, but given the lack of situations where it could be used, this feature mostly just bloats nix-update. If we agree on this idea, I'd be happy to implement it.

An alternative to this could be to generalise the attribute (as discussed above) into something like postUpdateScript. We could then make buildDotnetModule set it to fetch-deps by default. This would allow something like avalonia to leave it empty without losing fetch-deps. The downside to this is obviously that support would have to roll out through nixpkgs unless we keep fetch-deps support.

I could also just remove fetch-deps from avalonia, but I didn't really want to do that because we have maintainers/scripts/update-dotnet-lockfiles.nix which looks for it.

@gepbird
Copy link
Contributor Author

gepbird commented Feb 6, 2025

I'm not too fond of the postUpdateScript idea, at least yet. There could be some issues with it, what if there are multiple postUpdateScripts for a package, or what if you want to disable them? See #320 (comment) for more details. And even if those potential issues are resolved, I don't see the benefit of it over our current solutions.

What about generalizing --no-fetch-deps to not only disable fetching nuget deps, but to disable fetching pnpm/npm/maven and many other deps? It could possible have a wider use-case and maybe it's worth having in nix-update.

I could also just remove fetch-deps from avalonia, but I didn't really want to do that because we have maintainers/scripts/update-dotnet-lockfiles.nix which looks for it.

I think it's good that it provides a fetch-deps script. Other than its a bit ugly, making a request to github API and update-source-version seems to be a good solution. Just to say some numbers, there are 75 uses of this pattern with rg -l update-source-version $(rg -l "curl.*api.github.com") | wc -l (there are probably a few false positives and false negatives, but it's an approximation).

@corngood
Copy link
Contributor

corngood commented Feb 6, 2025

What about generalizing --no-fetch-deps to not only disable fetching nuget deps, but to disable fetching pnpm/npm/maven and many other deps? It could possible have a wider use-case and maybe it's worth having in nix-update.

I'd be happy with that behaviour. Maybe using fetch-deps in the name would be confusing, but I'm not sure what to call all of that post-update stuff, especially where there are already flags related to 'lock files'.

Just to say some numbers, there are 75 uses of this pattern with rg -l update-source-version $(rg -l "curl.*api.github.com") | wc -l (there are probably a few false positives and false negatives, but it's an approximation).

I would see this as an opportunity to use nix-update in more places, or at least whatever new tool we came up with for dealing with repos. I take your point though, and I'm not strongly against it.

@gepbird
Copy link
Contributor Author

gepbird commented Feb 7, 2025

@Mic92 what is your opinion on adding a new flag, perhaps called --dont-update-deps that would skip all the npm/pnpm/maven/nuget/... dependency updates?

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.

4 participants