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

gdal2 v2 #275

Closed
wants to merge 16 commits into from
Closed

gdal2 v2 #275

wants to merge 16 commits into from

Conversation

rbuffat
Copy link
Contributor

@rbuffat rbuffat commented Sep 23, 2015

Done:

  • Seperated ogerext / ograpi for gdal1 and 2.
  • Ported ogrext / ograpi to gdal2
  • Replace OGR_DETECTED_ENCODING/OLC_STRINGSASUTF8 to utf8 for gdal2
  • Detect gdal2 in setup.py and copy the right implementation

Open issues:

  • Gdal switch for windows builds (no gdal-config --version available)
  • Internal handling of bigints (32/64bit Python 2x)
  • Add tests

@sgillies
Copy link
Member

@rbuffat do you want to take on the open issues within this PR or are they better done in new branches?

@rbuffat
Copy link
Contributor Author

rbuffat commented Sep 28, 2015

@sgillies a seperate branch would be nice, especially as this enables other to contribute more easily

@sebastic
Copy link
Contributor

I've tested these changes in the fiona Debian package, and I needed these additional patches to make the build with GDAL 2.0.1 succeed:

$ cat debian/patches/0004-clean-setup.patch
Description: Don't cythonize in clean target.
 Just like [rasterio](https://github.com/mapbox/rasterio/blob/master/setup.py#L141)
Author: Bas Couwenberg <sebastic@debian.org>

--- a/setup.py
+++ b/setup.py
@@ -131,7 +131,7 @@ ext_options = dict(
     extra_link_args=extra_link_args)

 # When building from a repo, Cython is required.
-if os.path.exists("MANIFEST.in"):
+if os.path.exists("MANIFEST.in") and "clean" not in sys.argv:
     log.info("MANIFEST.in found, presume a repo, cythonizing...")
     if not cythonize:
         log.critical(
$ cat debian/patches/0005-builtins.patch 
Description: Use __builtin__ module for Python 2.
 Fixes exception:
  ImportError: No module named builtins
Author: Bas Couwenberg <sebastic@debian.org>

--- a/fiona/ogrext2.pyx
+++ b/fiona/ogrext2.pyx
@@ -23,7 +23,11 @@ from fiona.rfc3339 import FionaDateType,

 from libc.stdlib cimport malloc, free
 from libc.string cimport strcmp
-from builtins import int
+
+if sys.version_info > (3,):
+    from builtins import int
+else:
+    from __builtin__ import int

 log = logging.getLogger("Fiona")
 class NullHandler(logging.Handler):
--- a/tests/test_props.py
+++ b/tests/test_props.py
@@ -2,11 +2,16 @@ import json
 import os.path
 from six import text_type
 import tempfile
+import sys

 import fiona
 from fiona import prop_type, prop_width
 from fiona.rfc3339 import FionaDateType
-from builtins import int
+
+if sys.version_info > (3,):
+    from builtins import int
+else:
+    from __builtin__ import int

 def test_width_str():
     assert prop_width('str:254') == 254

@rbuffat
Copy link
Contributor Author

rbuffat commented Oct 25, 2015

@sebastic
Regarding the builtins import: I used the information from http://python-future.org/what_else.html#int. On the CI we test for Python 2.7, 3.3 and 3.4 and it does not fail: https://travis-ci.org/Toblerity/Fiona/builds/87333488
I'm courious why it fails for you.

@rbuffat
Copy link
Contributor Author

rbuffat commented Oct 25, 2015

@sgillies seems like the issues about gdal2 start to increase.
I added the argument "gdalversion" to setup.py for cases when gdal-config is not available. Do you know someone that builds fiona for windows and could test that? E.g. http://www.lfd.uci.edu/~gohlke/pythonlibs/ However it is not done in a nice way. The nice way to do it would be to subclass command as in https://stackoverflow.com/questions/677577/distutils-how-to-pass-a-user-defined-parameter-to-setup-py However that needs more setup.py refactoring.

Except from the changes to import builtins suggested by @sebastic I think this PR is ready to be reviewed.

@sebastic
Copy link
Contributor

On 25-10-15 18:49, René Buffat wrote:

Regarding the builtins import: I used the information from http://python-future.org/what_else.html#int. On the CI we test for Python 2.7, 3.3 and 3.4 and it does not fail: https://travis-ci.org/Toblerity/Fiona/builds/87333488
I'm courious why it fails for you.

Because the Debian package build environment didn't have python-future
installed, with the patch the future module isn't required.

If the future module will stay as a new requirement it should also be
included in the 'Python Requirements' in the README.rst to match
requirements.txt.

@sgillies
Copy link
Member

@rbuffat @sebastic I'm 👎 on the future requirement. I think six should cover our needs.

The import of int from builtins can be removed from tests/test_props.py. I'm not sure it's even needed in ogrext2.pyx, to be honest. Let's remove it from that file, too, and only add it if necessary after this PR gets merged.

I've scheduled a bunch of time to work on this, would like to merge early tomorrow and unblock the other great pending work we have!

@sgillies
Copy link
Member

Replaced by #286.

@sgillies sgillies closed this Oct 27, 2015
@coveralls
Copy link

Coverage Status

Coverage remained the same at 72.786% when pulling 01c8b6f on rbuffat:newgdal2 into 3406b62 on Toblerity:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants