Don't uppercase Windows environment variables

[db4]

Changelog: Omit
Docs: Omit

Fix: Don't uppercase Windows environment variables
Due to manipulations with os.environ Conan currently uppercases all Windows environment variables. I stumbled upon a situation where this breaks a Conan-run bash script. This PR should fix that.

[memsharded]

While I understand the intent, I am concerned about the usage of import nt

[memsharded]

I think this is mostly a private Python implementation detail, I am concerned about using this directly, as this is not public documented Python library: https://docs.python.org/3/library/

[db4]

I've found the recommendation here: https://stackoverflow.com/questions/19023238/why-python-uppercases-all-environment-variables-in-windows
nt module is also mentioned here: https://www.oreilly.com/library/view/python-standard-library/0596000960/ch12s12.html

[memsharded]

It would be necessary to illustrate with a failing test what is the behavior that we want to fix. Because independently of what it is being done here, Python os.environ will still be uppercase later, because this is how Python environment works in Windows, as environment vars in Windows are case-insensitive.

[memsharded]

Yes, and from the link you shared:

(Implementation, Windows only) The nt module is an implementation module used by the os module on Windows platforms. There’s hardly any reason to use this module directly; use os instead

[db4]

It would be necessary to illustrate with a failing test what is the behavior that we want to fix.

Yor need a real problem or just a test case? Suppose you run a bash script under Cygwin that reads ProgramFiles environment variable. It will fail if it's run from Conan.

[memsharded]

Yor need a real problem or just a test case? Suppose you run a bash script under Cygwin that reads ProgramFiles environment variable. It will fail if it's run from Conan.

A test case that illustrate and captures the essence of the real problem. It doesn't need to be the full real problem, but the closest it is, the better.

[db4]

There’s hardly any reason to use this module directly; use os instead

Yes, but it's exactly that rare case where nt should be used :) If there is another way to fix it - great, let's use it.

[memsharded]

In this case, that function is just modifying the HOME env-var.
It sounds better to avoid the full restoration of the environment, and just restore the HOME variable, and do it conditionally only to the case the HOME var was temporarily deleted.

[db4]

The real problem: I'm trying to package https://github.com/ocaml/opam. I do:

    def build_requirements(self):
        if self.settings.os == "Windows":
            self.tool_requires("cygwin_installer/2.9.0")

    def build(self):
        port = "OCAML_PORT=msvc" if is_msvc(self) else ""
        arch = "64" if self.arch = "x86_64" else ""
        self.run(f"make compiler {port}{arch}", env="conanbuild")
        self.run("make lib-pkg", env="conanbuild")
        self.run("sh -c ./configure", env="conanbuild")

and the last command fails. I spent enough time until I found that https://github.com/ocaml/opam/blob/master/shell/msvs-detect was sensible to the environment variables case. If it is really necessary, I can dig further and find out what exactly is failing. But IMHO preserving the case is generally better than changing it.

[memsharded]

If it is really necessary, I can dig further and find out what exactly is failing. But IMHO preserving the case is generally better than changing it.

I think it sounds more like a limitation of that script that is failing to acknowledge/implement the fact that env-vars in Windows are case-insensitive.

But I agree it might be not necessary to investigate it further, and I agree that it is better if Conan doesn't do always a full restoration of the environment for just computing the Conan user home, so my above hints could be a viable solution without needing to use the nt module.

[db4]

In this case, that function is just modifying the HOME env-var.
It sounds better to avoid the full restoration of the environment, and just restore the HOME variable, and do it conditionally only to the case the HOME var was temporarily

OK, I agree not to use nt. But looking at the code I don't see that even HOME variable is changed -

conan/conans/util/windows.py

Lines 24 to 26 in a91972c

	finally:
	os.environ.clear()
	os.environ.update(old_env)

always restores the original environment. Am I missing something?

[memsharded]

OK, I agree not to use nt. But looking at the code I don't see that even HOME variable is changed -

That is the thing, it is only del deleted.
So the restoration of the full environment is overkill, not necessary to do it, if only the HOME variable is to be restored (only if it was deleted)

I think this code is mostly copy & paste legacy of environment manipulation, generic way to make sure the env is restored. But for this specific case, it seems it can be greatly optimized while removing the issue of the forced uppercase.

[db4]

I see, thanks. The current implementation seems to contain another error: if os.path.exists(home) raises an exception (as it did in Python < 3.8), return result would raise another exception because of the undefined result variable. What should be returned in that case?

[memsharded]

if os.path.exists(home) raises an exception

I am not aware of this issue. When does it raise? I haven't seen anything related to this, is it possible to reproduce it? and to cover it with a test?

[db4]

I am not aware of this issue. When does it raise?

https://docs.python.org/3/library/os.path.html

Changed in version 3.8: exists(), lexists(), isdir(), isfile(), islink(), and ismount() now return False instead of raising an exception for paths that contain characters or bytes unrepresentable at the OS level.

[db4]

I think that "characters or bytes unrepresentable at the OS level" are hardly possible here, but then try block is just not necessary, right?

[db4]

@memsharded Can you please look at the last variant? I left try/finally for now, but if you like I can remove them completely. It would be great to have this included in 2.3.0 :)

[memsharded]

Thanks for your contribution!

[memsharded]

Lets merge it, the code is relatively straightforward, and the test looks it would be too ugly (like needing to access nt again)