Experiment/maintainer ping #293

grahamc · 2019-01-01T23:47:20Z

Ping maintainers based on the algorithm:

identify all the files changed by a PR
identify all the attributes changed by a PR
identify the file which defines each changed attribute
filter out all attributes whose containing file is not modified by the PR
ping those attributes' maintainers

Presently it is "no-op": posting the list of packages under the grahamcofborg-eval-check-maintainers check to a gist and not pinging anyone. After a few days of "burning in" and seeing how it goes, I plan on actually pinging people.

Here is a demo: NixOS/nixpkgs#53142

output: https://gist.github.com/GrahamcOfBorg/fa9a84d85a7ad2633df025f2735dfa22

tilpner · 2019-01-02T00:01:59Z

Does it need to say pkgs. for every single entry?

grahamc · 2019-01-02T00:07:43Z

No, removed in 8c2a87a. Thanks!

tilpner · 2019-01-02T00:05:03Z

ofborg/src/maintainers.rs

+use ofborg::nix::Nix;
+use std::path::Path;
+use tempfile::NamedTempFile;
+use std::io::Write;


What's your import ordering system? It looks either random or append-to-bottom

tilpner · 2019-01-02T00:07:20Z

ofborg/src/maintainers.rs

+
+impl ImpactedMaintainers {
+    pub fn calculate(nix: &Nix, checkout: &Path, paths: &Vec<String>, attributes: &
+                     Vec<Vec<&str>>) -> Result<ImpactedMaintainers, CalculationError> {


&Vec is never what you want, use &[...]

tilpner · 2019-01-02T00:14:24Z

ofborg/src/maintainers.rs

+            .checkout_ref(&OsStr::new(&hash))
+            .unwrap();
+
+        let remote = env::var("NIX_REMOTE").unwrap_or("".to_owned());


It's very subjective, but I prefer String::new(). I haven't checked whether "".to_owned() gets away with not heap allocations, but String::new() does. Feel free to ignore

tilpner · 2019-01-02T00:15:53Z

ofborg/src/nix.rs

+            attrargs.push(argstr.to_owned());
+        }
+
+        return self.safe_command(Operation::Evaluate, nixpkgs, attrargs, extra_paths);


Unnecessary return

tilpner · 2019-01-02T00:16:19Z

ofborg/src/nix.rs

@@ -150,7 +161,26 @@ impl Nix {
            attrargs.push(attr);
        }

-        return self.safe_command(Operation::Instantiate, nixpkgs, attrargs);
+        return self.safe_command(Operation::Instantiate, nixpkgs, attrargs, vec![]);


Unnecessary return

FRidh

The algorithm looks good to me.

FRidh · 2019-01-02T08:47:54Z

ofborg/src/maintainers.nix

+{ changedattrsjson, changedpathsjson }:
+let
+  pkgs = import ./. {};
+


maybe a with pkgs.lib; with builtins; to improve the readability. Namespacing is typically good, but I don't think it's worth it here. Or do you want to keep track of what's provided by lib since it's vendored?

FRidh · 2019-01-02T08:50:43Z

ofborg/src/maintainers.nix

+      matching = builtins.filter
+        (changed: pkgs.lib.strings.hasSuffix changed filename)
+        changedpaths;
+    in (builtins.length matching) > 0;


( isEmpty = list: (length list) == 0 function in lib would be nice :) )

FRidh · 2019-01-02T08:52:49Z

ofborg/src/maintainers.nix

+    })
+    changedattrs;
+
+validPackageAttributes = builtins.filter


indentation changes

Wishing for a nice nixfmt :)

FRidh · 2019-01-02T08:53:04Z

ofborg/src/maintainers.nix

+  )
+  enrichedAttrs;
+
+  attrsWithPackages = builtins.map


indentation changes

7c6f434c · 2019-01-02T09:52:04Z

3. identify the file which defines each changed attribute 4. filter out all attributes whose containing file is not modified by the PR

What does it mean «defines the changed attribute»? I have a feeling that either we will have problems with all-packages.nix, or with Firefox/Wine (where bumps go in one file and mkDerivation goes into another). Were both these cases considered, or should I try to work out the specifics? (I assume that a change also includes addition/removal; by the way, how do we want to handle aliases.nix ?)

FRidh · 2019-01-02T11:38:00Z

@7c6f434c those cases the algorithm indeed won't cache.

As a work-around, we may continue on 4) with:

if an attribute doesn't have a transitive parent that is already in the result of 4), then extend 4) with that one. I think a breadth first search is needed for this.

It won't solve it entirely, but I don't think we can do any better than that.

7c6f434c · 2019-01-02T11:51:49Z

@7c6f434c those cases the algorithm indeed won't cache.

There is a moderate chance of false-positive on something interesting in all-packages.nix or aliases.

As a work-around, we may continue on 4) with: - if an attribute doesn't have a transitive parent that is already in the result of 4), then extend 4) with that one. I think a breadth first search is needed for this.

I think if you do that (and even if you just don't special-case large files), it is also a good idea to have logic like «going to ping more than 6 people → file an issue in ofborg instead» (you can just ping me, I will file the issue manually)

grahamc · 2019-01-02T14:22:50Z

The current algorithm says an attribute is changed if the derivation it points to changed.

…ainers

…the nixpkgs lib

grahamc · 2019-01-03T03:43:37Z

I've rebased this on top of my branch where I rustfmt'd and cargo clippy'd the codebase, and formatted the Nix code with the changes in nix-mode at NixOS/nix-mode#61. I'm thinking I'll probably deploy this in the morning unless someone would like to review it more carefully.

grahamc · 2019-01-03T15:06:58Z

For people having questions about how ofborg handles' all-packages.nix: NixOS/nixpkgs#53281

it didn't ping anyone because the change was only in all-packages.nix.

7c6f434c · 2019-04-05T12:36:51Z

OK, so we do need a combination of all-packages.nix defined packages, and a mass rebuild, to trigger weird behaviour. Although indeed src for an override defined in all-packages.nix is unfortunate.

grahamc requested a review from LnL7 January 1, 2019 23:47

grahamc mentioned this pull request Jan 1, 2019

pytest: introduce runTests function NixOS/nixpkgs#53142

Closed

10 tasks

tilpner reviewed Jan 2, 2019

View reviewed changes

FRidh reviewed Jan 2, 2019

View reviewed changes

grahamc added 12 commits January 2, 2019 21:49

This nightmare compares files changed, attrs changed, and pings maint…

8333409

…ainers

nix: teach it to evaluate strings passed over the cli

a7de079

Update the maintainers script for clarity on how to call it

789e0d8

Calculate maintainers

5b6a387

Move themaintainer test to be self-contained with a healthy ccopy of …

31f4ec7

…the nixpkgs lib

Extract calculate to its own fuction

293ca1f

Use nice errors

810cdbe

Make the maintainer API public

c058b26

Make a gist with potential maintainers

65bec86

Support passing more authorized paths to the safe evaluator

9ba201d

Delete beta maintainer script

30372bb

Remove 'pkgs.' prefix from each packge

07a5a14

grahamc force-pushed the experiment/maintainer-ping branch from 8c2a87a to 07a5a14 Compare January 3, 2019 03:17

Fixup formatting

5c04e88

grahamc merged commit 6912713 into released Jan 3, 2019

grahamc deleted the experiment/maintainer-ping branch January 3, 2019 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experiment/maintainer ping #293

Experiment/maintainer ping #293

grahamc commented Jan 1, 2019

tilpner commented Jan 2, 2019

grahamc commented Jan 2, 2019

tilpner Jan 2, 2019

tilpner Jan 2, 2019

tilpner Jan 2, 2019

tilpner Jan 2, 2019

tilpner Jan 2, 2019

FRidh left a comment

FRidh Jan 2, 2019

FRidh Jan 2, 2019

FRidh Jan 2, 2019

grahamc Jan 2, 2019

FRidh Jan 2, 2019

7c6f434c commented Jan 2, 2019 via email

FRidh commented Jan 2, 2019

7c6f434c commented Jan 2, 2019 via email

grahamc commented Jan 2, 2019

grahamc commented Jan 3, 2019

grahamc commented Jan 3, 2019

7c6f434c commented Apr 5, 2019

Experiment/maintainer ping #293

Experiment/maintainer ping #293

Conversation

grahamc commented Jan 1, 2019

tilpner commented Jan 2, 2019

grahamc commented Jan 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FRidh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7c6f434c commented Jan 2, 2019 via email

FRidh commented Jan 2, 2019

7c6f434c commented Jan 2, 2019 via email

grahamc commented Jan 2, 2019

grahamc commented Jan 3, 2019

grahamc commented Jan 3, 2019

7c6f434c commented Apr 5, 2019