chromium,ungoogled-chromium: fix ofborg maintainer pings

[emilylange]

Description of changes

ofborg uses builtins.unsafeGetAttrPos internally, to figure out which maintainers need to be pinged.

e.g:
builtins.unsafeGetAttrPos "version" drv

When using a .json file containing the version via lib.importJSON, this will always return null and thus leading to no pings at all.

This commit works around this, resulting in properly working pings for any changes to the upstream-info file.

A similar thing has been done for element-{web,desktop} in the past (#219821).

ofborg not pinging maintainers can be seen in #245572 and #245710

The ofborg code section in question is https://github.com/NixOS/ofborg/blob/released/ofborg/src/maintainers.nix#L50-L66

And yes, this should probably be fixed in ofborg in the long run, but for now, this works too.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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/)
• 23.11 Release Notes (or backporting 23.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.

---

Result of nixpkgs-review run on x86_64-linux 1

[emilylange]

ofborg didn't ping for this PR either, because the change is a no-op.

cc @primeos @thefloweringash @networkException @squalus @michaeladler

[emilylange]

Rebased to include #245710 (and resolved the merge conflict).

@michaeladler, you reacted with 👍 on the initial PR comment. Does this mean you approve this fix? Any objections to merging? :)

I will also need to manually backport this to 23.05.

[primeos]

The changes look fine to me (although I cannot test/verify the result) - thanks for looking into this! :)

[primeos]

Note: It's a bit unfortunate that this hack is required (twice) but I guess it should be fine.

[emilylange]

Testing this change isn't really straight forward, yeah.
I didn't really think of adding instructions, sorry for that.

As mentioned in multiple places, ofborg uses builtins.unsafeGetAttrPos for a bunch of attr names, including, but not limited to, version¹.

Without this patch:

❯ nix repl -f "<nixpkgs>"
Welcome to Nix 2.15.1. Type :? for help.

Loading installable ''...
Added 19765 variables.
nix-repl> builtins.unsafeGetAttrPos "version" chromium
{ column = 10; file = "/nix/store/nhk2gnp8jf1xshlym3g63m6k7v35p21h-source/pkgs/applications/networking/browsers/chromium/default.nix"; line = 160; }

nix-repl> builtins.unsafeGetAttrPos "version" ungoogled-chromium
{ column = 10; file = "/nix/store/nhk2gnp8jf1xshlym3g63m6k7v35p21h-source/pkgs/applications/networking/browsers/chromium/default.nix"; line = 160; }

Note that it does not return null, but instead the default.nix.
This is because of the inherit, which loses the "pointer".
If we kept the weird builtins.removeAttrs logic together with lib.fromJSON upstream-info.json, is would return null.
Which is why we have to use a .nix instead.

With this patch:

❯ nix repl -f .
Welcome to Nix 2.15.1. Type :? for help.

Loading installable ''...
Added 19832 variables.
nix-repl> builtins.unsafeGetAttrPos "version" chromium           
{ column = 5; file = "/home/me/dev/NixOS/nixpkgs/pkgs/applications/networking/browsers/chromium/upstream-info.nix"; line = 46; }

nix-repl> builtins.unsafeGetAttrPos "version" ungoogled-chromium
{ column = 5; file = "/home/me/dev/NixOS/nixpkgs/pkgs/applications/networking/browsers/chromium/upstream-info.nix"; line = 63; }

---

The actual ofborg thingy can be tested by running the following in a git clone of https://github.com/NixOS/ofborg -- instructions mostly copied from #219821 (comment)

Pointing ofborg to the local nixpkgs checkout:

diff --git a/ofborg/src/maintainers.nix b/ofborg/src/maintainers.nix
index c519725..dbc861e 100644
--- a/ofborg/src/maintainers.nix
+++ b/ofborg/src/maintainers.nix
@@ -1,6 +1,6 @@
 { changedattrsjson, changedpathsjson }:
 let
-  pkgs = import ./. {};
+  pkgs = import ../../../nixpkgs {};
 
   changedattrs = builtins.fromJSON (builtins.readFile changedattrsjson);
   changedpaths = builtins.fromJSON (builtins.readFile changedpathsjson);

Populating the diff:

❯ echo '[["chromium"]]' > changedattrs.json
❯ echo '["pkgs/applications/networking/browsers/chromium/upstream-info.nix"]' > changedpaths.json

Running the script:

❯ nix-instantiate ofborg/src/maintainers.nix --arg changedattrsjson ./changedattrs.json --arg changedpathsjson ./changedpaths.json --eval
{ networkexception = <CODE>; primeos = <CODE>; thefloweringash = <CODE>; }

Without this PR, on the current nixpkgs master branch and telling ofborg that some change to the update-info.json happened, ofborg would simply not return anything:

❯ echo '["pkgs/applications/networking/browsers/chromium/upstream-info.json"]' > changedpaths.json
{ }

Which is why ofborg did not ping maintainers for version bumps in the past, at least those, that only touch the upstream-info.json and nothing else.

---

I am a quite tired; sorry if this doesn't make all that much sense.
But feel free to ask if I should clear up something or elaborate/rephrase.

Footnotes

1. https://github.com/NixOS/ofborg/blob/cda5aa2ac77a70bb5660d8d5614a640aacbe7523/ofborg/src/maintainers.nix#L50-L66 ↩

[michaeladler]

If it works (and the order of the keys is preserved when dumping upstream-info.nix) then I'm fine with it :)

[emilylange]

^Manual backport in #246843 :)