nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 396749

aarch64-linux

[posting here from matrix to log this more permanently]
Not zero rebuild, but the three things that changed still build fine, and the vast majority didn't change at all.
While zero rebuild would be nice, a couple rebuilds are affordable imo, if nothing breaks.

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 396749

aarch64-linux

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 396749

aarch64-linux

reviewing this at the current state is quite bothersome because github doesnlike to display such big diffs. It might make sense to split the commit into multiple, each doing ~1000 files?

You can also append .patch at the end of the commit URL: https://github.com/NixOS/nixpkgs/commit/64bd63ab68ec9c4ae3325375ebfef3f5d1da780e.patch

Not very convenient, but at least you can see it all.

Helpful command to review (finds changes that are not -rev => +tag):
curl -s https://patch-diff.githubusercontent.com/raw/NixOS/nixpkgs/pull/396749.patch | grep -Ev '^\+\s*tag' | grep -v '^+++' | grep -vE '^diff' | grep -vE '^index' | grep -vE '@@' | grep -vE '^---' | grep -vE '^-\s*rev' | grep -vE '^\s+'

This is not a catch-all, but helps make sure nothing fishy is going on.

I testify that indeed, I'm not doing anything malicious.

@drupol Thanks for this! I have a few questions, though:

1. How did you generate these changes? It would be useful if you provided the commands/scripts you used to do it, so that anyone can verify the changes based on the parent.
2. What heuristic did you use to ensure that rev = "<commit hash>" instances weren't changed? It looks like you did a pretty good job (I can't find any wrong changes), but it would be appreciated if you could clarify. My current guess is you just s/rev = ("v${version}"|version)/tag = ....

Also: I don't think it was the best idea to self-merge this massive change after mere hours (where these sorts of questions could be asked), without any other committer reviewing it, nor even a heads up on Matrix. Please try to do better next time. :(

Did you verify all of these tags exist, and that they give the same hash?
(The first part is easy using the GH API, the second part is rather network-heavy.)

Thank you for the feedback — here’s some background and clarification regarding this PR.

It took me around two days to complete this work.

It all began with #396740, which was initially a simple search-and-replace task. While working on that, I realised it would be more meaningful and semantically correct to replace instances of rev = ... with tag = ... where applicable.

I started by identifying which fetchers support the tag attribute. To my surprise, not all do. The ones I found to support it include fetchFromGitHub, fetchFromGitLab, fetchFromGitea, and fetchgit.

While examining nixpkgs, I identified several patterns, such as:

<fetcher> {
  rev = ...;
}

<fetcher> {
  line1;
  rev = ...;
}

…and so on.
As you can imagine, writing a single regular expression to cover all possible permutations (rev being the 1st, 2nd, 3rd, etc. attribute) was far too complex. So, I decided to use multiple regular expressions tailored to the position of the rev line within the fetcher block.

I used VSCode exclusively for this. A representative search regex I used was something like:

fetchFromGitHub \{[\n\s](.*?)[\n\s](.*?)[\n\s](.*?)(rev = "v\$\{version\}";)

This would catch examples like:

fetchFromGitHub {
  owner = "foo";
  repo = "bar";
  rev = "foobar"; # ← this is the relevant line
}

The process was iterative:

1. I adjusted the regex by adding/removing [\n\s](.*?) blocks to locate patterns where the rev line appeared in different positions.
2. I applied similar logic across the other fetchers that support tag.

As you can imagine, this required a fair amount of scripting and testing. Once I was satisfied with the results, I pushed the PR.

Regarding the merge: you're absolutely right, and I’ll be more mindful next time. I did ask for feedback in two different Matrix channels and received some responses, but I admit that merging it after only a few hours wasn't ideal.

That said, I was resolving conflicts every 30 minutes due to the size and scope of the change, and feared it would become unmanageable if I delayed further.

I also wish I had a good way to identify which packages required a rebuild, but I didn’t find a straightforward method for this. It would be great to have some documentation on how to do that efficiently.

As a follow-up, I started cleaning up the doc/ directory as well — you can find the WIP draft PR here: #396914.

Thanks again for the feedback — it’s much appreciated.

Did you verify all of these tags exist, and that they give the same hash? (The first part is easy using the GH API, the second part is rather network-heavy.)

Echoing this. If you cannot assert that you checked that the hashes didn’t change, I would suggest a revert at this point. We’re a month away from 25.05, I don’t think it’s worth the risk.

After some discussion on Matrix, we've confirmed the affected FODs have not been verified, and decided to revert the change and reland once we're confident we're not breaking anything.

@infinisil @philiptaron @wolfgangwalther Do any of y'all know why the eval failure caused by this PR wasn't caught by the eval action? I can reproduce it locally on master by commenting out

Did some preliminary debugging on the eval issue, and refine is included in the attrpaths superset... so I'm not really sure what's going on here. I have limited testing ability at the moment since the workflow seems to be entirely broken on macOS (I'll fix it eventually, probably a sandbox issue from what I can deduce), but this is very bizarre.

If I'm unable to fix it myself within the next few days, I'll open an issue to track this properly.

I know the dev of refine (skelli, involved in bottles too), and when i asked them back when they first started they wanted to explicitly only support linux Gnome - can you even run a full gnome on darwin? If not, that probably makes no sense to try and support. And if you need to know details about how they manage tagging, i can reach out too.

I know the dev of refine (skelli, involved in bottles too), and when i asked them back when they first started they wanted to explicitly only support linux Gnome - can you even run a full gnome on darwin? If not, that probably makes no sense to try and support. And if you need to know details about how they manage tagging, i can reach out too.

I just mean the eval check, so unrelated. :)

I just saw this, but for future reference the script in #368177 can be adapted to perform this operation and verify each change with nix-build ... --check