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

Look for vswhere in PATH when using tools.vswhere() #4805

Merged
merged 4 commits into from Mar 22, 2019

Conversation

Projects
None yet
3 participants
@danimtb
Copy link
Member

commented Mar 22, 2019

Changelog: Fix: Look for vswhere in PATH when using tools.vswhere()
Docs: omit

  • Refer to the issue that supports this Pull Request: closes #4786
  • 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.

@danimtb danimtb added this to the 1.14 milestone Mar 22, 2019

@ghost ghost assigned danimtb Mar 22, 2019

@ghost ghost added the stage: review label Mar 22, 2019

@lasote lasote merged commit 7c829c8 into conan-io:develop Mar 22, 2019

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 Mar 22, 2019

program_files = os.environ.get("ProgramFiles(x86)", os.environ.get("ProgramFiles"))

installer_path = None
vswhere_in_path = which("vswhere")

This comment has been minimized.

Copy link
@memsharded

memsharded Mar 22, 2019

Contributor

Don't call which() if you are not going to use it.
Delay it to:

vswhere_path = installer_path or which("vswhere")
expected_path = os.path.join(program_files, "Microsoft Visual Studio", "Installer",
"vswhere.exe")
if os.path.isfile(expected_path):
vswhere_path = expected_path

This comment has been minimized.

Copy link
@memsharded

memsharded Mar 22, 2019

Contributor

I don't see this vswhere_path is used.
I don't understand what this part of the test is testing, that vswhere() can be executed even if it is not in the path? Maybe a comment, "vswhere is in PrgramFiles BUT NOT in the system PATH"

This comment has been minimized.

Copy link
@danimtb

danimtb Mar 25, 2019

Author Member

Yes, that's it. I use the vswhere_path variable to locate the real path in the testing machine and then set-up the different scenarios for a real test

env = {"ProgramFiles": None, "ProgramFiles(x86)": None}
if not which("vswhere") and vswhere_path:
vswhere_folder = os.path.join(program_files, "Microsoft Visual Studio", "Installer")
env.update({"PATH": [vswhere_folder]})

This comment has been minimized.

Copy link
@memsharded

memsharded Mar 22, 2019

Contributor

Is it not something we want to do, even if vswhere is in the path, and which() returned True?

This comment has been minimized.

Copy link
@danimtb

danimtb Mar 25, 2019

Author Member

The rationale is the following: If we know (from previous case) where is vswhere and we dont found it in the PATH (which() returned None). Then set vswhere_path value in the path so it can be found only there (that is the previus conditions of the test: "vswhere in PATH but not in ProgramFiles")

@lasote

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

@danimtb take a look to the @memsharded comments and open a new PR if needed. Sorry

@danimtb danimtb referenced this pull request Mar 25, 2019

Merged

Review of vswhere PR #4805 #4815

5 of 5 tasks complete

memsharded added a commit that referenced this pull request Mar 25, 2019

Review of vswhere PR #4805 (#4815)
* Test vswhere found in path too

* Look for vswhere in PATH when using tools.vswhere()

* improvement

* test

* optimize which() call

* clarify test cases

* Use six for assert
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.