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

BugFix: allow run_in_windows_bash in MSYS/Cygwin #8506

Merged
merged 1 commit into from
Feb 22, 2021

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Feb 15, 2021

related to #8476

running AutoToolsBuildEnvironment(self, win_bash=True) in MSYS2 results in an error:

ERROR: apr/1.7.0: Error in build() method, line 92
        autotools = self._configure_autotools()
while calling '_configure_autotools', line 76
        self._autotools.configure(args=conf_args, configure_dir=self._source_subfolder)
        ConanException: Command only for Windows operating system

the fix allows run_in_windows_bash to work in sub-systems as well.

Changelog: BugFix: Allow run_in_windows_bash in MSYS/Cygwin.
Docs: omit

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@@ -635,7 +635,7 @@ def run_in_windows_bash(conanfile, bashcmd, cwd=None, subsystem=None, msys_mingw
It requires to have MSYS2, CYGWIN, or WSL
"""
env = env or {}
if platform.system() != "Windows":
Copy link
Member

Choose a reason for hiding this comment

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

Cygwin also returns "Windows" here, could be only Msys will be different

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't check Cygwin (may check tomorrow), msys returns a different value for sure

Copy link
Member

@memsharded memsharded Feb 15, 2021

Choose a reason for hiding this comment

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

The fix seems fine anyway, it was just an observation. Would you like to check something more, or can it be merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the completeness.
Cygwin:

sse4@DESKTOP-LAD63V0 ~
$ python -c "import platform; print(platform.system())"
CYGWIN_NT-10.0-18363

MSYS2:

sse4@DESKTOP-LAD63V0 MSYS ~
$ python -c "import platform; print(platform.system())"
MSYS_NT-10.0-18363

MinGW32 (MSYS2):

sse4@DESKTOP-LAD63V0 MINGW32 ~
$ python -c "import platform; print(platform.system())"
MINGW32_NT-10.0-18363

MinGW64 (MSYS2):

sse4@DESKTOP-LAD63V0 MINGW64 ~
$ python -c "import platform; print(platform.system())"
MINGW64_NT-10.0-18363

Copy link
Member

Choose a reason for hiding this comment

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

Curiously, it seems it can change, I am not sure why:

Cygwin:

$ python -c "import platform; print(platform.system())"
Windows

In any case, doesn't seem relevant, if it can change in other computers, need to take it into account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is probably not a python from a Cygwin itself. run which python to check.
if it's /usr/bin/python, then it's cygwin's one.
if it's something like /cygdrive/c/users..., then it's installed in the main Windows system.

@memsharded memsharded added this to the 1.34 milestone Feb 15, 2021
@memsharded
Copy link
Member

I think this can be merged, but I would like to note that run_in_windows_bash() is very complicated and obscure thing inside Conan codebase. I really want to improve this while we define the new helpers, mostly for Autotools, simplifying when possible, make it a bit more explicit and configurable, and being covered by real tests better.

@SSE4
Copy link
Contributor Author

SSE4 commented Feb 16, 2021

checking cygwin, it seems to be not enough:

apr/1.7.0: run_in_windows_bash: /usr/bin/bash --login -c cd "/cygdrive/home/sse4/.conan/data/apr/1.7.0/_/_/build/520c5865652102ac1331f89be33c82aad2520eb9" && /cygdrivesource_subfolder/configure '--with-installbuilddir=${prefix}/bin/build-1' '--enable-shared=no' '--enable-static=yes' '--prefix=/home/sse4/.conan/data/apr/1.7.0/_/_/package/520c5865652102ac1331f89be33c82aad2520eb9'
/bin/sh: /cygdrivesource_subfolder/configure: No such file or directory
apr/1.7.0:
apr/1.7.0: ERROR: Package '520c5865652102ac1331f89be33c82aad2520eb9' build failed
apr/1.7.0: WARN: Build folder /home/sse4/.conan/data/apr/1.7.0/_/_/build/520c5865652102ac1331f89be33c82aad2520eb9
ERROR: apr/1.7.0: Error in build() method, line 94
        autotools = self._configure_autotools()
while calling '_configure_autotools', line 78
        self._autotools.configure(args=conf_args, configure_dir=self._source_subfolder)
        ConanException: Error 127 while executing /cygdrivesource_subfolder/configure '--with-installbuilddir=${prefix}/bin/build-1' '--enable-shared=no' '--enable-static=yes' '--prefix=/home/sse4/.conan/data/apr/1.7.0/_/_/package/520c5865652102ac1331f89be33c82aad2520eb9'

I guess the error is in unix_path function which needs a simple fix:

diff --git a/conans/client/tools/win.py b/conans/client/tools/win.py
index 43db28b5a..bd69a3e47 100644
--- a/conans/client/tools/win.py
+++ b/conans/client/tools/win.py
@@ -620,7 +620,10 @@ def unix_path(path, path_flavor=None):
     if path_flavor in (MSYS, MSYS2):
         return path.lower()
     elif path_flavor == CYGWIN:
-        return '/cygdrive' + path.lower()
+        if os.path.isabs(path):
+            return '/cygdrive' + path.lower()
+        else:
+            return path.lower()
     elif path_flavor == WSL:
         return '/mnt' + path[0:2].lower() + path[2:]
     elif path_flavor == SFU:

which allows the build to start.
actually, it needs a more complete fix (e.g. for other sub-systems, plus it should translate only paths starting with drives like C:, D:, because paths like /home don't need to be adjusted).

@memsharded
Copy link
Member

@SSE4 How would you like to proceed? Fixing those things in this PR?

@SSE4
Copy link
Contributor Author

SSE4 commented Feb 16, 2021

@SSE4 How would you like to proceed? Fixing those things in this PR?

nope, this deserves its own PR, and I want to write some tests for unix_path.

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.

2 participants