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

Python38/Windows dll load directory #852

Closed
wants to merge 24 commits into from

Conversation

rbuffat
Copy link
Contributor

@rbuffat rbuffat commented Jan 10, 2020

A start towards fixing #851

This PR loads the GDAL_HOME/bin directory with add_dll_directory when fiona is imported.

We should probably think about adding other directories. Maybe somebody with experience providing wheels for Windows has some advice here @cgohlke @jorisvandenbossche

gdal_home = os.getenv('GDAL_HOME', None)

if gdal_home is not None and os.path.exists(gdal_home):
os.add_dll_directory(os.path.join(gdal_home, "bin"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If GDAL_HOME is set, but the directory does not exist, we should emit a warning

@rbuffat
Copy link
Contributor Author

rbuffat commented Jan 12, 2020

@cgohlke I tested this issue with Fiona‑1.8.13‑cp38‑cp38‑win32.whl from https://www.lfd.uci.edu/~gohlke/pythonlibs/#fiona.

Changing fiona/init.py from

# Unofficial Windows Binaries:
# Use gdal DLLs and data files from the osgeo package
try:
    import numpy
    import osgeo
except ImportError:
    pass

to

# Unofficial Windows Binaries:
# Use gdal DLLs and data files from the osgeo package
try:
    import osgeo
    if (3, 8) <= sys.version_info:
        for p in osgeo.__path__:
            os.add_dll_directory(p)
except ImportError:
    pass

fixed the issue for me. However, I think this is not a "standard" gdal location to look for directly in Fiona.

I removed numpy, because the code silently fails to import osgeo if it is not present.

@cgohlke
Copy link
Contributor

cgohlke commented Jan 12, 2020

issue with Fiona‑1.8.13‑cp38‑cp38‑win32.whl from https://www.lfd.uci.edu/~gohlke/pythonlibs/#fiona

Those binaries are compiled against GDAL from https://www.lfd.uci.edu/~gohlke/pythonlibs/#gdal. They are not meant to work with any other GDAL distribution. os.add_dll_directory() should not be required because import osgeo pre-loads the DLLs required by Fiona into the process.

@rbuffat
Copy link
Contributor Author

rbuffat commented Jan 12, 2020

@cgohlke yes, you are right. Sorry for that. My problem was, that I had numpy not installed.

I installed a fresh python 3.8, with gdal and fiona from your website. With this installation I got the following message:

Python 3.8.1 (tags/v3.8.1:1b293b6, Dec 18 2019, 22:39:24) [MSC v.1916 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import fiona
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Python38-32\lib\site-packages\fiona\__init__.py", line 87, in <module>
    from fiona.collection import BytesCollection, Collection
  File "C:\Python38-32\lib\site-packages\fiona\collection.py", line 9, in <module>
    from fiona.ogrext import Iterator, ItemsIterator, KeysIterator
ImportError: DLL load failed while importing ogrext: The specified module could not be found.

I wrongly assumed it is the same reason as in #851 due to the changes in Python 3.8 on Windows and the similar ImportError message. However, as I did not have numpy installed, the import of osgeo never happened.

@@ -77,6 +77,17 @@
class Path:
pass


# Add gdal dll directory on Windows and Python >= 3.8, see https://github.com/Toblerity/Fiona/issues/851
if platform.system() == 'Windows' and (3, 8) <= sys.version_info:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is add_dll_directory harmful when gdal dlls are already loaded? E.g. when multiple gdal binaries are present.

@cgohlke
Copy link
Contributor

cgohlke commented Jan 12, 2020

My problem was, that I had numpy not installed.

The issue is that the GDAL Python package does not list numpy as an install requirement when the array_module is enabled. https://github.com/OSGeo/gdal/blob/master/gdal/swig/python/setup.py

fiona/__init__.py Outdated Show resolved Hide resolved
fiona/__init__.py Outdated Show resolved Hide resolved

# Use GDAL_HOME if present
if dll_directory is not None:
gdal_home = os.getenv('GDAL_HOME', None)
Copy link
Member

Choose a reason for hiding this comment

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

This will need to be added to the documentation. I don't think it's a standard runtime environment variable for GDAL, although it does seem to be used in the build guide for Windows.

@snorfalorpagus
Copy link
Member

Should this be targetted at master or at a maint-* branch?

Copy link
Member

@snorfalorpagus snorfalorpagus left a comment

Choose a reason for hiding this comment

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

Minor changes requested inline.

fiona/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@snorfalorpagus snorfalorpagus left a comment

Choose a reason for hiding this comment

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

Some more changes requested inline.

fiona/__init__.py Outdated Show resolved Hide resolved

add_dll_directory_win()

import fiona.ogrext
Copy link
Member

Choose a reason for hiding this comment

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

There should only be one space here

fiona/__init__.py Outdated Show resolved Hide resolved
fiona/__init__.py Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
Comment on lines 82 to 84

import fiona.ogrext

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the line breaks around this are necessary.

@s7726
Copy link
Contributor

s7726 commented Apr 1, 2020

Currently hitting this. The GDAL installers put the executables and libraries in the GDAL directory, not under bin. So the add_dll_directory_win function is a little too strict.
Also once I manually add C:\Program Files\GDAL using os.add_dll_directory I run into another issue.

ImportError: DLL load failed while importing ogrext: The specified procedure could not be found.

Took awhile to hunt down this issue to solve the specified module problem. I could use some direction in how to hunt down the procedure aspect.

@rbuffat
Copy link
Contributor Author

rbuffat commented Apr 1, 2020

@s7726 I'm not sure what caused this error message. Maybe it helps to recompile Fiona and check that it compiles against the same gdal dlls.

@sgillies @snorfalorpagus @cgohlke Numpy has a separate init file for distributions: https://github.com/numpy/numpy/blob/master/numpy/_distributor_init.py
Should we add the same to Fiona? Especially on Windows, there is no standardized way to distribute GDAL binaries and we can't check every possible location. This could be a place to locate the code for distribution specific loading of GDAL.

@sgillies
Copy link
Member

sgillies commented Apr 2, 2020

@rbuffat thanks for taking the lead on this. I would prefer a more explicit approach.

Effectively, what we need to do on Windows/Python 3.8 is

with os.add_dll_directory(DIR):
    import extension_module

in every Python module that imports Fiona extension modules (_env, _geometry, etc). We could write a generic context manager that used the Windows function when required and did nothing in other cases.

with fiona._loading.add_dll_directories():  # or something like.
    import extension_module

What would you think about doing this for a 1.8 bug fix release?

@rbuffat
Copy link
Contributor Author

rbuffat commented Apr 3, 2020

@sgillies is that the direction you had in mind?

try:

    import fiona.ogrext
    import fiona._env

except ImportError as e:

    import fiona._loading

    with fiona._loading.add_gdal_dll_directories():

            import fiona.ogrext
            import fiona._env

could be simplified to

with fiona._loading.add_gdal_dll_directories():

    import fiona.ogrext
    import fiona._env

However, my concern here is that providers of wheels could implement other ways to load the required dlls independent on environment variables. Adding additional directories could lead to side effects. As far as I currently see the only way to know if the dlls are not available is when the import fails.

@rbuffat rbuffat changed the base branch from master to maint-1.8 April 4, 2020 15:50
@rbuffat rbuffat changed the base branch from maint-1.8 to master April 4, 2020 15:50
@rbuffat
Copy link
Contributor Author

rbuffat commented Apr 20, 2020

closed in favor of #875

@rbuffat rbuffat closed this Apr 20, 2020
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.

5 participants