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

No tests for .py GDAL utilities? #834

Closed
plimkilde opened this issue Nov 7, 2023 · 15 comments · Fixed by #934
Closed

No tests for .py GDAL utilities? #834

plimkilde opened this issue Nov 7, 2023 · 15 comments · Fixed by #934
Labels

Comments

@plimkilde
Copy link
Contributor

Comment:

It seems there are no tests in this feedstock for the GDAL apps implemented in Python (gdal_calc.py, gdal_fillnodata.py etc.). On Windows in particular, implementation of Python commands can be finicky - currently, calling these utilities relies on .py file association, which can be messy with multiple environments. In contrast, pip installation of an entry point creates a small .exe file in the environment, which does not cause issues if .py files are associated with an editor or similar.

What would be needed to add tests for the Python utilities - simply adding gdal_calc.py --help etc. to run_test.bat/run_test.sh?

And would it be acceptable to use a pip-based install instead?

@gillins
Copy link
Contributor

gillins commented Nov 7, 2023

Indeed finicky on Windows! Regarding entry points, I haven't had much luck creating entry points some something with an extension already (ie gdal_calc.py, creating a gdal_calc.py.exe doesn't work - Windows doesn't search for a .exe if already has an extension last time I checked). Dropping the .py means we'll be out of step with the documentation (and Unix).
But yes, we should test that these run in run_test.bat/run_test.sh - a PR would be most welcome.

@rouault are there any plans to move to an entry point install for these scripts?

@rouault
Copy link
Contributor

rouault commented Nov 7, 2023

@rouault are there any plans to move to an entry point install for these scripts?

hum Python setup.py magic isn't totally an area I'm familiar with. May I ask @idanmiara if he sees some counter-indication in adding a entry_points keyword in https://github.com/OSGeo/gdal/blob/master/swig/python/setup.py.in while there is one in https://github.com/OSGeo/gdal/blob/master/swig/python/gdal-utils/setup.py ? I'm afaid that the GDAL Python bindings and the gdal-utils package would then collide regarding the wrapper scripts... ?

@gillins
Copy link
Contributor

gillins commented Nov 8, 2023

Thanks @rouault, isn't there quite a bit of collision between GDAL Python and gdal-utils anyway? Happy to defer to you and @idanmiara but moving to entry points for GDAL Python seems like a good idea (while keeping the old scripts around for a bit). This would definitely fix the unusual behaviour on Windows. I can do a PR if you like.

@idanmiara
Copy link

Back then I've created the batch wrapper creator which was mostly superseded by entry_points and console_scripts by @maphew. see:
OSGeo/gdal#5281
OSGeo/gdal#5296
OSGeo/gdal#5282

@rouault I'm not familiar with python.py.in file (and somewhat familiar with console scripts and entry points), but regarding package colliding between gdal and gdal-utils:
If one chooses to install both packages (as to override the utils with a different version of gdal-utils),
gdal is installed first (as it is a requirement for gdal-utils), then AFAIK pip would just override with the files in gdal-utils.
So if both packages install the console scripts, then I assume that the version in gdal-utils would override the version that was installed with gdal.

@maphew
Copy link

maphew commented Nov 10, 2023

Thanks for tagging me Idan. :) Gdal-utils console_scripts has been on my mind. I've been reading this week that Python 3.12 introduces changes with handling setup.py that are giving some folks a hard time. I've wondered if this is one of them.

Re: collision: Yes, as I recall the last installed wins. I don't think this introduces new problems though, since as noted above there already is a fair bit of intermingling. My back of brain tingling has vaguely lit thoughts/memories of discussion on moving the gdal python scripts into gdal-utils. Maybe it's time to look at that? I'm interested in helping, but won't be able to commit much attention for a couple weeks.

@jratike80
Copy link

OSGeo4W creates .bat files which look like this (gdal_calc.bat)

@echo off
call "%OSGEO4W_ROOT%\bin\o4w_env.bat"
python -u "%OSGEO4W_ROOT%\apps\Python39\Scripts\gdal_calc.py" %*

The .bat files hide the difference between .exe and .py tools from the OSGeo4W users but I can't say if there is anything re-usable in that idea.

@idanmiara
Copy link

The .bat files hide the difference between .exe and .py tools from the OSGeo4W users but I can't say if there is anything re-usable in that idea.

I've implemented a similar generalized approach (without the OSGeo4W specific stuff) for all the gdal scripts in
https://github.com/OSGeo/gdal/blob/master/swig/python/gdal-utils/osgeo_utils/auxiliary/batch_creator.py

@rouault
Copy link
Contributor

rouault commented Nov 15, 2023

attempt at fixing that in OSGeo/gdal#8718 . Apparently on Windows setuptools create .exe launchers

@maphew
Copy link

maphew commented Nov 15, 2023

yes that's right about the exe launchers, and the result looks good from here to me.

Pathlib includes glob, so getting the scripts list could be slightly more concise with Path(scripts_dir).glob("*.py"). Not a functional change though so shrug.

@rouault
Copy link
Contributor

rouault commented Nov 22, 2023

Note that now that OSGeo/gdal#8774 has been merged in the 3.8 branch, there are no longer .py scripts installed on Windows, but .exe launchers.
Cf https://github.com/OSGeo/gdal/actions/runs/6956258939/job/18926737430 log:

2023-11-22T11:27:52.9486555Z Deleting C:\Miniconda3\envs\test\conda-bld\gdal-split_1700650594769\_h_env\Scripts\gdal2tiles.py
2023-11-22T11:27:52.9489765Z Installing gdal2tiles-script.py script to C:\Miniconda3\envs\test\conda-bld\gdal-split_1700650594769\_h_env\Scripts
2023-11-22T11:27:52.9497424Z Installing gdal2tiles.exe script to C:\Miniconda3\envs\test\conda-bld\gdal-split_1700650594769\_h_env\Scripts

Consequently meta.yaml will have to be modified to remove the .py extension in the "Check Python-implemented GDAL utilities" section

@gillins
Copy link
Contributor

gillins commented Nov 22, 2023

Thanks @rouault this is going to be a huge improvement in usability for Windows users!

@rouault
Copy link
Contributor

rouault commented Nov 24, 2023

Thanks @rouault this is going to be a huge improvement in usability for Windows users!

unfortunately I've had to revert that change for 3.8.1RC2 as it was found to break stuff. Cf https://lists.osgeo.org/pipermail/gdal-dev/2023-November/057985.html . I'll guess someone will have to figure out how to have both .py scripts and .exe/.sh launchers at the same time... It doesn't appear the standard mechanism of using entry_points.console_scripts of setuptools allows for that.

@rouault
Copy link
Contributor

rouault commented Nov 24, 2023

unfortunately I've had to revert that change for 3.8.1RC2 as it was found to break stuf

arg, it was partly a false alarm: https://lists.osgeo.org/pipermail/gdal-dev/2023-November/057992.html. Anyway re-enabling that will likely be for 3.9.0

@rouault
Copy link
Contributor

rouault commented Nov 25, 2023

ok, this will be fixed in 3.9.0 per OSGeo/gdal#8815 + OSGeo/gdal#8824 . The last one was tricky. For some reason "python setup.py install" as used in this feedstock's install_python.bat uses the internal easy_install submodule of setuptools that had the side effect of removing the .py scripts when it installed the .exe wrappers ! I had to resort to ugly monkey patching of setuptools to avoid that, so users can still use the .py scripts directly if they rely on that...

@gillins
Copy link
Contributor

gillins commented Nov 26, 2023

Yikes!

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 a pull request may close this issue.

6 participants