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

[fix] Issues related to OSInfo.bash_path() #3455

Merged
merged 2 commits into from Sep 3, 2018

Conversation

Projects
None yet
2 participants
@jgsogo
Copy link
Member

commented Sep 1, 2018

Fails when there is no windows_subsytem

Changelog: Bugfix: Fixed OSInfo.bash_path() when there is no windows_subsystem.

@ghost ghost assigned jgsogo Sep 1, 2018

@ghost ghost added the stage: review label Sep 1, 2018

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2018

I need to fix also this test, but I need some advice about it...

def run_in_bash_test(self):

It fails when there is no windows_subsystem because of these lines in run_in_windows_bash:

        bash_path = os_info.bash_path()  # <--- Returns None!!!
        bash_path = '"%s"' % bash_path if " " in bash_path else bash_path

We can skip the test if not os_info.detect_windows_subsystem() or run these tests only if cygwin is present...

@jgsogo jgsogo requested a review from memsharded Sep 1, 2018

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2018

What you spotted is indeed a bug in the test, good catch.

The run_in_bash_test() bug, I am not sure, the test seems Mocked, so it shouldn't need a real subsystem to run, what am I missing? How does the test fail in your system?

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2018

The two lines from function run_in_windows_bash that I copied from the function code:

        bash_path = os_info.bash_path()  # <--- Returns None!!!
        bash_path = '"%s"' % bash_path if " " in bash_path else bash_path

If there is no "bash" in the system, bash_path is None (see comment) and in the next line it is trying to iterate it: if " " in bash_path, so:

======================================================================
ERROR: run_in_bash_test (conans.test.util.tools_test.ToolsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Javi\dev\conan\conans\test\util\tools_test.py", line 830, in run_in_bash_test
    tools.run_in_windows_bash(conanfile, "a_command.bat", subsystem="cygwin")
  File "C:\Javi\dev\conan\conans\client\tools\win.py", line 481, in run_in_windows_bash
    bash_path = '"%s"' % bash_path if " " in bash_path else bash_path
TypeError: argument of type 'NoneType' is not iterable

To keep the test we need to mock the call to bash_path().

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2018

Yes, I think that mocking OSInfo.bash_path() could be the way to go, specially because in the test, we can see:

   with tools.environment_append({"CONAN_BASH_PATH": "path\\to\\mybash.exe"}):
            tools.run_in_windows_bash(conanfile, "a_command.bat", subsystem="cygwin")
            self.assertIn('path\\to\\mybash.exe --login -c', conanfile._runner.command)

Maybe it is good enough to set CONAN_BASH_PATH, instead of actually mocking.

@memsharded memsharded added this to the 1.8 milestone Sep 2, 2018

@memsharded memsharded merged commit 8c9ea8d into conan-io:develop Sep 3, 2018

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Sep 3, 2018

@jgsogo jgsogo deleted the jgsogo:fix/tests/osinfo_bash_path branch Sep 19, 2018

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.