Treat empty environment variables as unset

[djc]

Fixes #4419.

[rami3l]

I'm not quite sure if this is the optimal approach... at least it isn't aware of the existence of non_empty_env_var() as @FranciscoTGouveia has noticed in FranciscoTGouveia@43f8115.

It might indeed be helpful to override the env var fetching behavior in all places, but some sort of merging would be better for future maintenance, and some more investigation/refactoring might be required to change something so global and fundamental in rustup's current behavior. As such, I am hesitant to merge this PR as-is.

[djc]

I did indeed fail to notice/forgot about the existence of non_empty_env_var(), but I strongly feel that it is better to try to tackle this issue once and for all compared to finding out later we have similar issues with other environment variables. We could try to audit whether this makes sense everywhere, but I think using an empty value to unset values is pretty common usage.

[rami3l]

I did indeed fail to notice/forgot about the existence of non_empty_env_var(), but I strongly feel that it is better to try to tackle this issue once and for all compared to finding out later we have similar issues with other environment variables. We could try to audit whether this makes sense everywhere, but I think using an empty value to unset values is pretty common usage.

@djc You are probably right; but before reaching the conclusion I'd like to take some time to further investigate into the use cases of non_empty_env_var() across the repo; maybe your fix has subsumed them all?

[rami3l]

@djc Sorry for the long wait, but here is what I have found after my investigation:

non_empty_env_var() has only been used four times in this repo:

1. RUSTUP_TOOLCHAIN for the active toolchain override
2. RUSTUP_DIST_SERVER for the distro server override
3. RUSTUP_DIST_ROOT for the legacy distro server override
4. RUSTUP_VERSION for overriding the "latest" version in self-updates

... and the current env vars being used in this repo can go to several categories:

1. URI or FS overrides, e.g. RUSTUP_DIST_ROOT. I don't think this new behavior will be harmful in this case nor should it cause common breakages.
2. Enumerations, e.g. RUSTUP_TERM_COLOR. Same reasoning as above.
3. Integers and booleans, e.g. RUSTUP_IO_THREADS, RUSTUP_RECURSION_COUNT, NO_COLOR. Often we won't want to set these variables to empty, so the current change sounds acceptable.
4. External env vars, including lib (Windows), DYLD_FALLBACK_LIBRARY_PATH (Mac), CARGO, PATH, MANPATH, HOME, SHELL, ZDOTDIR, XDG_CONFIG_HOME. I think this category is the one that needs the most attention, since it's not immediately clear if setting those to empty values has specific semantics.

My current standpoint is that, if we can figure out the 4. above and conclude that it is not violating with what we would like to achieve, we can fully eliminate non_empty_env_var() and close #4424 in favor of this PR.

[rami3l]

@djc I am really sorry for the belated response! I've done a full audit of all the env variable uses in the final section of my previous response and concluded that Rustup already assumes these env variables to be non-empty if present to be able to proceed. So this should be a safe move as far as my investigation goes 🙏

[djc]

Thanks for going through this! I wanted to spend time on it as well but haven't been able to yet. I'll follow up clean up the non_empty_env_var() stuff after this merges.