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

Jump to bottom

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

Merged
merged 3 commits into from Feb 13, 2020
Merged

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

merged 3 commits into from Feb 13, 2020

Conversation

jonringer
Copy link
Contributor

@jonringer jonringer commented Feb 11, 2020 •
edited

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 February 11, 2020 22:36
@jonringer
Copy link
Contributor Author

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

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 commented Feb 12, 2020 •
edited

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

@ofborg ofborg bot requested review from peti and alyssais February 13, 2020 06:11
@jonringer
Copy link
Contributor Author

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
@jonringer jonringer deleted the fix-eval-mailman branch February 13, 2020 06:27
@jonringer
Copy link
Contributor Author

@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

created an issue Mic92/nixpkgs-review#82

@jonringer
Copy link
Contributor Author

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
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsix lsix Awaiting requested review from lsix

@FRidh FRidh Awaiting requested review from FRidh

@peti peti Awaiting requested review from peti

@alyssais alyssais Awaiting requested review from alyssais

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jonringer @lsix