Allow running updateScript of package sets

[jian-lin]

When reviewing #2063, I realize that the current package update CI does not work for package sets. This PR fixes that.

Without this patch, package sets, such as bonfire1, cannot be updated in PRs created by .github/workflows/update-packages.yaml.

Before, update and update-all commands relied on flakes. This patch also lifts that restriction.

a follow-up of #1699
cc @eljamm for review

---

With this patch, disabled package sets can be enabled for update. That can be done in future PRs.

[jian-lin]

I plan to compare the update result of update-all with and without this patch. Mark this as draft before I finish the comparison.

[jian-lin]

nix-update -> update.nix of Nixpkgs

After ignoring bonfire package set (update is very slow and should work, see here), adding log for successful updates and adding a few updateScript fixes, we have the following comparison.

success -> failure

• inventaire-unwrapped: no updateScript
• kip: no updateScript
• mcaptcha: no updateScript
• mcaptcha-cache: no updateScript
• meta-press: no updateScript
• openfire-unwrapped: no updateScript
• tlspool-gui: no updateScript
• atomic-server: no updateScript

failure -> success

• anastasis-gtk: not exposed as flake output packages.x86_64-linux.anastasis-gtk because it is broken
• dnsvizor: not exposed as flake output packages.x86_64-linux.dnsvizor because it is a package set
• peertube-plugins: not exposed as flake output packages.x86_64-linux.peertube-plugins because it is a package set

[jian-lin]

This is ready for review @eljamm

[jian-lin]

BTW, during comparison, I find a seemingly wrong update of funkwhale. cc @JuneStepp

funkwhale: 2.0.0-alpha.2 -> 1.4.1

[JuneStepp]

BTW, during comparison, I find a seemingly wrong update of funkwhale. cc @JuneStepp

funkwhale: 2.0.0-alpha.2 -> 1.4.1

It might be easiest to just leave it; they're on release candidate 17 now, so 2.0 probably isn't far away.

[eljamm]

By itself, this change is great and allows us to update packages inside scopes, but this PR actually does more than this, namely replacing nix-update with the Nixpkgs maintainers script. The issue for us here is that we can no longer update packages that don't have an explicit update script, which is a desirable feature for us.

Could we perhaps introduce a default/fallback updater as well?

[jian-lin]

Currently, there are only 8 packages without a updateScript. Adding updateScript for them is not a big deal.

Not a fan of nix-update. Feel free to close this PR and experiment with nix-update.

[ju1m]

Currently, there are only 8 packages without a updateScript. Adding updateScript for them is not a big deal.

Thanks for working on this nice feature @jian-lin, from what you already did, do you already know if it would be equivalent to add the following to the 8 packages?

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

[jian-lin]

@jian-lin Not a fan of nix-update.

Let me elaborate this.

@ju1m if it would be equivalent to add the following to the 8 packages?
passthru.updateScript = nix-update-script { };

Yes. But apparently it is not always correct. For example, some packages need to update to unstable versions and some need to update to latest commit.

@eljamm we can no longer update packages that don't have an explicit update script, which is a desirable feature for us.

This is a misfeature. I just explained why it is not always correct.

@eljamm Could we perhaps introduce a default/fallback updater as well?

We can. It can be done by customizing get-script parameter of update.nix.

[jian-lin]

Let me add one point.

For package sets sharing the same src, there can be race condition if we run updateScript multiple times. @ju1m proposes that we create a dummy package in the package set to run updateScript once.

To express that we do not want to run updateScript on other packages in those package sets, we can not add updateScript to them, which is clear and simple.

If we implement the misfeature, i.e., adding a default updateScript for packages not having one, we have to use another way to express not running updateScript for some packages.

[eljamm]

For package sets sharing the same src, there can be race condition if we run updateScript multiple times.

How does Nixpkgs handle this, if at all?

Yes. But apparently it is not always correct. For example, some packages need to update to unstable versions and some need to update to latest commit.

That's fine. The point is having a sane fallback so we at least attempt to update the packages. If we discover that some have more requirements (unstable releases, updating hashes, ...), we just add explicit update scripts to them. This is how Nixpkgs does it for Python modules and possibly other package sets.

[jian-lin]

This is how Nixpkgs does it for Python modules

Note that unstable versions are only 1.6% of python3Packages. While in Ngipkgs, they are about 50%. So it makes some sense to default to nix-update-script { } in python3Packages and it makes much less sense to do so in Ngipkgs.

How does Nixpkgs handle this, if at all?

Not sure. Note that update.nix of Nixpkgs ignores packages without a updateScript.

---

Anyway, following the logic of choosing nixpkgs flake input, if Ngipkgs maintainers who deal with package updates want to do so, then fine.

[jian-lin]

pushed a new commit which provides a default updateScript

[eljamm]

I ran this locally against the latest main branch, and it's successfully updating package-set items, although it fails for bonfire and dnsvizor:

Packages updated!

Failed to update the following packages:
bonfire.ember
bonfire.open_science
bonfire.social
dnsvizor.hvt
dnsvizor.muen
dnsvizor.qubes
dnsvizor.spt
dnsvizor.unix
dnsvizor.updateScript
dnsvizor.virtio
dnsvizor.xen
openfire

For the former, it's because --repair fails locally (it succeeds in CI):

error: repairing is not allowed because you are not in 'trusted-users'
+ deps=

but for the latter, I assume this PR would fix it.

I'm also not sure about the issue of race conditions that's been mentioned here. I haven't personally encountered it and I can't reproduce it, but I guess we can discuss it in other issues.

This said, not needing flakes to update anymore is obviously a big win, so all in all this looks good to me. Thanks!

[ju1m]

For the former, it's because --repair fails locally (it succeeds in CI):

error: repairing is not allowed because you are not in 'trusted-users'
+ deps=

The correct flag is --rebuild, but still I removed it in #2155 because I had a problem using it that I do not understand yet.

I'm also not sure about the issue of race conditions that's been mentioned here. I haven't personally encountered it and I can't reproduce it, but I guess we can discuss it in other issues.

When an input (flake or FOD) is shared between updated packages, the race condition would happen every time upstream of that input releases a new version during an update. Eg. when updating from v1 to v2, the first package pA of the set would have all hashes and any materialization derived from v2, while the second package pB would have all hashes and any materialization derived from v3 when v3 is release during NGIpkgs updates from v1 to v2. pA would then have invalid hashes and materialization (unless the v3 release did not change anything changing the hashes or materialization, but that's unlikely).

It happened to me with Bonfire because a Bonfire update takes more than 3 hours.

Also, same problem happens without a race condition if a package updates a globally shared flake input.
Hence globally shared input should be updated before updating packages, and all materialization using those inputs regenerated even if their package received no update.

[jian-lin]

Note that in #2154 (comment), I said "adding a few updateScript fixes". Without those fixes, dnsvizor package set fails to update. That's because updateScripts of them are wrong and are orthogonal to this PR which is a updateScript runner/driver running those updateScripts.

Yes, #2174 is one proper fix.

A small "fix" which lets the update process succeeds but not semantically correct is:

--- a/pkgs/by-name/dnsvizor/mirage.nix
+++ b/pkgs/by-name/dnsvizor/mirage.nix
@@ -175,7 +175,7 @@ rec {
           (
             lib.composeExtensions (finalAttrs: previousAttrs: {
               passthru = previousAttrs.passthru or { } // {
-                updateScript = writeShellApplication {
+                updateScript = lib.getExe <| writeShellApplication {
                   name = "${pname}-${target}-update";
                   runtimeInputs = [
                     coreutils
@@ -205,14 +205,6 @@ rec {
     lib.recurseIntoAttrs (
       self
       // {
-        updateScript = writeShellApplication {
-          name = "dnsvizor-update";
-          runtimeInputs = [
-            jq
-            nix
-          ];
-          text = lib.concatMapStringsSep "\n" (target: lib.getExe self.${target}.updateScript) targets;
-        };
       }
     );
 }