Treat an empty destination the same as an unspecified one #188
Conversation
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 pypa#136
|
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.) |
|
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. |
|
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. |
|
I think you’re missing my point. In most POSIX tools, an empty string means the current directly, so Also, off topic about the suboptimal Bash, if you search “bash best practice” anywhere, most people would tell you to do |
| @@ -138,7 +138,7 @@ def __init__( | |||
|
|
|||
| def _path_with_destdir(self, scheme: Scheme, path: str) -> str: | |||
| file = os.path.join(self.scheme_dict[scheme], path) | |||
| if self.destdir is not None: | |||
| if self.destdir: | |||
There was a problem hiding this comment.
I don't think we should handle it like this in the Python side, and instead this should be a CLI-specific thing. I'd much prefer that the CLI pass None to these objects, rather than changing the behaviour of empty strings within the API here.
I'd also be OK with disallowing empty strings in the API, just to avoid this failure mode in downstream code as well.
There was a problem hiding this comment.
Also, I'd appreciate it if you could add tests for the relevant behaviour here as well. :)
That's the current behaviour, FWIW. |
With a command line such as:
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