Upstream features from r-rmcgibbo bot #187

rmcgibbo · 2021-03-28T22:42:02Z

I've re-based my fork onto the current top-of-tree, and tried to split my (otherwise disorganized) commits into a logical set of features. Each commit should be separate enough to review independently.

The commits are:

Add --post-logs feature to post the build logs of failed packages to gist.github.com and to provide a link to the log in the github commit. In order to stay within usage restrictions of gist.github.com, this truncates the logs to 1MB if they're extremely long, and also strips out ANSI color codes. It also takes some care to try to capture stderr from the nix build process that might be relevant. For example, hash mismatches are not reported in the log accessible via nix log, and so you need to take other measures to capture them and associate them with the log.
Add --prefer-edit feature. If running nixpkgs-review from multiple systems with the same github account (e.g. aarch64-linux and x86_64-linux), this feature causes the second upload of the github comment to edit the first comment, to avoid an extra ping to each github user subscribed on the thread.
pre-build-filter: this feature adds a hook point where nixpkgs-review will call and external binary to filter the package set. this hook is used by the r-rmcgibbo bot to implement a dynamic form the the skip-packages feature that tries to skip packages such that the estimated build time falls within a cutoff. This is sufficiently complicated (it involves approximately solving a combinatorial optimization problem called the precedence constrained knapsack problem) that it makes sense as a hook to an external program.
A minor commit to reduce the amount of output from git that is sent to stderr, which is useful for keeping the logs clear.

cc #171 @SuperSandro2000

I will ping this PR once my implementation of the pre-build-filter is available. I'm cleaning that up next.

nixpkgs_review/nix.py

SuperSandro2000 · 2021-03-29T10:18:49Z

default.nix

+  propagatedBuildInputs = [
+    python3.pkgs.humanize
+    # humanize fails to declare its dependency on septools correctly
+    # https://github.com/NixOS/nixpkgs/pull/113060


That PR is merged.

Is that sufficient? I personally call this default.nix with nixpkgs from 20.09.

We can backport this is required.

Would you be able to do that? I've never done a backport.

NixOS/nixpkgs#118153

nixpkgs_review/github.py

nixpkgs_review/nix.py

SuperSandro2000 · 2021-03-29T10:36:07Z

nixpkgs_review/nix.py

+def pre_build_filter(attrs: List[Attr]) -> List[Attr]:
+    for cmd in (
+        cmd
+        for cmd in os.environ.get("NIXPKGS_REVIEW_PRE_BUILD_FILTER", "").split(":")


That ENV needs documentation.

This is a potential area of discussion, @Mic92:

Despite being a small amount of code, for me this is the most important feature. The idea is that I want to run some code to manipulate the set of attrs that the rest of the code deals with, to set some as skipped=True based on a dynamic calculation that I can control. Basically a "plugin".

I can think of two possible ways to achieve this feature. One is as written, where nixpkgs-review connects with these external plugins by environment variable and subprocesses them. A different way to do it that you might prefer would be to use import.metadata entry_points, which is kind of a python plugin feature. We could agree on the name of the entry point, and then this pre_build_filter function would find the plugins by iterating over the matching entry points rather than looking through an environment variable. The big difference here would be that the plugin would execute inside the same process as the main python interpreter.

I am open to either mechanism.

I will have a look this evening into this PR. Thanks for leading the review so far @SuperSandro2000 . Due to diverse requirements and languages used in the community a language-independent solution might be preferred.

@ryantm is this something that nixpkgs-update could benefit from as well? If not what is missing?

I added some documentation to the README.md

nixpkgs_review/report.py

setup.cfg

SuperSandro2000 · 2021-03-29T10:39:48Z

Thanks for doing this! ❤️

nixpkgs_review/nix.py

SuperSandro2000 · 2021-04-01T12:06:20Z

nixpkgs_review/github.py

        return None
+
+    def upload_gist(self, name: str, content: str, description: str) -> Dict[str, Any]:


Can we add an option to use a different token for posting gists? I have them on my bot account but would like to post as me.

Would you want to control this by providing an environment variable like GITHUB_GIST_TOKEN that would have a higher precedence than the other mechanisms for setting the token?

Thats how I do it right now. Would be great if you could add this here, too :)

How about adding build logs within the comment instead of a gist? NixOS/nixpkgs#125133 (comment)

Than we don't have this issue at all.

That seems strictly worse to me. At least in my browser (firefox), there's no dedicated HTML page for files embedded within the comment, so glancing at each log requires downloading the file and navigating to the downloads folder and opening it up.

Fair enough.

Mic92 · 2021-05-31T05:27:48Z

default.nix

@@ -5,6 +5,14 @@ python3.pkgs.buildPythonApplication rec {
  name = "nixpkgs-review";
  src = ./.;
  buildInputs = [ makeWrapper ];
+
+  propagatedBuildInputs = [
+    python3.pkgs.humanize


Eventually I want get rid of humanize/backport again, but I can also do this later on master.
Right now we are unsetting PYTHONPATH to avoid injecting dependencies into nix-shell.

Do you prefer if I vendor the naturaldelta function from humanize into this PR, or we could also do something based on wrappers so that we get python3.pkgs.humanize on sys.path even though external settings of PYTHONPATH are ignored?

Vendoring the code is probably less hacky. Maybe we can also rewrite the function if it is less complex.

…tly to gist.github.com

… to build

… from which to fetch the ofborg evalation. This is not required for normal users, but can be useful when nixpkgs-review is run as part of a distributed system like r-rmcgibbo, and you want to use an ofborg evaluation that has since been invalidated by a force-push to the PR and you do not want to fall back to local evaluation.

happysalada · 2021-07-04T00:35:13Z

@rmcgibbo thank you for this! ❤️

SuperSandro2000 · 2021-07-06T14:15:40Z

@rmcgibbo Can you take a look at the tests failures? I think it is unable to parse remotes that are local files.

Artturin · 2021-10-14T19:34:55Z

@Mic92 the test log has expired, please rerun it

SuperSandro2000 · 2021-10-14T20:52:44Z

@Mic92 why can't I rerun the tests?
The author of the PR can always trigger a test run by force pushing.

rmcgibbo · 2021-10-14T21:52:30Z

I made a half-heared attempt to resolve the conflicts.

rmcgibbo commented Mar 28, 2021

View reviewed changes

nixpkgs_review/nix.py Show resolved Hide resolved

rmcgibbo force-pushed the r-rmcgibbo-1 branch 3 times, most recently from d75a270 to e5e01e6 Compare March 28, 2021 23:22

SuperSandro2000 reviewed Mar 29, 2021

View reviewed changes

rmcgibbo force-pushed the r-rmcgibbo-1 branch 8 times, most recently from 88fcdd8 to 5b5393b Compare March 30, 2021 13:09

SuperSandro2000 mentioned this pull request Mar 31, 2021

Explore how to combine comments SuperSandro2000/nixpkgs-review-checks#2

Closed

SuperSandro2000 reviewed Apr 1, 2021

View reviewed changes

nixpkgs_review/nix.py Show resolved Hide resolved

SuperSandro2000 reviewed Apr 1, 2021

View reviewed changes

Mic92 reviewed May 31, 2021

View reviewed changes

rmcgibbo added 8 commits June 6, 2021 14:45

--post-logs: add feature to upload build logs of failing builds direc…

3e8aa68

…tly to gist.github.com

--prefer-edit: add feature to edit prior github comment

a125048

pre-build-filter: provide a hook for customization of the package set…

ae8c111

… to build

minor: send fewer subprocess message to stderr

f6a2b36

Fix rmcgibbo/r-rmcgibbo#7

d7bd3ab

!fixup: cache _store_dir

29ad00d

!fixup: github.py: support other remotes

7c07e78

rmcgibbo force-pushed the r-rmcgibbo-1 branch from 99c5c06 to da07d63 Compare June 6, 2021 18:47

!fixup: document NIXPKGS_REVIEW_PRE_BUILD_FILTER

2161d4c

rmcgibbo force-pushed the r-rmcgibbo-1 branch from da07d63 to 2161d4c Compare June 6, 2021 18:48

!fixup: black formatting

b5bc2ea

Mic92 mentioned this pull request Jul 3, 2021

Add feature to post logs of failed builds #206

Open

Merge branch 'master' into r-rmcgibbo-1

f69986a

SomeoneSerge mentioned this pull request Apr 21, 2023

cudaPackages: bump default cudaPackages_11_7 -> cudaPackages_11_8 NixOS/nixpkgs#224927

Closed

4 tasks

natsukium mentioned this pull request Aug 12, 2024

python312Packages.reflex: init at 0.5.9 NixOS/nixpkgs#333337

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upstream features from r-rmcgibbo bot #187

Upstream features from r-rmcgibbo bot #187

rmcgibbo commented Mar 28, 2021

SuperSandro2000 Mar 29, 2021

rmcgibbo Mar 29, 2021

SuperSandro2000 Mar 29, 2021

rmcgibbo Mar 30, 2021

SuperSandro2000 Mar 31, 2021

SuperSandro2000 Mar 29, 2021

rmcgibbo Mar 30, 2021 •

edited

Loading

Mic92 May 31, 2021

Mic92 May 31, 2021

rmcgibbo Jun 6, 2021

SuperSandro2000 commented Mar 29, 2021

SuperSandro2000 Apr 1, 2021

rmcgibbo Apr 1, 2021

SuperSandro2000 Apr 1, 2021 •

edited

Loading

Mic92 Jun 1, 2021 •

edited

Loading

rmcgibbo Jun 1, 2021

Mic92 Jun 1, 2021

Mic92 May 31, 2021

rmcgibbo Jun 6, 2021

SuperSandro2000 Jun 6, 2021

happysalada commented Jul 4, 2021

SuperSandro2000 commented Jul 6, 2021

Artturin commented Oct 14, 2021

SuperSandro2000 commented Oct 14, 2021

rmcgibbo commented Oct 14, 2021

		return None

		def upload_gist(self, name: str, content: str, description: str) -> Dict[str, Any]:

Upstream features from r-rmcgibbo bot #187

Are you sure you want to change the base?

Upstream features from r-rmcgibbo bot #187

Conversation

rmcgibbo commented Mar 28, 2021

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

rmcgibbo Mar 30, 2021 • edited Loading

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

SuperSandro2000 commented Mar 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperSandro2000 Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Mic92 Jun 1, 2021 • edited Loading

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

happysalada commented Jul 4, 2021

SuperSandro2000 commented Jul 6, 2021

Artturin commented Oct 14, 2021

SuperSandro2000 commented Oct 14, 2021

rmcgibbo commented Oct 14, 2021

rmcgibbo Mar 30, 2021 •

edited

Loading

SuperSandro2000 Apr 1, 2021 •

edited

Loading

Mic92 Jun 1, 2021 •

edited

Loading