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

automatically add msys2/usr/bin path #12457

Merged
merged 5 commits into from
Nov 15, 2022

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Nov 3, 2022

Changelog: Feature: Automatically add the msys2 usr/bin folder where bash.exe is to the PATH.
Docs: Omit

Close #12110

@memsharded memsharded added this to the 1.54 milestone Nov 3, 2022
path = os.path.join(conanfile.generators_folder, "msys2_mode.bat")
# Make sure we save pure .bat files, without sh stuff
wb, conanfile.win_bash = conanfile.win_bash, None
Copy link
Member Author

Choose a reason for hiding this comment

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

I have to say that I don't love this approach, I tried different alternatives: passing an argument, forcing the .bat extension to override the configuration... I didn't love them either, and all had other potential downsides too, so this is the best I could come up with

@memsharded memsharded modified the milestones: 1.54, 1.55 Nov 4, 2022
@memsharded
Copy link
Member Author

Relevant: https://www.msys2.org/wiki/Launchers/

@@ -91,7 +94,7 @@ def _windows_bash_wrapper(conanfile, command, env, envfiles_folder):
accepted_extensions=("sh", ))
wrapped_user_cmd = _escape_windows_cmd(wrapped_user_cmd)

final_command = '{} -c {}'.format(wrapped_shell, wrapped_user_cmd)
final_command = '{} --login -c {}'.format(wrapped_shell, wrapped_user_cmd)
Copy link
Member Author

Choose a reason for hiding this comment

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

Trying with --login, lets see what happens

Copy link
Member Author

Choose a reason for hiding this comment

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

It is weird, the test_autotools_bash_complete fails with

aclocal-1.16: error: 'configure.ac' is required

when using --login, I don't know how it is connected

Copy link
Contributor

Choose a reason for hiding this comment

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

mm it could be related - that error would come from autoconf, which is being called in that test - however, configure.ac is also provided by that test. I suspect --login is changing the current working directory, so it's probably defaulting to the "user home", and thus autoconf no longer finds configure.ac in the cwd

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. The --login is effectively doing a cd ~ to my current user home, so it doesn't find the project files expected to be. This is the fragility of the --login, that it depends on the user running the command and what they might have configured in their ~/.bash_profile, ~/.bash_login, and ~/.profile

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the setting to cwd to the current working directory is more of a shell behaviour rather than a user configuration files issue.

I can understand the concerns of loading ~/.bash_profile and similar, but I have a suspicion that in practical terms, in most other scenarios if Conan is being invoked from a shell that already loaded those files anyways and the environment state is propagated to all the subprocesses. So on Linux, macOS, on Windows if Conan is invoked from one of the subsystem shells - those files (~/.bash_profile, ~/.bash_login, and ~/.profile) are already loaded and exposed.

I think the problem here is unique in the sense that Conan (the Python interpreter running conan) may be invoked say from a windows command prompt or windows powershell, but it is running some commands by invoking a bash.exe directly, and not the command prompt wrappers they recommend, so the shell is misconfigured (and thus /usr/bin is missing). My concern by automatically adding msys2/usr/bin is that there may be other things in the future that we realise are missing or not set up, and users may have a difficult time reproducing or troubleshooting the issues because when they try to run commands inside their msys2 shells, things will work for them.

bash will load /etc/profile, followed by the user profile files. It does have a flag --no-profile to inhibit the behaviour, but I'm not sure whether that applies to all files (/etc and the user ones), or just the user ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that using --login and defining CHERE_INVOKING at least the test suite is green.

@memsharded memsharded merged commit aad870b into conan-io:develop Nov 15, 2022
@memsharded memsharded deleted the feature/auto_add_msys2_path branch November 15, 2022 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] self.win_bash and msys2, internal commands not found
3 participants