treat empty NIX_STORE_DIR env vars as unset

[zimbatm]

Motivation

Fixes #6409

Actually written by @NinjaTrappeur

Context

See the above issue ^

We might want to expand the same change to the other env vars, IFF having empty env vars doesn't make sense to them.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

• agreed on idea
• agreed on implementation strategy
• tests, as appropriate
  • functional tests - tests/**.sh
  • unit tests - src/*/tests
  • integration tests - tests/nixos/*
• documentation in the manual
• code and comments are self-explanatory
• commit message explains why the change was made
• new feature or incompatible change: updated release notes

[edolstra]

I guess this could be expressed in terms of getEnv()? I.e. something like

    auto value = getEnv(key);
    return value == "" ? std::nullopt : value;

[Ericson2314]

This feels ad-hoc to me. What about --store '' for example? This seems like giving into postal's law making errors go away, but the hidden cost is more complexity and more possibility for issues to hide.

The other issue had a bad error message. I would simply make a nice error message on the store URL parse to reject the empty string.

[Ericson2314]

Oh the title is wrong this is about NIX_STORE_DIR; still we can make nicer error handling rather than silent accepting a dubious environment variables which problem indicate an issue elsewhere.

[zimbatm]

@Ericson2314 I agree with your assessment in general, it's better to report failures than to paper over them. Bor env vars specifically, I think most scripts already don't make the distinction between empty and unset env vars ([ -z "${MY_ENV:-}" ]). It's probably asking for trouble if we rely on that distinction.

[Ericson2314]

So I wouldn't mind if we did this for every environment variable --- at least it would be consistent. But for some of them it does matter, e.g. for NIX_PATH an empty one clears the default value.

[picnoir]

👍 Makes sense.

But for some of them it does matter, e.g. for NIX_PATH an empty one clears the default value.

I think this pattern mostly makes sense for the env variables sent to canonPath where we're 100% sure that a empty value will result in a runtime failure. In that case, it'd probably be better to rather return a nullopt upstream (+ a warning) to move on with a default value.

[Ericson2314]

Yeah I think it could be good to make a function that combines canonPath and getEnv use it everywhere possible, and then separately we can debate whether that function has a nice error or nice warning :)

[zimbatm]

^ is that better?

[Ericson2314]

Yeah it is better :)

[edolstra]

I would drop this warning. If empty vars are equivalent to the var not being set, there is no reason to complain about it.

[Profpatsch]

They are not equal on my system at least:

philip@shiki ~> env FOO="" printenv FOO

philip@shiki ~> env printenv FOO
philip@shiki ~ [1]>

one returns "" and the other status 1

[Profpatsch]

If I had something set to "", I’d be kind of confused if nix ignored it silently.

[zimbatm]

Anything else before I squash the commits?

[picnoir]

I personally think that silently ignoring configuration paths that have been explicitly set by the user will make debugging such a situation much harder.

An env variable that is defined, but empty is likely a code smell/downstream bug, I think warning the user about that makes sense.

[Edit]: this post is apparently unclear, apologies. I personally think we should either warn the user and move on with the default value, or fail and throw an error.

[Ericson2314]

I agree with @NinjaTrappeur that the warning is important for debugging, which is why I asked for it. @edolstra would you be OK if we added the warning after all?

I would prefer the error, but I accept the warning as a middle ground.

[Ericson2314]

@NinjaTrappeur Can you open a new PR for the warning? That will give us something to discuss.