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.affine: init at 2.2.1 #50951

Merged
merged 7 commits into from Nov 26, 2018
Merged

pythonPackages.affine: init at 2.2.1 #50951

merged 7 commits into from Nov 26, 2018

Conversation

mredaelli
Copy link
Contributor

cartopy: init at 0.17.0
pyepsg: init at 0.3.2
rasterio: init at 0.34.0
snug: init at 1.3.4
snuggs: init at 1.4.2

A few libraries related to geospatial data analysis

Motivation for this change
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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@FRidh
Copy link
Member

FRidh commented Nov 23, 2018

please rebase instead of merging

@mredaelli
Copy link
Contributor Author

Ack

@c0bw3b
Copy link
Contributor

c0bw3b commented Nov 23, 2018

@GrahamcOfBorg eval

@mredaelli
Copy link
Contributor Author

@c0bw3b Sorry, do you know why earlier it was failing while now, after your message, it worked?
Did I do something wrong?

@c0bw3b
Copy link
Contributor

c0bw3b commented Nov 23, 2018

@mredaelli no you did nothing wrong, it was a temporary breakage in master branch

worldofpeace
worldofpeace reviewed Nov 24, 2018
View reviewed changes
pkgs/development/python-modules/cartopy/default.nix Outdated
];

meta = with lib; {
description = ''Python package designed for geospatial data processing in
Copy link
Contributor

Choose a reason for hiding this comment

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

This description can be shortened for brevity.

Something like

"Process geospatial data to create maps and perform analyses"

worldofpeace
worldofpeace reviewed Nov 24, 2018
View reviewed changes
pkgs/top-level/python-packages.nix Outdated
@@ -4998,6 +5010,11 @@ in {

nanoleaf = callPackage ../development/python-modules/nanoleaf { };


Copy link
Contributor

Choose a reason for hiding this comment

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

There's whitespace here from before.

@worldofpeace
Copy link
Contributor

worldofpeace commented Nov 24, 2018 •
edited

Also looks like accidentally squashed your commits.

Anyway they should be in the format python: attribute-name: init at version

@worldofpeace
Copy link
Contributor

Going to check if they build properly now since my feedback isn't critical to their function.

@GrahamcOfBorg pythonPackages.cartopy python3Packages.cartopy pythonPackages.pyepsg python3Packages.pyepsg pythonPackages.rasterio python3Packages.rasterio pythonPackages.snug python3Packages.snug pythonPackages.snuggs python3Packages.snuggs

@mredaelli
Copy link
Contributor Author

Cartopy works only with Python 3.6. Not sure if that's a problem, or how to manage it

@worldofpeace
Copy link
Contributor

Cartopy works only with Python 3.6. Not sure if that's a problem, or how to manage it

buildPythonPackages takes a parameter disabled when true, it won't build for that python interpreter version.

However from SciTools/cartopy#1186 I can see that it does work for other interpreter versions, though they are looking to drop support for python2 very soon.

Shouldn't be a problem yet.

@mredaelli
Copy link
Contributor Author

mredaelli commented Nov 24, 2018 •
edited

It's the (optional) dependency on pyproj through owslib, which doesn't work: pyproj4/pyproj#136

Should I add a withOwslib parameter for the derivation?

@worldofpeace
Copy link
Contributor

Should I add a withOwslib parameter for the derivation?

This should be fine

diff --git a/pkgs/development/python-modules/cartopy/default.nix b/pkgs/development/python-modules/cartopy/default.nix
index 4c6e05149fc..efe26b217fa 100644
--- a/pkgs/development/python-modules/cartopy/default.nix
+++ b/pkgs/development/python-modules/cartopy/default.nix
@@ -1,6 +1,6 @@
 { buildPythonPackage, lib, fetchPypi,
   pytest, filelock, mock, pep8,
-  cython,
+  cython, isPy37,
   six, pyshp, shapely, geos, proj, numpy,
   gdal, pillow, matplotlib, pyepsg, pykdtree, scipy, owslib, fiona
 }:
@@ -24,8 +24,8 @@ buildPythonPackage rec {
     six pyshp shapely geos proj numpy
 
     # optional
-    gdal pillow matplotlib pyepsg pykdtree scipy owslib fiona
-  ];
+    gdal pillow matplotlib pyepsg pykdtree scipy fiona
+  ] ++ lib.optionals (!isPy37) [ owslib ]; # owslib is broken with python3.7
 
   meta = with lib; {
     description = ''Process geospatial data to create maps and perform analyses.'';

worldofpeace
worldofpeace reviewed Nov 25, 2018
View reviewed changes
pkgs/development/python-modules/cartopy/default.nix Outdated
] ++ lib.optionals (!isPy37) [ owslib ]; # owslib is broken with python3.7

meta = with lib; {
description = ''Process geospatial data to create maps and perform analyses.'';
Copy link
Contributor

Choose a reason for hiding this comment

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

The old description sneaked in somehow.

dotlambda
dotlambda requested changes Nov 25, 2018
View reviewed changes
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

I'd prefer beginning the commit messages with pythinPackages.<attrname> but that's up to you.

pkgs/development/python-modules/cartopy/default.nix Outdated

# optional
gdal pillow matplotlib pyepsg pykdtree scipy fiona
] ++ lib.optionals (!isPy37) [ owslib ]; # owslib is broken with python3.7
Copy link
Member

Choose a reason for hiding this comment

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

WIll probably be fixed by #50982.

Copy link
Member

Choose a reason for hiding this comment

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

You should be able to rebase on master and get rid of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, locally it worked both for python 3.6 and 3.7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ready for review :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually opened that to fix this :)

pkgs/development/python-modules/rasterio/default.nix Show resolved Hide resolved
pkgs/development/python-modules/snuggs/default.nix Show resolved Hide resolved
@mredaelli mredaelli force-pushed the geopystuff branch 4 times, most recently from 10748ff to 1ae2f56 Compare November 25, 2018 20:27
@worldofpeace
Copy link
Contributor

@dotlambda

I'd prefer beginning the commit messages with pythinPackages. but that's up to you.

I'm seeing a lot of different commit msg for python so I don't even know what to suggest to people 😄

@alyssais
Copy link
Member

I'd prefer beginning the commit messages with pythinPackages. but that's up to you.

I'm seeing a lot of different commit msg for python so I don't even know what to suggest to people 😄

pythonPackages.whatever: commit messages mean that ofborg will automatically build them (if you have that access), so recommending that one to everybody is probably most sensible for consistency…

worldofpeace
worldofpeace previously approved these changes Nov 25, 2018
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

I've successfully built all of the packages locally(both python attributes).

Everything looks great to me, however we should wait if @dotlambda has any other feedback.

@worldofpeace worldofpeace dismissed their stale review November 25, 2018 23:22

need to disable pyepsg tests

@dotlambda dotlambda changed the title affine: init at 2.2.1 pythonPackages.affine: init at 2.2.1 Nov 26, 2018
worldofpeace
worldofpeace approved these changes Nov 26, 2018
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

@mredaelli Again, I'm able to build these all successfully 👍

I think this is good to merge.
I'll go ahead and do so once we have @dotlambda approval.

@dotlambda dotlambda merged commit c4f8216 into NixOS:master Nov 26, 2018
@worldofpeace
Copy link
Contributor

@mredaelli Thanks for contributing and bearing with us when our reviews weren't converging 😄

@mredaelli
Copy link
Contributor Author

@worldofpeace Not at all! It was very instructive, thank you all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alyssais alyssais alyssais left review comments

@dotlambda dotlambda dotlambda requested changes

@worldofpeace worldofpeace worldofpeace approved these changes

@FRidh FRidh Awaiting requested review from FRidh

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

Successfully merging this pull request may close these issues.

None yet
7 participants
@mredaelli @FRidh @c0bw3b @worldofpeace @alyssais @dotlambda @GrahamcOfBorg