Questions about Tracking issues

Development Nixpkgs Architecture
1

I’m interested in getting finalAttrs pushed more across nixpkgs, especially in cases like this where it fixes potential AppImage double extraction (thanks @uku3lig!):

Unfortunately, it seems like bulk PRs like this aren’t a good way to go, and creating 1 LLM-asissted PR for each package is just more spammy.

It seems like a sprintable tracking issue is the way to go for this specific case, but I have a few questions about making one:

  • Is there any way to notify all maintainers of each package listed in a tracking issue?
  • Should we even notify all the maintainers of listed packages in tracking issues?
  • What about if you created an independently verifiable check that the new LLM-assisted finalAttrs output is the byte-for-byte identical (at least excluding embedded store paths) to the old appimageTools.extract pattern? Would a bulk PR/independent PR for each package be more likely to be accepted I was to do that?
    • Do we have any existing tools to help with this? I know that Lila recommends diffoscope for reviewing diffs, but do we have any existing tools to compare the outputs of 2 different derivations w/o self output path noise?
      • Maybe I should make small script like this that subs out the self store paths and then runs diffoscope.
1 Like
2
  1. You would have to manually ping them in the tracking issue, i have seen scripts used to create the issue desc
  2. For finalattrs matters, maybe not, for something EOL, for sure
  3. finalAttrs shound not change the closure at all iirc
1 Like
3

Ah, there were some whitespace changes in the scripts for some of them (additional blank lines), which is why my agent-assisted diffoscope run of the PR I linked had some output path changes. I thought it might have some other changes, but apparently manual extract (usually, at least in the cases I checked) gets called with the same arguments that wrapType2 does, so this is mostly just an overriding/cleanliness change.

4

I made this a few months ago: jackr/finalAttrs-migrator: little project for funzies - Codeberg.org and landed this PR with it. Feel free to use it if it’s helpful :slight_smile:

Be aware that it’s quite hard to land these PRs when they get large, since review is costly.

A more sophisticated approach would build an AST, as opposed to text replacement. I’ll be trying my hand at that in a few months probably.

1 Like
5

My rule of thumb with large scale migration is generally to avoid treewide when rebuild get involved or that the diff is not trivially predictible (with finalAttrs, I would have to look the entire package in cases there is a missing case). Thus I would personally split this one or keep up varous pr small. In any case, theses type of migration can be planned and spread on a long period to avoid spamming prs too much (this is also what was done with pyproject). We had good result with pinging maintainers on meta.changelog, but I think that it is preferable to not ask for modernization.

A good part of the finalAttrs is also happening within other type of cleanup or packages update. For instance with python “format” migration. I do like the approach of individual investigation, to take the time to do things properly, remove unwanted packages or catch problems (in the pyproject, we plenty of weird cases, even some unnoticed broken software…)

1 Like
6

Neat, although a bit different then the specific finalAttrs use-case I’m targeting with AppImage support.
I’ll go ahead and make a simple Tracking issue for all appimageTools-based packages not using finalAttrs, and clean them up and verify as I find the time. Just wanted to make sure I’m doing everything right here!

1 Like
7

Made a tracking issue here:

8

I rarely see a benefit of using finalAttrs over rec or let ... in: it doesn’t make overriding src any easier.

The main pro i see is if you need to be able to react to overrides (e.g. in passthru.tests) or if you need access to finalAttrs.finalPackage. The con on the other hand is more expensive eval.

As such i don’t see the benefit or blindly migrating to it.

9

I’m just interested in this because appimageContents = appimageTools.extract { inherit pname version src; }; is a common pattern and IMO finalAttrs.contents is cleaner.
finalAttrs.contents also responds to src overrides, as opposed to appimageTools.extract in a let block.

1 Like
10

In this case yes, finalAttrs is strictly beneficial :+1:

1 Like
11

Update: after a few very annoying hours with my coding agent, I was able to get an ast-grep-based treewide for some trivial cases (Still needs me to do a manual test to make sure that I didn’t break any packages with this EDIT: 0 rebuilds! Although it did break nixpkgs-review :wink:).

For the rest, I think that I’ll just get my coding agent to fix them in batch with more autonomous self-verification, then do a manual review of their changes and submit them to nixpkgs as I have the time to do review.

1 Like
12

This prior art with treesitter might be of interest in the future GitHub - Aleksanaa/by-name-migrate · GitHub (it also has a neat open PR waiting to get merged)

1 Like
13

Neat, although not all cases are as clear-cut as these. I just thought I’d save tokens (given the amount of tokens it took to make that rule though, I’m not sure it succeeded) and time (definitely failed that one) by doing a simple AST migration for trivial cases, as most other cases are non-trivial (trivial by LLMs, but not by pure deterministic automation, at least not without building in 20 edge cases used by 1 package each).