tasks/eval: label package updates

[cole-h]

Package version updates that follow the well-established and widely-used
commit message format of attr: 1.0.0 -> 2.0.0 will now be labeled with
the 8.has: package (update) label. Only ASCII arrows (e.g. ->) are
accepted -- Unicode arrows will not trigger the addition of this label.

---

Due to the naivety of this approach, anybody who adds an ASCII arrow to their commit title will get an "update" label, e.g. attr: builtins.fetchTarball -> fetchFromGitHub.

I can't think of a robust solution for detecting these kinds of false-positives, but am open to suggestions for mitigating this. I don't know if it's worth it to add a dependency on regex for this one function, but it's already in our dependency graph...

1. Split on whitespace and check if split[2] == "->" (safely, of course) -- the above example would split to attr:, 1.0.0, ->, and 2.0.0
   • Downside: lib/cli: encodeGNUCommandLine -> toGNUCommandLineShell is a package update (no change from the current solution, but would protect against e.g. lib/cli: change encodeGNUCommandLine -> toGNUCommandLineShell)
2. Regex match on .*: .* -> .*
3. Something else?

---

Closes #318.

[LnL7]

Would labeling all updates be useful practice? Updates are pretty common and can have quite a large range of complexity. I wonder if it would be better to instead filter out updates which are probably easy to review.

Some ideas of things that could be detected.

• We don't parse the changed attributes currently AFAIK, but check if the attr: is in the changed package list.
• Check if the file returned by builtins.unsafeGetAttrPos "src" pkgs.attr is in the diff.
• Check if the diff is relatively small in general, like updating hashes and applying a small build fix.

[cole-h]

As it stands, this PR only aims to tag version bumps (attr: 1.0.0 -> 1.1.0) as a package update, because those are usually easy to review. I don't think using the same conditions as the 8.has: module (update) label (literally just touch a module) is a good idea for the same reasons you note: package updates (not just version bumps) are very common, and labeling them all would add no substance.

If by "filter out updates which are probably easy to review" you mean label packages that probably aren't easy to review with 8.has: package (update), I disagree. People should be able to search for that label for easy reviews rather than have to search for the absence of that label.

---

• We don't parse the changed attributes currently AFAIK, but check if the attr: is in the changed package list.

I don't think using this as a metric for tagging would filter things that are easy to review, because it would get applied to almost every PR. Unless you meant using that as an additional metric, in which case, what's the point? Just sounds like extra complexity for something we can get by just checking for two sequential characters.

• Check if the file returned by builtins.unsafeGetAttrPos "src" pkgs.attr is in the diff.

How do we account for wrapped packages where the source information is defined in the unwrapped attribute and not the attribute in the commit title, e.g. sway vs sway-unwrapped? builtins.unsafeGetAttrPos "src" pkgs.sway returns null.

• Check if the diff is relatively small in general, like updating hashes and applying a small build fix.

I'm not thrilled with this suggestion because it'll just impose an arbitrary limit. Do we accept a diff of 2-5 lines? 2-10? What if the update introduces lots of tests that need to be patched and it then goes above this limit? Even needing to manually specify a buildPhase, checkPhase, or installPhase has the potential to eat up most of this, no matter how trivial the update and/or additions actually are.

Sorry to turn down all your suggestions, but I'm open to hearing more if you got them.

[LnL7]

The first suggestion was to improve update detection in general eg. fetchTarball is a function so it won't show up in the package list even if it's a valid attribute that was changed.

For undafeGetAttrPos take a look at our maintainers.nix or what nix edit does, it should catch most things and ideally things that don't would be fixed so other usecases also work.
Limiting the diff is arbitrary and should allow a bit of wiggle room to fix things, but at some point it's not exactly easy to review anymore even if all the changes are "trivial".

[LnL7]

I think this needs to move to after_merge, since OutPathDiff::find_after is called there. Before that point only the original package outputs are available which is not enough to calculate the diff.

[cole-h]

Ack, you're probably right. I'll need to find a way to get the commit message(s) in there, then.