v1.21.0 Broke Relative FFMPEG_PATH

[thenewguy]

My deployment places a wrapper script for ffmpeg on the path. The actual placement of the file is not known in advance so the FFMPEG_PATH var cannot be set absolute.

This seems to be the offending line

After narrowing down the changeset I've discovered No file exists at ... logs in the CI runner output.

Are you opposed to using something like ffmpeg_exe = which(os.environ['FFMPEG_PATH'])? Or perhaps another way of running the file check that doesn't break for relative exe paths that do actually work

[slhck]

Hm, I guess I could wrap it in an abspath call, or remove it again. I'll check later!

[thenewguy]

For background, the script is installed using the setup.py option scripts. So it is placed on the path in a virtualenv and available using something like wrapper.py. When originally implemented, it was not straightforward how one would go about determining the absolute path placement. I am not sure if running abspath(os.environ['FFMPEG_PATH']) would work in this case. But perhaps some sort of test that attempts to use the command or expand based on the PATH variable would be suitable (if the path is not absolute)

[slhck]

In principle, PATH variables should be set absolutely, anything else is bound to break at some point.

That said, can you please show me a concrete example of how you are setting FFMPEG_PATH?

Because this works fine (setting a relative path):

FFMPEG_PATH=./test/ffmpeg python3 -m ffmpeg_normalize test/test.mp4 -n

The path of course has to be correct with respect to your current working directory.

[slhck]

If os.path.isfile(os.environ['FFMPEG_PATH']) fails, then I am not sure how a command run against that path later on could succeed. So it makes sense for me to keep that check in there. If you have a solution that works for your use case and also doesn't break current behavior, please feel free to send a PR!

[thenewguy]

It is set env['FFMPEG_PATH'] = 'limiter.py' which is installed on the path by setup.py. Before this release it worked

[thenewguy]

Here is the documentation for the particular installation approach: https://python-packaging.readthedocs.io/en/latest/command-line-scripts.html#the-scripts-keyword-argument

[thenewguy]

Anyways, if you want to require FFMPEG_PATH to be an absolute path I can find another way to work around the issue. It just seems like an unnecessary requirement. I thought it was unintentional since it previously worked

[slhck]

I can remove the check if that makes it easier. But I still don't quite get how it can find the file while executing a shell command, when it can't locate the file before.

[thenewguy]

@slhck If I remember correctly, Popen (used here) uses os.execvp and will lookup commands that do not contain slashes against the configured $PATH

[slhck]

I will remove the check and let you know when I release a new version later today.

[thenewguy]

If you wanted to handle the cases separately and still raise the error when appropriate you could test os.sep in ffmpeg_exe

[thenewguy]

Like this: https://github.com/slhck/ffmpeg-normalize/pull/148

Happy to write a test and proper PR if you like

[slhck]

I would appreciate a PR!

No need to write a test if it's too complicated. I'm going to revamp the test suite soon, it's too limited in terms of what it does.

[thenewguy]

Ok - I will adjust my PR and run it through integration tests to confirm

[slhck]

Released a fix in 1.21.1!