Skip to content

BugFix: allow run_in_windows_bash in MSYS/Cygwin - #8506

Merged
memsharded merged 1 commit into
conan-io:developfrom
SSE4:run_in_windows_bash_msys
Feb 22, 2021
Merged

BugFix: allow run_in_windows_bash in MSYS/Cygwin#8506
memsharded merged 1 commit into
conan-io:developfrom
SSE4:run_in_windows_bash_msys

Conversation

@SSE4

@SSE4 SSE4 commented Feb 15, 2021 •
edited by czoido
Loading

Copy link
Copy Markdown
Contributor

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.

@SSE4
- allow run_in_windows_bash in MSYS/Cygwin
b30257f 
Signed-off-by: SSE4 <tomskside@gmail.com>
memsharded
memsharded approved these changes Feb 15, 2021
View reviewed changes
Comment thread conans/client/tools/win.py
It requires to have MSYS2, CYGWIN, or WSL
"""
env = env or {}
if platform.system() != "Windows":

@memsharded memsharded Feb 15, 2021

Copy link
Copy Markdown
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

@SSE4 SSE4 Feb 15, 2021

Copy link
Copy Markdown
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

@memsharded memsharded Feb 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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?

@SSE4 SSE4 Feb 16, 2021

Copy link
Copy Markdown
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

@memsharded memsharded Feb 16, 2021

Copy link
Copy Markdown
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.

@SSE4 SSE4 Feb 16, 2021

Copy link
Copy Markdown
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

memsharded commented Feb 15, 2021

Copy link
Copy Markdown
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

SSE4 commented Feb 16, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

memsharded commented Feb 16, 2021

Copy link
Copy Markdown
Member

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

@SSE4

SSE4 commented Feb 16, 2021

Copy link
Copy Markdown
Contributor Author

@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.

@SSE4 SSE4 mentioned this pull request Feb 16, 2021
Merged
5 tasks
@memsharded
memsharded merged commit 54fd723 into conan-io:develop Feb 22, 2021
@dependabot dependabot Bot mentioned this pull request Mar 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@memsharded memsharded memsharded approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.34

Development

Successfully merging this pull request may close these issues.

2 participants

@SSE4 @memsharded