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.

Sign up for GitHub

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
Merged

automatically add msys2/usr/bin path #12457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f82a228
automatically add msys2/usr/bin path
memsharded Nov 3, 2022
0f88879
fixed
memsharded Nov 3, 2022
c27ee16
try --login
memsharded Nov 14, 2022
1195b57
CHERE_INVOKING
memsharded Nov 14, 2022
f7f60ba
comments
memsharded Nov 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion conans/client/subsystems.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,18 @@ def _windows_bash_wrapper(conanfile, command, env, envfiles_folder):
# Configure MSYS2 to inherith the PATH
msys2_mode_env = Environment()
_msystem = {"x86": "MINGW32"}.get(conanfile.settings.get_safe("arch"), "MINGW64")
# https://www.msys2.org/wiki/Launchers/ dictates that the shell should be launched with
# - MSYSTEM defined
# - CHERE_INVOKING is necessary to keep the CWD and not change automatically to the user home
msys2_mode_env.define("MSYSTEM", _msystem)
msys2_mode_env.define("MSYS2_PATH_TYPE", "inherit")
# So --login do not change automatically to the user home
msys2_mode_env.define("CHERE_INVOKING", "1")
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

msys2_mode_env.vars(conanfile, "build").save_bat(path)
conanfile.win_bash = wb
env.append(path)

wrapped_shell = '"%s"' % shell_path if " " in shell_path else shell_path
Expand All @@ -91,7 +99,9 @@ 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)
# according to https://www.msys2.org/wiki/Launchers/, it is necessary to use --login shell
# running without it is discouraged
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.

return final_command


Expand Down
38 changes: 38 additions & 0 deletions conans/test/functional/toolchains/gnu/autotools/test_win_bash.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,41 @@ def build(self):
# To check that the ``autotools.install()`` has worked correctly
# FIXME IN CONAN 2.0 this will break, no local `package_folder`
assert os.path.exists(os.path.join(client.current_folder, "package", "bin", "main.exe"))


@pytest.mark.skipif(platform.system() != "Windows", reason="Requires Windows")
def test_add_msys2_path_automatically():
""" Check that commands like ar, autoconf, etc, that are in the /usr/bin folder together
with the bash.exe, can be automaticallly used when running in windows bash, without user
extra addition to [buildenv] of that msys64/usr/bin path

# https://github.com/conan-io/conan/issues/12110
"""
client = TestClient(path_with_spaces=False)
bash_path = None
try:
bash_path = tools_locations["msys2"]["system"]["path"]["Windows"] + "/bash.exe"
except KeyError:
pytest.skip("msys2 path not defined")

save(client.cache.new_config_path, textwrap.dedent("""
tools.microsoft.bash:subsystem=msys2
tools.microsoft.bash:path={}
""".format(bash_path)))

conanfile = textwrap.dedent("""
from conan import ConanFile

class HelloConan(ConanFile):
name = "hello"
version = "0.1"

win_bash = True

def build(self):
self.run("ar -h")
""")

client.save({"conanfile.py": conanfile})
client.run("create .")
assert "ar.exe" in client.out