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.

Sign up for GitHub

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

pythonPackages.mesa: init at 0.8.6 #71244

Merged
merged 2 commits into from Oct 21, 2019
Merged

pythonPackages.mesa: init at 0.8.6 #71244

merged 2 commits into from Oct 21, 2019

Conversation

dpaetzel
Copy link
Contributor

Motivation for this change

mesa is a Python 3+ library for creating multi-agent simulations.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.

@dpaetzel dpaetzel requested a review from FRidh as a code owner October 16, 2019 17:51
@dpaetzel dpaetzel force-pushed the add-mesa branch 2 times, most recently from a0ecefa to c8713f3 Compare October 17, 2019 06:25
@dpaetzel dpaetzel force-pushed the add-mesa branch 2 times, most recently from 2551a4d to f87484e Compare October 17, 2019 12:07
@dpaetzel
Copy link
Contributor Author

dpaetzel commented Oct 17, 2019 •
edited

I disabled the tests for the examples directory (but it feels a bit too hacky, to be honest).

@dpaetzel dpaetzel force-pushed the add-mesa branch 2 times, most recently from 21d2231 to 3303093 Compare October 17, 2019 16:20
@flokli
Copy link
Contributor

flokli commented Oct 17, 2019

@GrahamcOfBorg build python3.pkgs.mesa

@jonringer
Copy link
Contributor

do you mind rebasing this on top of master... I seem to be building things that I shouldn't when I review...

git pull -r origin master
git push --force .. ..

@dpaetzel
Copy link
Contributor Author

Yes, sure. 🙂

@jonringer
Copy link
Contributor

figured it out, pyside is importing the mesa package, before it was getting it from the pkgs package scope, but now it's finding in python packages.

$ rg "mesa "
pyside/default.nix
1:{ lib, fetchurl, cmake, buildPythonPackage, pysideGeneratorrunner, pysideShiboken, qt4, mesa }:
18:  buildInputs = [ mesa ];

do you mind adding doing:

  pyside = callPackage ../development/python-modules/pyside {
    inherit (pkgs) mesa;
  };

to top-level/python-modules.nix and naming it: pythonPackages.pyside: inherit mesa from pkgs

@risicle
Copy link
Contributor

risicle commented Oct 20, 2019

nix-review is happy for me, macos 10.13.

@flokli
Copy link
Contributor

flokli commented Oct 21, 2019

@dpaetzel can you address the changes requested from @jonringer? Looks good for me to merge then :-)

@dpaetzel
Copy link
Contributor Author

I finally got to address the pyside request. Sorry it took so long.

jonringer
jonringer approved these changes Oct 21, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nix-review passes on NixOS (python38Packages.pytest is broken on master)
diff LGTM
outputs LGTM
able to import from python interpreter

[2 built (4 failed), 8 copied (0.8 MiB), 0.7 MiB DL]
error: build of '/nix/store/2nni8zqp3cgyn0k12h856j7nalgl9f2w-env.drv' failed
https://github.com/NixOS/nixpkgs/pull/71244
1 package failed to build:
python38Packages.mesa

1 package were build:
python37Packages.mesa

[nix-shell:/home/jon/.cache/nix-review/pr-71244-3]$ nix path-info -Sh ./results/python37Packages.mesa
/nix/store/rs9hnidl4zrakl1ka6kwbvdfgdaaxg2c-python3.7-mesa-0.8.6         403.0M
# mostly due to pandas closure
[nix-shell:/home/jon/.cache/nix-review/pr-71244-3]$ nix path-info -Sh ./result
/nix/store/aml9daqjz8c7z33k3j2sg12p622z50hd-python3.7-pandas-0.25.1      371.6M

@jonringer
Copy link
Contributor

@GrahamcOfBorg build python3Packages.mesa

@dpaetzel
Copy link
Contributor Author

Oh, I probably should exclude mesa from python38Packages then?

@jonringer
Copy link
Contributor

No, python38Packages.pytest is broken on master, so I'm disregarding it for now

@jonringer
Copy link
Contributor

it was fixed in staging, just hasn't made it's way to master yet

@jonringer
Copy link
Contributor

I was wondering why the closure was so huge, i managed to shave off 57MB #71553

@dpaetzel
Copy link
Contributor Author

Ah, that clarifies it. Nice work on the closure size!

@jonringer
Copy link
Contributor

ofborg finally built everything :), merging.

Thanks for sticking through all the pain

@jonringer jonringer merged commit 3db3c78 into NixOS:master Oct 21, 2019
@dpaetzel
Copy link
Contributor Author

Nice! Thank you, reviewers 🙂

@dpaetzel dpaetzel deleted the add-mesa branch October 21, 2019 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flokli flokli flokli requested changes

@FRidh FRidh FRidh left review comments

@jonringer jonringer jonringer approved these changes

Assignees
No one assigned
Labels
6.topic: python 8.has: clean-up 8.has: package (new) 9.needs: reporter feedback 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dpaetzel @flokli @jonringer @risicle @FRidh