Windows environment variables set incorrectly during install

[9names]

Bug description

#182 Added support for setting CLANG_PATH and PATH in Windows to allow xtensa builds without manually changing environment variables. Unfortunately, that implementation is incorrect. It uses setx /m, which sets system-wide environment variables.
This implementation has 3 major issues that I'm aware of:

1. you need to be at at command prompt with elevated privileges while running espup install to successfully set these variables. The installer still reports "Your environment variables have been updated!" but it doesn't set any environment variables.
2. if you do succeed at settings these env-vars, other users will inherit them but cannot use the installed libclang because it exists in another user's %PROFILEDIR%. This could escape detection until a second user runs espup install
3. it does not check the new path will fit before adding the esp paths to the beginning, so if it does succeed at setting the path it will for most users truncate it, dropping potentially important paths from PATH

The fix for 3 is to check path length first and only update the path if it is safe to do so.

The fix for 1 and 2 is to use setx, not setx /m. This will set the variables in the current users environment instead, which does not require any extra privileges and does not interfere with other users. It does require the user to open a new console before the environment variables are observable, as is noted in the docs for setx:

2) On a local system, variables created or modified by this tool
    will be available in future command windows but not in the
    current CMD.exe command window.

• Would you like to work on a fix?
  No

To Reproduce

Start windows
Install rust
Install espup
Run espup install
Open new cmd
Inspect environment variables - discover that they are missing.
Build esp-idf-template - fails on missing libclang

[9names]

Now that it's actually working, I can see that my path is getting truncated by setx.
The path environment variable can be 2048 characters long, but set/setx truncates at 1028.
There should be a check that the new path length will fit before trying to set it.
[edit: updated the original issue with this problem]

[9names]

Oh, it also seems that espup install adds the paths to both the beginning and end of the PATH env-var, so truncation is certain to to happen

[9names]

The more I think about this, the more I feel it's the wrong solution to this problem.
Putting all these clang lib paths first in the path envvar helps fix your problem (getting Rust to see the right libclang during linking), but doesn't this potentially break other apps that are trying to use the system libclang?

Instead of trying to shove all this in the users path so that is implicitly part of the linker library search path, can we instead put these paths in their own environment variables and inject them into the linker step via build.rs?
I assume the problem is if we add them to cargo:rustc-link-search they will be search last, which might mean we get the wrong version? Can we name the libclang differently so that it can only match espup's vendored version?

[SergioGasquez]

Hi @9names!

Thanks for reporting this issue in a very detailed way, and sorry for the late reply (I was on vacation). I will try to reproduce it on a Windows environments and try to solve it!

[SergioGasquez]

Just tried reproducing the issue in a Windows Sandbox:

1. Downloaded espup-x86_64-pc-windows-msvc.exe from v0.3.2 release and rust-init.exe from https://www.rust-lang.org/tools/install.
2. Installed rustup via rust-init.exe
3. Installed MSVC via rustup
4. Run "espup-x86_64-pc-windows-msvc.exe" install in a Command Prompt (without running it as admin)
5. Here is the resulting environment:
   

[SergioGasquez]

It looks like when using Windows Sandbox, the user has elevated privileges, so the terminal is run by admin even when you don't open it as an administrator.

[SergioGasquez]

Since set and setx seem to have a 1024-character limit, maybe the solution is to revert the changes and have Windows user source the export file as we do in Unix. @9names do you have any feedback on this?

[9names]

I think this is the best approach. I can make a PR to improve the esp book's instructions around this workflow.

[SergioGasquez]

Also, once that LLD is supported as linker for no_std, most of the PATH changes won’t be necessary as we would not require GCC in no_std and for std projects, it will be handled by esp-idf-sys. Maybe, this will solve the 1024 max char issue.

[SergioGasquez]

Hi @9names! Just discovered that we may be able to solve the issue using winreg, here is my branch where I was able to install and set the environment without requiring elevated privileges and without truncation: https://github.com/SergioGasquez/espup/tree/fix/windows-environment

Feel free to have a look at it or testing it:

cargo install espup --git https://github.com/SergioGasquez/espup --branch fix/windows-environment
espup install

[9names]

I tested install + uninstall with very long PATH, it works as expected now.
Also looked over the code and it looks good to me. Great work! 🚀 🚀 🚀

[SergioGasquez]

I tested install + uninstall with very long PATH, it works as expected now. Also looked over the code and it looks good to me. Great work! rocket rocket rocket

Thanks for reporting the issue and testing/reviewing the changes! Just merged it and will create a new release!