Add tests for .py utilities #835

plimkilde · 2023-11-07T12:12:58Z

Checklist

Used a personal fork of the feedstock to propose changes
Bumped the build number (if the version is unchanged)
Reset the build number to 0 (if the version changed)
Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
Ensured the license file is being packaged.

This adds tests for the Python-based GDAL utilities (gdal_calc.py, gdal_fillnodata.py etc.), see #834.

Interestingly, local testing revealed spotty support for both --help and --version.

conda-forge-webservices · 2023-11-07T12:13:04Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

xylar · 2023-11-07T13:09:52Z

@plimkilde, run_test.sh and run_test.bat get run when building libgdal, not gdal so the python tools aren't available. I think you want to add these tests directly in meta.yaml here:
https://github.com/plimkilde/gdal-feedstock/blob/py-utility-tests/recipe/meta.yaml#L285-L286

      commands:
        - python run_test.py
        - rgb2pct.py --help
        - pct2rgb.py --help
        - gdalattachpct.py --help
...

This should work for all architectures.

This reverts commit f60d0ef.

recipe/meta.yaml

xylar · 2023-11-07T15:34:25Z

recipe/meta.yaml

+        - gdal_merge.py --version
+        - gdal2tiles.py --help
+        - gdal2xyz.py --help
+        - gdal_retile.py --version


Oh, dear. Seeing:

gdal_retile.py --version GDAL 3.7.3, released 2023/10/30

but then it seems like it's exiting with an error code.

will be addressed per OSGeo/gdal#8676

…layer_algebra return 0 error code when invoked with --version switch Fixes conda-forge/gdal-feedstock#835 (review)

xylar · 2023-11-08T08:52:08Z

Finally passing for Linux. Let's see what happens with OSX and Windows.

xylar · 2023-11-08T09:30:41Z

Aaargh! Windows testing is just saying:

Tests failed for gdal-3.7.3-py312hb5f3cee_3.conda - moving package to D:\bld\broken

but not telling us why. Anyone from @conda-forge/gdal have an suggestions for how to get more helpful output here? I really don't speak Windows.

plimkilde · 2023-11-08T10:17:15Z

Aaargh! Windows testing is just saying:
Tests failed for gdal-3.7.3-py312hb5f3cee_3.conda - moving package to D:\bld\broken
but not telling us why. Anyone from @conda-forge/gdal have an suggestions for how to get more helpful output here? I really don't speak Windows.

Hm, I don't see any of the Windows builds successfully running to completion. I'm wondering if we are checking for existence in the correct directory?

xylar · 2023-11-08T10:28:14Z

I'm wondering if we are checking for existence in the correct directory?

I believe so.

xylar · 2023-11-08T10:34:16Z

recipe/meta.yaml

+        - rgb2pct.py --help
+        - pct2rgb.py --help
+        # Neither --help nor --version seem to work
+        # - if not exist %PREFIX%\\Library\\bin\\gdalattachpct.py exit 1  # [win]


Maybe try this next:

Suggested change

# - if not exist %PREFIX%\\Library\\bin\\gdalattachpct.py exit 1 # [win]

# - if not exist %LIBRARY_BIN%\\gdalattachpct.py exit 1 # [win]

But I believe that's the same.

zklaus · 2023-11-08T10:36:23Z

If we need more info about how things are failing, we could put the tests in their own scripts, say test_python_scripts.{sh,bat} and add set -xe and @echo on, respectively.

xylar · 2023-11-08T10:39:58Z

@zklaus, that's a great suggestion.

gillins · 2023-11-08T10:57:59Z

I think on Windows, you'll need to run:

%PYTHON% %LIBRARY_BIN%\rgb2pct.py --help  # [win]

etc. It's probably popped up a dialog box wanting to know how to execute a .py file and that's why the build if failing (?). Windows doesn't have the shebang thing so needs to be told how to execute the file....

plimkilde · 2023-11-08T11:04:14Z

I think on Windows, you'll need to run:
%PYTHON% %LIBRARY_BIN%\rgb2pct.py --help  # [win]
etc. It's probably popped up a dialog box wanting to know how to execute a .py file and that's why the build if failing (?). Windows doesn't have the shebang thing so needs to be told how to execute the file....

Trouble is, then we're not testing the documented commands 😕

gillins · 2023-11-08T11:09:12Z

Trouble is, then we're not testing the documented commands 😕

What's documented has never worked on Windows as-is. This has been a source of much confusion when I've been helping Windows users. I've always had to tell people to prefix everything with python and then hardcode the path to the script (which then breaks when they change envs).
Entry points would be the way forward for cross platform (conda can generate these if asked: https://docs.conda.io/projects/conda-build/en/stable/resources/define-metadata.html#python-entry-points). But then we'd have to lose the extension, which again, wouldn't match the docs. Argghh.

plimkilde · 2023-11-08T11:38:11Z

Trouble is, then we're not testing the documented commands 😕

What's documented has never worked on Windows as-is. This has been a source of much confusion when I've been helping Windows users. I've always had to tell people to prefix everything with python and then hardcode the path to the script (which then breaks when they change envs). Entry points would be the way forward for cross platform (conda can generate these if asked: https://docs.conda.io/projects/conda-build/en/stable/resources/define-metadata.html#python-entry-points). But then we'd have to lose the extension, which again, wouldn't match the docs. Argghh.

I don't feel inclined to write a passing test for something that doesn't work, so maybe we should limit the scope of this PR to simply adding tests of what works (i.e. Unix), then enabling Windows testing in a later PR when a solution is ready.

conda-forge-webservices · 2023-11-08T12:43:27Z

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [323]

plimkilde · 2023-11-08T12:44:35Z

Hm, it seems to pass on Windows with existence checks disabled. As suggested above, I've tried to move them into a separate .bat file to have more details.

recipe/meta.yaml

Co-authored-by: Xylar Asay-Davis <[email protected]>

conda-forge-webservices · 2023-11-08T12:49:11Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

xylar · 2023-11-08T12:50:54Z

recipe/test_script_exist.bat

+REM script checks only for the existence of the .py files below.
+
+echo Test existence of gdalattachpct.py...
+if not exist %PREFIX%\\Library\\bin\\gdalattachpct.py exit 1


We could tidy up with %LIBRARY_BIN% but let's see if this works first.

That does not seem to make any difference

As I said, it's just tidier. If the file isn't one place, it isn't the other because they point the same place. If the test is failing, the file presumably really isn't installed there.

gillins · 2023-11-08T21:58:37Z

recipe/test_script_exist.bat

+@echo on
+
+echo Test existence of gdalattachpct.py...
+if not exist %LIBRARY_BIN%\\gdalattachpct.py exit 1


Single back slash should do it. You just need double in the yaml.

True, though it should work in practice in most cases.

plimkilde · 2023-11-09T13:14:55Z

It's passing! I believe this is about the best that can be done without changes to GDAL itself, so I'm marking this PR as ready.

rouault · 2023-11-09T13:18:36Z

recipe/test_script_exist.bat

@@ -0,0 +1,21 @@
+@echo off
+REM As a workaround for GDAL apps unexpectedly returning error codes, this


Note: GDAL 3.8.0RC2 has been issued with the fix for that issue

@rouault, is there also a plan to move from scripts to entry points to better support Windows?

@xylar I'm waiting for feedback. See #834 (comment)

Ah, sorry, I had noticed that comment and just forgot. Thanks!

xylar

@plimkilde, thanks for persisting through this one! Nice work!

gillins · 2023-11-13T05:15:18Z

Thanks @plimkilde !

plimkilde added 2 commits November 7, 2023 13:09

Add tests for .py utilities

f60d0ef

Bump build number

d6febed

Move calls to meta.yaml

0fc76fa

This reverts commit f60d0ef.

xylar reviewed Nov 7, 2023

View reviewed changes

recipe/meta.yaml Outdated Show resolved Hide resolved

Check only for existence of gdalattachpct.py

66838ce

xylar reviewed Nov 7, 2023

View reviewed changes

Check only for existence of gdal_retile.py

07a4791

rouault added a commit to rouault/gdal that referenced this pull request Nov 7, 2023

Python utilities: make gdal_edit, gdal_pansharp, gdal_retile and ogr_…

70ac331

…layer_algebra return 0 error code when invoked with --version switch Fixes conda-forge/gdal-feedstock#835 (review)

rouault mentioned this pull request Nov 7, 2023

Python utilities: make gdal_edit, gdal_pansharp, gdal_retile and ogr_layer_algebra… OSGeo/gdal#8676

Merged

Check only for existence of gdal_proximity.py

26181e3

rouault added a commit to rouault/gdal that referenced this pull request Nov 7, 2023

Python utilities: make gdal_edit, gdal_pansharp, gdal_retile and ogr_…

98eb5f6

…layer_algebra return 0 error code when invoked with --version switch Fixes conda-forge/gdal-feedstock#835 (review)

xylar added 4 commits November 7, 2023 13:07

Check only for existence of gdal_edit.py

19a9151

Fix comment

6657d28

Check only for existence of gdal_pansharpen.py

c2fe14c

Only test for existence of ogrmerge.py

3e23b5a

rouault added a commit to OSGeo/gdal that referenced this pull request Nov 8, 2023

Python utilities: make gdal_edit, gdal_pansharp, gdal_retile and ogr_…

2072770

…layer_algebra return 0 error code when invoked with --version switch Fixes conda-forge/gdal-feedstock#835 (review)

Check only for existence for ogr_layer_algebra.py

d36c34f

Try disabling existence checks on Windows

321e9b4

xylar reviewed Nov 8, 2023

View reviewed changes

plimkilde added 2 commits November 8, 2023 13:41

Re-enable existence checks in separate batch file

02242e4

Whoops, run .bat file only on Windows

6326991

xylar reviewed Nov 8, 2023

View reviewed changes

recipe/meta.yaml Outdated Show resolved Hide resolved

Update recipe/meta.yaml

8825f9b

Co-authored-by: Xylar Asay-Davis <[email protected]>

xylar reviewed Nov 8, 2023

View reviewed changes

Try existence check in %LIBRARY_BIN%

b283465

gillins reviewed Nov 8, 2023

View reviewed changes

plimkilde added 3 commits November 9, 2023 10:15

Use single backslashes in batch file paths

970ce6d

Try to print path to .py files in output

ff2cadd

Check in %PREFIX%\Scripts, clean up comments

c2963d3

plimkilde marked this pull request as ready for review November 9, 2023 13:14

plimkilde requested review from akrherz, kmuehlbauer, mingwandroid, msarahan, ocefpaf and zklaus as code owners November 9, 2023 13:14

rouault reviewed Nov 9, 2023

View reviewed changes

xylar approved these changes Nov 9, 2023

View reviewed changes

gillins merged commit 6cf10f7 into conda-forge:main Nov 13, 2023
7 checks passed

This was referenced Nov 15, 2023

Use proper calls for tests of fixed .py commands #853

Merged

Unexpected nonzero return code from .py utilities with --help or --version OSGeo/gdal#8717

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for .py utilities #835

Add tests for .py utilities #835

plimkilde commented Nov 7, 2023

conda-forge-webservices bot commented Nov 7, 2023

xylar commented Nov 7, 2023

xylar Nov 7, 2023

plimkilde Nov 7, 2023

rouault Nov 7, 2023

xylar commented Nov 8, 2023

xylar commented Nov 8, 2023

plimkilde commented Nov 8, 2023

xylar commented Nov 8, 2023

xylar Nov 8, 2023

xylar Nov 8, 2023

zklaus commented Nov 8, 2023

xylar commented Nov 8, 2023

gillins commented Nov 8, 2023

plimkilde commented Nov 8, 2023

gillins commented Nov 8, 2023

plimkilde commented Nov 8, 2023 •

edited

Loading

conda-forge-webservices bot commented Nov 8, 2023

plimkilde commented Nov 8, 2023

conda-forge-webservices bot commented Nov 8, 2023

xylar Nov 8, 2023

plimkilde Nov 8, 2023

xylar Nov 8, 2023

gillins Nov 8, 2023

plimkilde Nov 9, 2023

plimkilde commented Nov 9, 2023

rouault Nov 9, 2023

xylar Nov 9, 2023

rouault Nov 9, 2023

xylar Nov 9, 2023

xylar left a comment

gillins commented Nov 13, 2023

	# - if not exist %PREFIX%\\Library\\bin\\gdalattachpct.py exit 1 # [win]
	# - if not exist %LIBRARY_BIN%\\gdalattachpct.py exit 1 # [win]

		@@ -0,0 +1,21 @@
		@echo off
		REM As a workaround for GDAL apps unexpectedly returning error codes, this

Add tests for .py utilities #835

Add tests for .py utilities #835

Conversation

plimkilde commented Nov 7, 2023

conda-forge-webservices bot commented Nov 7, 2023

xylar commented Nov 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xylar commented Nov 8, 2023

xylar commented Nov 8, 2023

plimkilde commented Nov 8, 2023

xylar commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zklaus commented Nov 8, 2023

xylar commented Nov 8, 2023

gillins commented Nov 8, 2023

plimkilde commented Nov 8, 2023

gillins commented Nov 8, 2023

plimkilde commented Nov 8, 2023 • edited Loading

conda-forge-webservices bot commented Nov 8, 2023

plimkilde commented Nov 8, 2023

conda-forge-webservices bot commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plimkilde commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xylar left a comment

Choose a reason for hiding this comment

gillins commented Nov 13, 2023

plimkilde commented Nov 8, 2023 •

edited

Loading