Entry points for gdal-utils package

[maphew]

What does this PR do?

this PR supercedes #5282, in which I couldn't figure out how to cleanly rebase so I created this new branch and cherry picked the relevant commit.

This PR modifies setup.py so that when pip install gdal-utils is run all of the .py files in ./scripts are turned into native platform executable scripts that call the .py file using the appropriate interpreter. Although developed to improve the experience on Windows the implementation is cross platform (due to upstream magic by setuptools and/or distutils) and also is better for linux (c.f. https://click.palletsprojects.com/en/8.0.x/setuptools/?highlight=entry_points).

Benefits for the user is that after install the gdal python utility scripts are available from the command line immediately, just like all the first class binary utiltiies, e.g. gdalinfo, gdal_translate, and without needing to use .py extension with the name. Gdal edit is just gdal_edit.

What are related issues/pull requests?

gdal-utils: improve command line in Windows and ditch .bat wrappers #5281

Tasklist

updated Sun 27 Mar 2022 11:55:11 PM UTC

• implement and test locally
• fix gdal_merge (_"TypeError: not a sequence")
• Add test case(s)
  • Insert a new step after the "Standalone Python bindings build " step in .github/workflows/cmake_builds.yml which tests the "gdal" package of swig/python/setup.py
  • testing gdal2tiles multiprocessing on Windows with python >= 3.8 to make sure the issue fixed/workarounded by 0bdb2c8 doesn't reappear.
  • Add tests to ensure both modules AND scripts work correctly (ref
• Add documentation
  • add a note that people who make binary distributions for gdal should also modify their setup file accordingly.
• Review
• Adjust for comments
  • put auxiliary functions (define_entry_points) in a separate module so it can be imported from both setup files? No. It's really short function so the overhead of pulling it out isn't warranted. Solve with comments.
  • we still see from distutils.errors import CompileError in has_flag and also in some comments. Can this be dealt with here? -- Fixed with Python bindings: remove all traces of distutils #5297
• All CI builds and checks have passed

Environment

• OS: Microsoft Windows [Version 10.0.19042.804]
• Compiler: n/a

[rouault]

I've tried the following on Ubuntu 20.04 with your patch applied:

• create the wheel with: python setup.py bdist_wheel (in swig/python/gdal-utils)
• create a venv and activate it
• PATH=/home/even/gdal/install/bin:$PATH pip install swig/python/gdal-utils/dist/gdal_utils-3.3.0.7-py3-none-any.whl
  and I get the following error:

ERROR: Invalid script entry point: <ExportEntry gdal2tiles = scripts.gdal2tiles:None []> for req: gdal-utils==3.3.0.7 - A callable suffix is required. Cf https://packaging.python.org/specifications/entry-points/#use-for-scripts for more information.

The same without the patch leads to successful installation.

Another point to consider is that the main "gdal" package (swig/python/scripts) also provides the scripts, so if we modify the way of installing them in "gdal-utils" we must also do the same way in "gdal" so that everything overwrites itself (@idanmiara do you confirm ?)

Regarding CI testing of gdal_utils, there's none currently. I guess a new step after the "Standalone Python bindings build " step in

gdal/.github/workflows/cmake_builds.yml

Line 91 in 3d77dd2

- name: Standalone Python bindings build

(which tests the "gdal" package of swig/python/setup.py) would be needed

[maphew]

Interesting that the ubuntu failure is on only one of the scipts, gdal2tiles. Thanks for pointing out the other (and primary!) means of installing the scripts. I'll study and see if I see a path for not causing problems.

[rouault]

Interesting that the ubuntu failure is on only one of the scipts, gdal2tiles.

maybe it just stops at the first encountered issue ? (just assuming)

[idanmiara]

Another point to consider is that the main "gdal" package (swig/python/scripts) also provides the scripts, so if we modify the way of installing them in "gdal-utils" we must also do the same way in "gdal" so that everything overwrites itself (@idanmiara do you confirm ?)

Yes, I think that once we can confirm that it works as expected we should apply the same logic to:
https://github.com/OSGeo/gdal/blob/master/swig/python/setup.py
I'm not sure if it would make more sense to put auxiliary functions (define_entry_points) in a separate module so it can be imported from both setup files.

We should also put a note that people who make binary distributions for gdal should also modify their setup file accordingly.

Since it's a setuptools functionality - I remember that we removed support for distutils in gdal setup, but I can still see from distutils.errors import CompileError in has_flag and also in some comments, maybe this should be modified?

[idanmiara]

ERROR: Invalid script entry point: <ExportEntry gdal2tiles = scripts.gdal2tiles:None []> for req: gdal-utils==3.3.0.7 - A callable suffix is required. Cf https://packaging.python.org/specifications/entry-points/#use-for-scripts for more information.

I didn't test this but I have an assumption based on the error message:

Maybe it has something to do with that in the entry points no function name was defined, i.e. :main.
I think I've added in all the scripts a main function that the script calls:

if __name__ == '__main__':
    sys.exit(main(sys.argv))

If I'm understanding correctly the explanation here:
https://packaging.python.org/en/latest/specifications/entry-points/#use-for-scripts
Maybe we should modify each script's main function to accept argv=None as follows:

def main(argv=None):
    if argv is None:
        argv = sys.argv
    ...

[rouault]

Depending on what the wrapper script generates as code, testing gdal2tiles multiprocessing on Windows with python >= 3.8 would be advised to make sure the issue fixed/workarounded by 0bdb2c8 doesn't reappear.

[maphew]

I've no comment on how to change or adapt main. I'm not familiar with those forms and their reasons (yet).

From what I see at present gdal2tiles is the only script that uses if __name__ == '__main__':, while all have sys.exit(main(sys.argv)). There's some comments in there about multiprocessing I've going to study.

# Running main() must be protected that way due to use of multiprocessing on Windows:
# https://docs.python.org/3/library/multiprocessing.html#the-spawn-and-forkserver-start-methods
if __name__ == '__main__':
    # Trick inspired from https://stackoverflow.com/questions/45720153/python-multiprocessing-error-attributeerror-module-main-has-no-attribute
    # and https://bugs.python.org/issue42949
    __spec__ = None
    deprecation_warn('gdal2tiles')
    sys.exit(main(sys.argv))
~~`

[maphew]

In c3d0105 I moved the __spec__ = None block into the osgeo/gdal2tiles.py script instead of the to be deprecated wrapper to see if that would help.

I tried using the recipe in #5296 (comment). It failed but at a different point and with "error: Microsoft Visual C++ 14.0 or greater is required." so someone with a linux machine will have to repeat to see if this change does anything useful.

[idanmiara]

In case gdal2tiles handling is further complicate things, and as gdal2tiles is the only multiprocessing script AFAIK and thus need special care, I would suggest to skip it in this PR (by explicitly exclude it in the setup) and maybe to tackle it in a dedicated PR.

…

On Wed, 16 Feb 2022, 22:35 matt wilkie, ***@***.***> wrote: In c3d0105 <c3d0105> I moved the __spec__ = None block into the osgeo/gdal2tiles.py script instead of the to be deprecated wrapper to see if that would help. I tried using the recipe in #5296 (comment) <#5296 (comment)>. It failed but at a different point and with *"error: Microsoft Visual C++ 14.0 or greater is required."* so someone with a linux machine will have to repeat to see if this change does anything useful. — Reply to this email directly, view it on GitHub <#5296 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGJBBLNEYOZZV2YCIUNVYFDU3QDARANCNFSM5OFDFL6A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[jcphill]

Just a heads-up that I just made gdal2tiles startup more complex by adding mpi4py support.

[jcphill]

ERROR: Invalid script entry point: <ExportEntry gdal2tiles = scripts.gdal2tiles:None []> for req: gdal-utils==3.3.0.7 - A callable suffix is required. Cf https://packaging.python.org/specifications/entry-points/#use-for-scripts for more information.

I suspect that you're seeing this error on gdal2tiles merely because gdal2tiles.py is the first file in the directory, that you would see the same error for the other files, and that the real problem is that it doesn't like entry points that are just a module name without :function at the end (even though these are valid according to the spec).

[maphew]

both those ideas can be tested by excluding gdal2tiles from console_scripts, so I'll do that and... we'll see!

[jcphill]

Just so I understand, why do these wrapper scripts import * and issue a deprecation warning? Are the wrapper scripts supposed to be loadable as modules?

[maphew]

Regardless of 2f2c688 outcome, courtesy of @jcphill qyestion I'm now thinking that using ./scripts to drive console_scripts isn't a good idea. Scripts is deprecated and (someday) will be going away. Since this project is a new thing it's better to be forward looking and work from ./osgeo_utils instead. That will be my next focus.

[idanmiara]

It imports * to be backwards compatible with old dependent scripts that were written before gdal-utils was introduced. People then added the scripts dir to the path and imported the script as a module. The depreciation warning is to draw their attention for the much more straight forward way of importing it.

…

On Fri, 18 Feb 2022, 16:14 Jim Phillips, ***@***.***> wrote: Just so I understand, why do these wrapper scripts import * and issue a deprecation warning? Are the wrapper scripts supposed to be loadable as modules? — Reply to this email directly, view it on GitHub <#5296 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGJBBLJMED53BDXJDPTWMO3U3ZH2TANCNFSM5OFDFL6A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[jcphill]

It imports * to be backwards compatible with old dependent scripts that were written before gdal-utils was introduced. People then added the scripts dir to the path and imported the script as a module. The depreciation warning is to draw their attention for the much more straight forward way of importing it.

Since almost all of the wrapper scripts don't check if name == 'main' before calling sys.exit(main(sys.argv)) I don't think you can import them as modules without main and exit being called.

[idanmiara]

Hmm... Good point. That was the original intention, but since nobody complained about it till now I'm wondering if to add that if name=main or just to import main() as if anyone cared about that they probably already fixed their import.

…

On Fri, 18 Feb 2022, 22:17 Jim Phillips, ***@***.***> wrote: It imports * to be backwards compatible with old dependent scripts that were written before gdal-utils was introduced. People then added the scripts dir to the path and imported the script as a module. The depreciation warning is to draw their attention for the much more straight forward way of importing it. Since almost all of the wrapper scripts don't check if *name* == '*main*' before calling sys.exit(main(sys.argv)) I don't think you can import them as modules without main and exit being called. — Reply to this email directly, view it on GitHub <#5296 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGJBBLJAATTBYSWWZ2CRDPLU32SMVANCNFSM5OFDFL6A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[maphew]

Sorry to only catch this at the last minute, buy does gdal2tiles.py also need main() to have sys.argv as its default argument?

@jcphill The short answer is yes, I think so. The main thing is that console_scripts requires main() to be called with no arguments, even though in practice it actually does pass arguments. I solved this in the other scripts by making argv optional:

def main(argv=sys.argv):
    return gdal_edit(argv)

if __name__ == '__main__':
    sys.exit(main(sys.argv))

However I've never seen the construct def main(argv: List[str]) -> int: gdal2tiles is using before and I don't understand what it's doing. So there might be something else that works too, or work better.

Sorry to only catch this at the last minute,

I'm not!! The question revealed a significant problem in how I've been testing. pytest pyscripts/gdal2tiles gives a glowing 100% success while the simple gdal2tiles on the command line fails with TypeError: main() missing 1 required positional argument: 'argv'. I've been proving the gdal-util modules are still working after this change but not that the scripts are. Eeep.

[jcphill]

Those are function annotations (https://peps.python.org/pep-3107/). Based on some web searches I think you would specify the default argument after the type annotation, i.e.:
def main(argv: List[str] = sys.argv) -> int:

[maphew]

@jcphill that seems to work. Commit 2b52658 and testing on Ubuntu 20 via Gitpod is successful for me.

Testing command line util arguments are passed:

gitpod /workspace/gdal (pr-5281-redux) $ pip install swig/python/gdal-utils
# ...snip...
gitpod /workspace/gdal (pr-5281-redux) $ gdal2tiles in out
Usage: gdal2tiles.py [options] input_file [output]

gdal2tiles.py: error: The provided input file in does not exist or is not a file

Autotest suite:

cd autotest
pytest pyscripts/test_gdal2tiles.py
# 14 passed
pytest pyscripts/gdal2tiles
# 41 passed

Also thanks for the study link :)

edited to insert pip install ... step

[maphew]

IThe question revealed a significant problem in how I've been testing. pytest pyscripts/gdal2tiles gives a glowing 100% success while the simple gdal2tiles on the command line fails with TypeError: main() missing 1 required positional argument: 'argv'. I've been proving the gdal-util modules are still working after this change but not that the scripts are. Eeep.

@idanmiara, @rouault I think I've built a test structure that can address the problem Jim found with gdal2tiles and can be applied to all the gdal-utils scripts, see autotest/pyscripts/test_gdal_utils_cli.py. This my first time writing pytest tests, so a close look and feedback is warranted. ;-) I followed the header of format of other scripts regarding license declaration, copyright etc. I'm not set on this (I know some projects need copyright assignment to the primary maintainer).

[maphew]

Something in the pip install process breaks gdal_merge:

» gdal_merge
Traceback (most recent call last):
  File "C:\tools\miniconda3\envs\gdal\lib\runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "C:\tools\miniconda3\envs\gdal\lib\runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "C:\tools\miniconda3\envs\gdal\Scripts\gdal_merge.exe\__main__.py", line 7, in <module>
  File "C:\tools\miniconda3\envs\gdal\lib\site-packages\osgeo_utils\gdal_merge.py", line 323, in main
    argv = gdal.GeneralCmdLineProcessor(argv)
  File "C:\tools\miniconda3\envs\gdal\lib\site-packages\osgeo\gdal.py", line 4596, in GeneralCmdLineProcessor
    return _gdal.GeneralCmdLineProcessor(*args)
TypeError: not a sequence

[maphew]

The gdal_merge issue was simple: it just hadn't been updated yet to use the pattern:

def main(argv=sys.argv):
    return gdal_merge(argv)

if __name__ == '__main__':
    sys.exit(main(sys.argv))

[maphew]

I believe everything has been resolved and the PR is ready for final review. (Take with salt. I've thought this before ! ;-). The two CI fails don't appear to be related to this work.

[rouault]

squashed and merged