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

Remove _loading, copy it into __init__ #2594

Merged
merged 5 commits into from
Sep 27, 2022
Merged

Remove _loading, copy it into __init__ #2594

merged 5 commits into from
Sep 27, 2022

Conversation

sgillies
Copy link
Member

Resolves #2593.

@sgillies sgillies changed the base branch from main to maint-1.3 September 26, 2022 16:47
sgillies added a commit to rasterio/rasterio-wheels that referenced this pull request Sep 26, 2022
That's the no _loading rasterio PR rasterio/rasterio#2594.
rasterio/__init__.py Outdated Show resolved Hide resolved
@sgillies sgillies marked this pull request as ready for review September 26, 2022 21:53
@sgillies
Copy link
Member Author

@snowman2 I'm testing this on Windows with a rasterio-wheels branch and seeing a couple unexpected failures involving PROJ after an upgrade to 9.1.0 https://github.com/rasterio/rasterio-wheels/actions/runs/3129647645/jobs/5081976996#step:7:237. I'm inclined to merge and then sort out the errors before we make wheels for 1.3.3.

@sgillies sgillies self-assigned this Sep 26, 2022
@sgillies sgillies added the bug label Sep 26, 2022
@sgillies sgillies merged commit 4b1e369 into maint-1.3 Sep 27, 2022
@sgillies sgillies deleted the issue2593 branch September 27, 2022 00:02
@sgillies sgillies restored the issue2593 branch September 27, 2022 00:03
@snowman2 snowman2 deleted the issue2593 branch September 27, 2022 01:15
@snowman2
Copy link
Member

Makes sense 👍

@snowman2 snowman2 added this to the 1.3.3 milestone Sep 27, 2022
@snowman2
Copy link
Member

seeing a couple unexpected failures involving PROJ after an upgrade to 9.1.0 https://github.com/rasterio/rasterio-wheels/actions/runs/3129647645/jobs/5081976996#step:7:237

Possibly related: #2592

@jorisvandenbossche
Copy link
Contributor

@sgillies I think you might still need to only add expose those DLLs while importing the rasterio extension modules. Because now you add them always when importing rasterio, which might then impact another module that would load GDAL? (like fiona)

@jorisvandenbossche
Copy link
Contributor

For example in pyogrio, we still do the extension module imports within a context manager that adds/removes the DLL directory: https://github.com/geopandas/pyogrio/blob/d1714041153416c0358658746a6b95f067c835a5/pyogrio/core.py#L5-L26 and https://github.com/geopandas/pyogrio/blob/d1714041153416c0358658746a6b95f067c835a5/pyogrio/_env.py#L50-L61

Although, now I am writing this, I realize that we do use delvewheel for pyogrio, which actually patches the __init__.py file inside windows wheels to always do this add_dll_directory. But maybe in the case of wheels this is OK because the included DLLs have a different name (a hash is added to them)?

@sgillies
Copy link
Member Author

@jorisvandenbossche @snowman2 delvewheel is patching rasterio and fiona (1.9 pre-releases) and mangling DLL names, too. I'm going to check to see if we can rely entirely on it, will add a test to rasterio-wheels that imports rasterio and fiona together.

sgillies added a commit to rasterio/rasterio-wheels that referenced this pull request Oct 11, 2022
* Test build of 34c0dd5

That's the no _loading rasterio PR rasterio/rasterio#2594.

* Set fetch depth

* Use branch name

* Update vcpkg.json

* Add vcpkg.json to hash key

* Update vcpkg.json

* Update win-wheels.yaml

* Add a baseline

* Add versions feature flag

* Build maint-1.3

* Update win-wheels.yaml

* Fix PROJ_LIB directory for PROJ 9.1+

* Try manifest feature flag and check against fiona

* vcpkg'ing before the checkout action

* Add debugging

* Use bash shell

* change directory before running vcpkg

* checkout step

* Remove baseline

* Add vcpkg as submodule

* Install vcpkg using cmd

* Get submodules

* Bootstrap vcpkg as separate step

* Remove platform tags from manifest

* Try adding a baseline and using builtin vcpkg

* Back to submodule vcpkg

* Use gitsha of vcpkg 2022.09.27

* Use vcpkg 2022.09.27

* GDAL 3.5.2 isn't available in a vcpkg release yet

* Override VCPKG_INSTALLATION_ROOT set by GHA

* Try GDAL 3.5.2 again

* One more override try

* Use default (powershell) more often

* Come on!

* Bootstrap from builtin vcpkg like pyproj does

* Run step with bash

* Suss out install directory changes

* Specify install root

* Try to find GDAL after installation

* add x-install-root

* Remove commands destined to fail

* Moar debugging

* manifests with an s

* Set manifest root, grasping at straws

* Use capital C in drive path

* Remove vcpkg from gitmodules

* Search for GDAL_DATA directory

* Add VCPKG_DEFAULT_TRIPLET

* fix PROJ_:LIB for PROJ 9.0.1

* Skip 2 tests on Windows

* Remove submodule directory

Co-authored-by: Alan D. Snow <alansnow21@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add DLL directory for Windows wheels once and only once
3 participants