Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows: Fixed environment variables not read as unicode. #2417

Merged
merged 12 commits into from
Apr 3, 2023

Conversation

Klaim
Copy link
Member

@Klaim Klaim commented Mar 28, 2023

The code acquiring environment variables on Windows was doing so using the non-unicode API which lead to some environment variables containing unicode path (or other unicode values) leading to random crashes.
This version uses a simpler API call which is unicode-enabled and should fix #2153

@Klaim
Copy link
Member Author

Klaim commented Mar 29, 2023

For info I'm trying to rewrite the solution with the safe functions and to handle the setenv because otherwse tests cannot pass.

Copy link
Member

@AntoinePrv AntoinePrv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 🎉

Can we take this opportunity to add some libmamba tests? Since we have env::get and env::set we can at least test them together.

Testing env::get alone would require setting an env var from outside the test exe. Not perfect, but maybe necessary?

Comment on lines +33 to +37
LOG_ERROR << fmt::format(
"Failed to acquire environment variable '{}' : errcode = {}",
key,
error_code
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: One can also do (with the right header)

Suggested change
LOG_ERROR << fmt::format(
"Failed to acquire environment variable '{}' : errcode = {}",
key,
error_code
);
fmt::format_to(LOG_ERROR,
"Failed to acquire environment variable '{}' : errcode = {}",
key,
error_code
);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW I know this is possible but I dont know if that changes something performance-wise for fmt? Do you know?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I tried that syntax and it doenst seem to compile, probably lacks some compatibility layer to add. I'll keep it as is for now.

@Klaim
Copy link
Member Author

Klaim commented Mar 30, 2023

@AntoinePrv I can add more tests although note that I had to change set because current C++ tests (test_libmamba) have tests that use the environment and check that they are getting what is expected (they were failing with my first commit) - but they are not testing these specific functions.

@Klaim
Copy link
Member Author

Klaim commented Mar 30, 2023

I will also add a basic test for the unicode functions.

@Klaim Klaim added the type::bug Something isn't working label Mar 30, 2023
@Klaim Klaim requested a review from Hind-M March 30, 2023 14:47
@Klaim
Copy link
Member Author

Klaim commented Mar 30, 2023

@AntoinePrv I added basic tests using mamba::env::get to get environment variables that are always available on windows, probably will also fail if the user running the test is using a unicode path (we migth want to do that on the CI too...).
If you want to add another test for linux be my guest, I cant check right now such code as I'm stuck on windows until next week.

@Klaim Klaim force-pushed the klaim/env-unicode-windows branch from 6240439 to 579a65b Compare March 30, 2023 16:32
Copy link
Member

@AntoinePrv AntoinePrv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

libmamba/tests/src/core/test_system_env.cpp Outdated Show resolved Hide resolved
libmamba/tests/src/core/test_util_os.cpp Outdated Show resolved Hide resolved
@Klaim Klaim force-pushed the klaim/env-unicode-windows branch from 579a65b to 3de4685 Compare April 3, 2023 15:31
@JohanMabille JohanMabille merged commit 1e3f415 into mamba-org:main Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type::bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.\micromamba.exe --help has no output
4 participants