tasks/eval: label package updates #503
Conversation
There was a problem hiding this comment.
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.attris in the diff. - Check if the diff is relatively small in general, like updating hashes and applying a small build fix.
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.
|
As it stands, this PR only aims to tag version bumps ( If by "filter out updates which are probably easy to review" you mean label packages that probably aren't easy to review with
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.
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.
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 Sorry to turn down all your suggestions, but I'm open to hearing more if you got them. |
This is slightly more robust than just checking if `->` exists anywhere in the title -- now, `attr: move from fetchTarball -> fetchFromGitHub` won't trigger the addition of the label.
|
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. |
If a commit has an ASCII arrow (`->`) in its title, and the specified attr will be rebuilt, count it as a package update.
| .commit_messages_from_head(&self.job.pr.head_sha) | ||
| .unwrap_or_else(|_| vec!["".to_owned()]); | ||
|
|
||
| if let Some(ref rebuildsniff) = self.outpath_diff { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ack, you're probably right. I'll need to find a way to get the commit message(s) in there, then.
Package version updates that follow the well-established and widely-used
commit message format of
attr: 1.0.0 -> 2.0.0will now be labeled withthe
8.has: package (update)label. Only ASCII arrows (e.g.->) areaccepted -- 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
regexfor this one function, but it's already in our dependency graph...split[2] == "->"(safely, of course) -- the above example would split toattr:,1.0.0,->, and2.0.0lib/cli: encodeGNUCommandLine -> toGNUCommandLineShellis a package update (no change from the current solution, but would protect against e.g.lib/cli: change encodeGNUCommandLine -> toGNUCommandLineShell).*: .* -> .*Closes #318.