When delocate is called with a non-expanded wildcard path it produces wrong result

[webknjaz]

Imagine you call python -m delocate.cmd.delocate_listdeps --all dist/*.whl in the env where dist/*.whl is not expanded by the shell so it's effectively python -m delocate.cmd.delocate_listdeps --all 'dist/*.whl'. And the dists dir has:

-rw-r--r--   1 runner  staff   144K Apr 28 08:30 ansible_pylibssh-0.0.1.dev1-cp38-cp38-macosx_10_9_x86_64.whl

It'll use dist/ansible_pylibssh-0.0.1.dev1-cp38-cp38-macosx_10_9_x86_64.whl as a source and will emit a wheel with the name dist/*.whl:

-rw-r--r--   1 runner  staff   1.4M Apr 28 08:30 *.whl
-rw-r--r--   1 runner  staff   144K Apr 28 08:30 ansible_pylibssh-0.0.1.dev1-cp38-cp38-macosx_10_9_x86_64.whl

which then makes twine check dist/*.whl explode: pypa/twine#612.

It looks like delocate applies glob in one place but not another which is a bug.

[webknjaz]

UPD: It's worse now — recently, delocate started erroring out early with that glob being passed. It worked a few weeks ago so the changes are related to v0.10.5 or v0.10.6. Though, skimming through the diffs, it's unobvious what exactly caused this.

[webknjaz]

I think, it's v0.10.5, after all: the last successful nightly build we had was on Nov 13 and since Nov 14, the macOS builds started failing. This correlates with the release date of v0.10.5 which happened on Nov 14, right in-between these CI cron runs.

[webknjaz]

I addressed this by expanding the wildcard outside the delocate invocation: ansible/pylibssh@025166d.

[HexDecimal]

@webknjaz What's the regression caused by v0.10.5? I mean what's the error output?

If the shell doesn't the expand the paths then delocate and probably most CLI's will take the path as-is. I don't see this as unexpected behavior but I don't think anyone would mind if delocate globbed the paths itself.

Should also add tests related to globbing.

[HexDecimal]

Looking into this more there are some edge cases which could be pretty devastating. Globs include character ranges inside of brackets [] which can create filenames which don't match themselves when globbed.

I'd highly recommend using the shell more effectively rather than trying to add a workaround in the delocate scripts. Once unexplanded globs are sent to the script it seems too late to make a clean and effective workaround. Trying to do so anyway may add shell-like problems into the script itself.

The problems caused by the workaround can be fixed with their own workarounds, but at this point I'd like to hear more opinions on this before I continue.

[webknjaz]

(UPD: fixed the commit link)

If the shell doesn't the expand the paths then delocate and probably most CLI's will take the path as-is. I don't see this as unexpected behavior but I don't think anyone would mind if delocate globbed the paths itself.

Yep. Although, it did work during the past 3 years. I only had to double-wrap it today. Since it's called from within tox, it's not as trivial as in a regular shell, though.

FWIW, it fails like this: https://github.com/ansible/pylibssh/actions/runs/7025841421/job/19117524211#step:10:61. It attempts to open a glob as string.

And I understand that it's usually better to get the external shell to expand stuff, but it's not always as straightforward when it's not called from shell. Wrapping a command with sh -c "..." when it's already a part of an INI file, that's parsed by configparser, then interpreted and post-processed by tox, is a can of worms by itself. It's ugly, to say the least: ansible/pylibssh@025166d#diff-ef2cef9f88b4fe09ca3082140e67f5ad34fb65fb6e228f119d3812261ae51449R214-R215.

Anyway, there are tools for this in the stdlib: pathlib.Path.glob() and glob.glob(). They exist to solve this issue specifically.

I'm not asking to interpret shell-specific syntax like "$(subcommand)". But simple globs supported by stdlib with asterisks are quite well-defined: it's fnmatch.fnmatch() + **.

[webknjaz]

FTR I do think that a fix on the tox level would be preferable: tox-dev/tox#1571. So this is not a call to implement it unconditionally. Some test coverage would indeed be nice for predictability in the future. But beyond that, probably wouldn't insist.

[HexDecimal]

I looked at #106 again. It does not affect the file path given to delocate-wheel. So I don't think it's the cause of the regression. The error from your log shows that the initial inconsistently is fixed as it tries to load *.whl instead of implicitly expanding it. This might not be what you wanted but at least it's a more predictable result.

Right now I can only guess that something in zipfile.ZipFile was silently expanding the path in an older version of Python. This is the only explanation I can come up with which explains why the output was still *.whl while the input was somehow expanded and resolved. My assumption is that delocate never globs the path itself since I can't find the code where that happens. Feel free to correct me since I couldn't find anything in the Python changelogs.

I'm not asking to interpret shell-specific syntax like "$(subcommand)". But simple globs supported by stdlib with asterisks are quite well-defined: it's fnmatch.fnmatch() + **.

The problem I see is those square bracket patterns. A poorly named file such as libfoo[x86].dylib will not match itself if globbed due to the square brackets. I also worry that other regressions from using glob this way might be more subtle. Using glob is not as simple as it seems. It's maybe okay if I check if a file exists on a path before running glob on it, but that's a workaround to a workaround.

I agree that it'd be better to fix this issue in Tox. Tox actually runs commands and should have some support for shell expansion. Though they seem to be having their own issues with their command syntax.

[webknjaz]

I looked at #106 again. It does not affect the file path given to delocate-wheel. So I don't think it's the cause of the regression.

Yes, that PR only appeared in v0.10.6. I posted above in this issue that the behavior changed in v0.10.5. I discovered this after posting the comment in the PR, but only clarified this in the issue comment 🤷‍♂️

[webknjaz]

I agree that it'd be better to fix this issue in Tox. Tox actually runs commands and should have some support for shell expansion. Though they seem to be having their own issues with their command syntax.

Yes, which is why I filed that linked issue there. Bernát is open to a feature, but I never got to making a PR..

[webknjaz]

Let's mark the issue as wontfix and make the expectations more clear in the docs/CLI.

[HexDecimal]

I've compared v0.10.5 to the previous version. The change in behavior comes from an update in the zip2dir function 5248eaa. The previous code called an unzip subprocess with the input file path as an argument (without shell=True). The new code uses Python's zipfile module.

I already have some code which will resolve this but I want to make sure it's reasonable before I make a PR.