Use compile_time_env in place of shims #2016

snowman2 · 2020-10-11T00:51:35Z

Reference:

Is this something you would be interested in? I was thinking it might simplify things.

snowman2 · 2020-11-20T00:35:35Z

Can the GDAL 1.x stuff be removed?

sgillies · 2020-11-22T23:54:52Z

@snowman2 unfortunately I didn't warn about dropping support for GDAL 1.x when we released 1.1.0. I think we had better keep it around for one more minor version.

sgillies · 2020-11-23T00:15:18Z

@snowman2 I like simplicity! But we do have some complex needs. We currently include 4 shim .c modules in our source distributions. Will these directives cover our need to be able to build against 4 different categories of GDAL versions from one source distribution?

snowman2 · 2020-11-23T00:39:59Z

Will these directives cover our need to be able to build against 4 different categories of GDAL versions from one source distribution?

That is a good point to bring up. This would probably make cython required to build rasterio from sdist. This shouldn't be an issue with the pyproject.toml file. pyproj has required cython or pip>=10.0.1 since version 2 and haven't had any complaints yet.

Relevant files:

With all of that in place, it should support various versions of GDAL from a single sdist. But, definitely should do some testing to ensure everything works as expected.

That also means this PR likely needs some tweaks.

sgillies · 2020-11-24T00:10:37Z

@snowman2 let's do this after 1.2.0. It's close and I don't want to add anything else and risk pushing it out further.

snowman2 · 2020-12-21T17:20:04Z

With #2064, the GDAL 1.x stuff can be removed. Correct?

snowman2 · 2020-12-30T01:56:03Z

With #2064 I was able to remove the shim files entirely and move things into their respective modules. Seems the only compatibility stuff left is GDAL 3 related.

snowman2 · 2021-01-26T14:55:54Z

@sgillies any blockers remaining?

sgillies · 2021-01-26T17:30:26Z

@snowman2 I'll look at this soon. I need to switch my open source attention to fiona and shapely for a bit, but will get back to rasterio soon.

sgillies · 2021-02-11T02:52:10Z

I've branched main-1.2 off of the master branch and will review this first thing tomorrow.

sgillies · 2021-02-15T17:29:04Z

@snowman2 have you seen https://mail.python.org/archives/list/python-dev@python.org/message/2K3IKBD4K7INMVV3LK6SJY6EXDDNC2M2/? It's an author of Cython arguing for not making Cython a build dependency. I do appreciate not having to deal with unknown versions of Cython on user systems.

snowman2 · 2021-02-15T19:34:04Z

@sgillies, that is definitely an interesting discussion he has there. He has some valid concerns. But, he does also lay out some reasons why you would want to have cython as a build dependency.

For the downsides he has mentioned, I have yet to run into them with pyproj, and it has been a couple of years now. It may be because most users use wheels, conda, or the linux distro install methods most of the time. From my understanding, this is the case for rasterio as well. So, it sounds like if you do depend on cython, it will be for a minority of users that actually compile rasterio from source. And of those users, there is a slight possibility that something might go wrong when cython compiles it due to an outdated version. From my perspective, that is a risk worth accepting.

For the upside of using cython, I see 2:

The simplifications introduced in this PR which makes future changes and bugfixes easier
"you may end up with a setting in which your code
adapts also to newer environments, by using a recent Cython version
automatically."

Side note: The current build from source method for rasterio always uses cython using modern pip due to pyproject.toml and the logic to use cython in setup.py if it is present. If that is not desired, some changes are likely needed.

sgillies · 2021-02-15T21:23:37Z

Yeah, the author doesn't mention build isolation or the fact that we can pin the cython version in pyproject.toml (like https://github.com/pyproj4/pyproj/blob/master/pyproject.toml#L3). I'm more in favor but still want to reflect on this a bit.

sgillies · 2021-02-18T18:51:14Z

rasterio/_base.pxd

@@ -36,3 +36,6 @@ cdef class DatasetBase:
 cdef const char *get_driver_name(GDALDriverH driver)
 cdef OGRSpatialReferenceH _osr_from_crs(object crs) except NULL
 cdef _safe_osr_release(OGRSpatialReferenceH srs)
+cdef void osr_set_traditional_axis_mapping_strategy(OGRSpatialReferenceH hSrs)
+cdef const char* osr_get_name(OGRSpatialReferenceH hSrs)
+cdef GDALDatasetH open_dataset(object filename, int mode, object allowed_drivers, object open_options, object siblings) except NULL


sgillies · 2021-02-18T18:51:56Z

rasterio/_base.pyx

+    IF CTE_GDAL_MAJOR_VERSION >= 3:
+        OSRSetAxisMappingStrategy(hSrs, OAMS_TRADITIONAL_GIS_ORDER)
+    ELSE:
+        pass


sgillies · 2021-02-18T18:55:14Z

rasterio/_env.pyx

+        paths = CSLAddString(paths, path_c)
+        OSRSetPROJSearchPaths(paths)
+    ELSE:
+        os.environ["PROJ_LIB"] = path


sgillies · 2021-02-18T19:09:00Z

setup.py

-                          '-o', 'rasterio/_shim30.c'])
-
-        print(_)
-        sdist.run(self)


sgillies

@snowman2 alright, let's do this! Thank you very much.

snowman2 force-pushed the compile_time_env branch from 53f353b to 37d25ad Compare October 11, 2020 02:54

snowman2 marked this pull request as ready for review October 11, 2020 03:04

snowman2 force-pushed the compile_time_env branch from 37d25ad to 58db89a Compare October 11, 2020 03:07

snowman2 mentioned this pull request Nov 20, 2020

PROPOSAL: WKT Format Support #2044

Closed

snowman2 mentioned this pull request Nov 20, 2020

Shims required to support GEOS version-specific functions in Cython pygeos/pygeos#255

Open

snowman2 force-pushed the compile_time_env branch from 75d5bd5 to bfff4e6 Compare December 12, 2020 00:40

snowman2 force-pushed the compile_time_env branch 2 times, most recently from 24dc498 to 8e16e0a Compare December 30, 2020 01:53

snowman2 force-pushed the compile_time_env branch from 8e16e0a to 4056df1 Compare December 30, 2020 02:10

snowman2 added 6 commits January 25, 2021 19:58

Use compile_time_env in place of shims

f30c825

Update setup.py to require cython

f0ca196

remove sdist_fill

f19b28f

update MANIFEST

dd87641

Remove support for GDAL < 2.3

30d8730

Remove rasterfill.cpp

05de781

snowman2 force-pushed the compile_time_env branch from b8d1b64 to 05de781 Compare January 26, 2021 02:00

sgillies reviewed Feb 18, 2021

View reviewed changes

rasterio/_base.pyx

IF CTE_GDAL_MAJOR_VERSION >= 3:

OSRSetAxisMappingStrategy(hSrs, OAMS_TRADITIONAL_GIS_ORDER)

ELSE:

pass

Copy link

Member

sgillies Feb 18, 2021

Choose a reason for hiding this comment

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

👍

sgillies reviewed Feb 18, 2021

View reviewed changes

rasterio/_env.pyx

paths = CSLAddString(paths, path_c)

OSRSetPROJSearchPaths(paths)

ELSE:

os.environ["PROJ_LIB"] = path

Copy link

Member

sgillies Feb 18, 2021

Choose a reason for hiding this comment

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

👍

sgillies reviewed Feb 18, 2021

View reviewed changes

setup.py

'-o', 'rasterio/_shim30.c'])

print(_)

sdist.run(self)

Copy link

Member

sgillies Feb 18, 2021

Choose a reason for hiding this comment

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

👋

sgillies approved these changes Feb 18, 2021

View reviewed changes

sgillies added this to the 1.3.0 milestone Feb 18, 2021

sgillies added cython enhancement labels Feb 18, 2021

sgillies merged commit 99511e4 into rasterio:master Feb 18, 2021

snowman2 deleted the compile_time_env branch February 18, 2021 19:23

snowman2 mentioned this pull request Jun 21, 2021

REF: Use compile_time_env in place of shims Toblerity/Fiona#1021

Closed

snowman2 mentioned this pull request Jul 12, 2021

CLN: Remove unused shims #2239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use compile_time_env in place of shims #2016

Use compile_time_env in place of shims #2016

snowman2 commented Oct 11, 2020

snowman2 commented Nov 20, 2020

sgillies commented Nov 22, 2020

sgillies commented Nov 23, 2020

snowman2 commented Nov 23, 2020

sgillies commented Nov 24, 2020

snowman2 commented Dec 21, 2020

snowman2 commented Dec 30, 2020

snowman2 commented Jan 26, 2021

sgillies commented Jan 26, 2021

sgillies commented Feb 11, 2021

sgillies commented Feb 15, 2021

snowman2 commented Feb 15, 2021

sgillies commented Feb 15, 2021

sgillies Feb 18, 2021

sgillies Feb 18, 2021

sgillies Feb 18, 2021

sgillies Feb 18, 2021

sgillies left a comment •

edited

Loading

Use compile_time_env in place of shims #2016

Use compile_time_env in place of shims #2016

Conversation

snowman2 commented Oct 11, 2020

snowman2 commented Nov 20, 2020

sgillies commented Nov 22, 2020

sgillies commented Nov 23, 2020

snowman2 commented Nov 23, 2020

sgillies commented Nov 24, 2020

snowman2 commented Dec 21, 2020

snowman2 commented Dec 30, 2020

snowman2 commented Jan 26, 2021

sgillies commented Jan 26, 2021

sgillies commented Feb 11, 2021

sgillies commented Feb 15, 2021

snowman2 commented Feb 15, 2021

sgillies commented Feb 15, 2021

sgillies Feb 18, 2021

Choose a reason for hiding this comment

sgillies Feb 18, 2021

Choose a reason for hiding this comment

sgillies Feb 18, 2021

Choose a reason for hiding this comment

sgillies Feb 18, 2021

Choose a reason for hiding this comment

sgillies left a comment • edited Loading

Choose a reason for hiding this comment

sgillies left a comment •

edited

Loading