pypa / installer

A low-level library for installing from a Python wheel distribution.
https://installer.readthedocs.io/
MIT License
123 stars 51 forks source link

Treat an empty destination the same as an unspecified one #188

Open eli-schwartz opened 1 year ago

eli-schwartz commented 1 year ago

With a command line such as:

python -m installer --destdir="$DESTDIR" dist/*.whl

The destdir may not be "set" but is always passed. When relocating paths relative to the destination directory, the anchor is chopped off, resulting in paths that are relative to the process-wide current working directory, and are installed relative to there. Avoid doing any relocations against an empty destdir.

Fixes #136

uranusjr commented 1 year ago

Personally I’d prefer this to install to the current directory; that is IMO how most commands operate. (And ultimately this is suboptimal bash programming; you should always check whether a variable is set or assume it can be empty.)

eli-schwartz commented 1 year ago

Can you refer me to some other commands that use staging-root relocations and have the behavior you imply? Because off the top of my head all I can come up with is a long list of software that treats the semantics of a blank staging-root relocation as equivalent to unspecified, including but not limited to the one I maintain.

eli-schwartz commented 1 year ago

Not entirely related, but...

Keep in mind that this is a python-installer implementation of the GNU Coding Standards, which uses a standardized environment variable and environment variables are an additional layer of fun implied defaults in cross-domain tooling. Even if you don't directly read that env variable, other tooling that invokes this tool does, and has to handle both unset and set-but-blank variables.

This leads a bit into your remark about "suboptimal bash programming"...

Bash is not python and shouldn't be treated as such.

In python, a variable either is or isn't set -- in bash, a variable that isn't set, is still set anyway if it's available in os.environ, and furthermore the entire language is designed from the ground up to make heavy use of the optionality of setting variables before you refer to them. Clearing a variable is most ergonomically done by setting its value to the empty string, especially when you want to override an environment variable in a subprocess. Even checking if a variable is set cannot be portably done without a) using sentinel values, and b) passing value-or-sentinel to a subprocess (/usr/bin/test), then checking whether the subprocess raises an error. The subprocess can be optimized away using the same "as-if" rule as C++, and most (but not all) implementations do optimize it.

It's very similar to the way bash is designed from the ground up to use "exceptions" as the native control flow for if/and/while and other language intrinsics -- a command that catastrophically fails with an internal error returns 1, and so does a language intrinsic that is testing the truthiness of a factoid return 1 for "False".

There are entire books that can be written about why neither erroring commands nor unset variables can be safely assumed to mean anything in the general case. People who write a lot of shell scripts often have to explain to people who don't, why bash can't be treated like other programming languages -- perhaps the term "like good programming languages" can be used -- so this is a common point of confusion. Quite frankly, you have to be an expert to write safe bash, which is not a problem that other languages have. But if you're interested, I suggest https://mywiki.wooledge.org as a primer.

uranusjr commented 1 year ago

I think you’re missing my point. In most POSIX tools, an empty string means the current directly, so --destdir="" should behave the same as --destdir=".". I don’t think I recall any tool that treats passing in an empty string as path as the same as not passing a path at all, as you are proposing.

Also, off topic about the suboptimal Bash, if you search “bash best practice” anywhere, most people would tell you to do set -u.

pradyunsg commented 1 year ago

Personally I’d prefer this to install to the current directory

That's the current behaviour, FWIW.