Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upstream @rmcgibbo fork #171

Open
SuperSandro2000 opened this issue Feb 17, 2021 · 4 comments
Open

Upstream @rmcgibbo fork #171

SuperSandro2000 opened this issue Feb 17, 2021 · 4 comments

Comments

@SuperSandro2000
Copy link
Collaborator

Upstream the changes in https://github.com/rmcgibbo/nixpkgs-review/commits/r-rmcgibbo especially the (feature to merge commits)[https://github.com/rmcgibbo/nixpkgs-review/commit/df28e262d8eaa63d8e4e7166bde7d7be092b9912].

features:

  • combine commits rmcgibbo@df28e26
  • download tarballs for faster checkout in VMs
  • ignore packages if they contain a ignored dependency
  • and more!

/cc @rmcgibbo

@rmcgibbo
Copy link

rmcgibbo commented Feb 17, 2021

Thanks, @SuperSandro2000.

I just started hacking on this, so I was trying to add the features I wanted for my bot as quickly as possible without really taking the time to file PRs and tests and all that good stuff.

So, some clean up will definitely be required.

In my fork, I've also added hooks at various points to have nixpkgs-review call into external binaries, and I was using those hooks to implement some of the features for my bot:

  • (a) I want to block the upload of the comment to github under various conditions (e.g. if I can determine that the PR was closed, the builder OOMd, ofborg already built the PR, etc), so I created a pre-github-comment hook
  • (b) I want to run nixpkgs-hammering, so I also do that in the pre-github-comment hook
  • (c) I'm also working on a feature to filter the set of drvs that are built -- kind of like --skip-packages but more dynamic -- by running a nix build --dry-run and then solving a precedence-constrained knapsack problem to determine a subset of the build graph that I think I can finish building within a reasonable amount of time. This feature isn't depolyed yet, but my hope is to be able to have r-rmcgibbo build a subset of PRs to staging even if it can't build every drv that changed. Anyways, this is why I introduced the NIXPKGS_REVIEW_PRE_BUILD_FILTER hook.
  • (d) Figuring out how long each individual drv took to build is quite difficult. The best way I was able to figure out requires stat-ing the file in /nix/var/log/nix/drvs where the build log is stored, which is extremely distasteful. And worse, you need to use the new statx system call that only works on some filesystems because the "regular" stat doesn't show the file birth time, and for that you need new code because there's no interface to statx in the python stdlib... Ugh. It's a bit complicated.

I'm still not entirely sure where I should draw the line between nixpkgs-review and my bot's code -- i.e. which features should be implemented in which locations. But I agree in principle that we should upstream the good parts that make sense to upstream. We just need to figure out which parts those are.

@Mic92
Copy link
Owner

Mic92 commented Feb 18, 2021

I try to make to refactor my tests to depend on way less mocking. Than adding new features should become less painful.

@SuperSandro2000
Copy link
Collaborator Author

I think we can safely upstream the following commits: rmcgibbo@df28e26, 2 from rmcgibbo@cd991b6, rev in comment rmcgibbo@73c2237

@Mic92
Copy link
Owner

Mic92 commented Feb 20, 2021

Tests are refactored are here: #173

To make programmatic access easier, I also added a --run argument and the report is stored additionally in json.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants