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

python3Packages.mailman-web: prevent error from crashing eval #79869

Merged
merged 3 commits into from Feb 13, 2020

Conversation

@jonringer
Copy link
Contributor

@jonringer jonringer commented Feb 11, 2020

Motivation for this change

throwing an error prevents nix-review from evaluating if the package is affected

$ nixpkgs-review pr 78754
$ git -c fetch.prune=false fetch --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0 pull/78754/head:refs/nixpkgs-review/1
$ git worktree add /home/jon/.cache/nixpkgs-review/pr-78754-3/nixpkgs f77e057cda60a3f96a4010a698ff3be311bf18c6
Preparing worktree (detached HEAD f77e057cda6)
HEAD is now at f77e057cda6 pythonPackages.pysaml2: fix tests with fixed & now-expired timestamps
$ git merge --no-commit a7b890985513cad46c7a158553890d9b52f11d87
Automatic merge went well; stopped before committing as requested
error: mailman-web requires django < 2.2
(use '--show-trace' to show detailed location information)
nix eval --json (import /nix/store/9yy1gv7az9mqxak6jsqkyppb4g1zvzl0-nixpkgs-review-2.1.1/lib/python3.7/site-packages/nixpkgs_review/nix/evalAttrs.nix /tmp/tmpbcmvt646) failed to run, /tmp/tmpbcmvt646 was stored inspection
https://github.com/NixOS/nixpkgs/pull/78754 failed to build

regression introduced in 2711c74

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Nothing changed
https://github.com/NixOS/nixpkgs/pull/79869
$ nix-shell /home/jon/.cache/nixpkgs-review/pr-79869/shell.nix
@jonringer jonringer requested a review from lsix Feb 11, 2020
@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Feb 11, 2020

not actually sure how this is different than buildPythonPackage:

if disabled
then throw "${name} not supported for interpreter ${python.executable}"
else
@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Feb 12, 2020

actually, I think I know, the if/else in the web.nix doesn't actually get touched during evaluation, but rather during instantiation, which is why it's breaking the eval.

I could be wrong

@lsix
Copy link
Member

@lsix lsix commented Feb 12, 2020

That's odd, I actually reproduced the disabled implementation for that.

Anyway, if your pattern is better and nicer to the evaluation, it should probably be used everywhere I used mine in 2711c74:

  • pkgs/development/python-modules/django-compat/default.nix
  • pkgs/development/python-modules/mezzanine/default.nix
  • servers/mail/mailman/web.nix
jonringer added 2 commits Feb 11, 2020
@jonringer jonringer force-pushed the jonringer:fix-eval-mailman branch from bb1d080 to 2b29932 Feb 13, 2020
@jonringer jonringer requested a review from FRidh as a code owner Feb 13, 2020
@ofborg ofborg bot added the 6.topic: python label Feb 13, 2020
@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Feb 13, 2020

not sure if this is the perfect solution, but I should be able to review other packages now

@jonringer jonringer merged commit 5f67b6a into NixOS:master Feb 13, 2020
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@jonringer jonringer deleted the jonringer:fix-eval-mailman branch Feb 13, 2020
@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Feb 13, 2020

@lsix I did some more research, your approach was fine, i guess nixpkgs-review will be halted if master gets a commit that disables a package, and that commit isn't present in the PR branch.

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Feb 13, 2020

created an issue Mic92/nixpkgs-review#82

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Feb 13, 2020

not sure if this is the perfect solution, but I should be able to review other packages now

Still can't review, unless people rebase their branches

@jonringer jonringer mentioned this pull request Feb 14, 2020
5 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.