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

- allow vcvars for clang-cl #3574

Merged
merged 13 commits into from Sep 27, 2018

Conversation

Projects
None yet
4 participants
@SSE4
Copy link
Contributor

commented Sep 19, 2018

Signed-off-by: SSE4 tomskside@gmail.com

related to the:
#1839
conan-community/community#43

  • 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. Also adding a description of the changes in the changelog.rst file. https://github.com/conan-io/docs

Changelog: Feature: Allow vcvars to run if clang-cl compiler is detected.

@ghost ghost added the contributor pr label Sep 19, 2018

Show resolved Hide resolved conans/test/util/vcvars_clangcl_test.py Outdated
Show resolved Hide resolved conans/test/util/vcvars_clangcl_test.py Outdated
Show resolved Hide resolved conans/test/util/vcvars_clangcl_test.py Outdated

@SSE4 SSE4 force-pushed the SSE4:vcvars_clangcl branch from 51b0a2a to 6b59c11 Sep 19, 2018

- allow vcvars for clang-cl
Signed-off-by: SSE4 <tomskside@gmail.com>
@lasote

lasote approved these changes Sep 20, 2018

Copy link
Contributor

left a comment

My only concern is the breaking behavior, but prior to this PR, passing a non-visual studio compiler with whatever version could have unexpected results. So for me is ok.

@lasote lasote added this to the 1.8 milestone Sep 21, 2018

@jgsogo
Copy link
Member

left a comment

I will add also a test mocking the function vs_installation_path in order to test the new if/else clauses.

Show resolved Hide resolved conans/client/tools/win.py Outdated
Show resolved Hide resolved conans/client/tools/win.py Outdated
Show resolved Hide resolved conans/client/tools/win.py Outdated

SSE4 added some commits Sep 24, 2018

- move _visual_compiler_last from detect.py into win.py, renaming to …
…the latest_visual_studio_version_installed

Signed-off-by: SSE4 <tomskside@gmail.com>
- look from the latest to oldest
Signed-off-by: SSE4 <tomskside@gmail.com>
- add negative test case
Signed-off-by: SSE4 <tomskside@gmail.com>
@SSE4

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

@lasote @jgsogo added test with mocks, moved and renamed _visual_compiler_last

Show resolved Hide resolved conans/client/tools/win.py Outdated
Show resolved Hide resolved conans/client/tools/win.py Outdated
Show resolved Hide resolved conans/client/tools/win.py Outdated

SSE4 added some commits Sep 24, 2018

- handle latest_visual_studio_version_installed returns None
Signed-off-by: SSE4 <tomskside@gmail.com>
- add system_registry_key context manager, ensure key is always closed
Signed-off-by: SSE4 <tomskside@gmail.com>
@lasote

lasote approved these changes Sep 25, 2018

@lasote

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

Pending to open a PR in the docs with:

  • changelog
  • system_registry_key tool (reference/tools)
  • latest_visual_studio_version_installed (reference/tools)
@jgsogo

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

After talking with @lasote, @SSE4, please document only latest_visual_studio_version_installed in reference/tools; but keep context manager system_registry_key undocumented and make it private (change name to _system_registry_key).

@lasote lasote added stage: review and removed contributor pr labels Sep 25, 2018

- make system_registry_key private
Signed-off-by: SSE4 <tomskside@gmail.com>
@SSE4

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

made system_registry_key private, opened conan-io/docs#850

@danimtb
Copy link
Member

left a comment

I see the value of having a _system_registry_key() function and it will come handy in some other detections and checks but we have to reduce the complexity of its usage

Show resolved Hide resolved conans/client/tools/win.py Outdated
Show resolved Hide resolved conans/client/tools/win.py

SSE4 added some commits Sep 26, 2018

- return just Visual Studio version
Signed-off-by: SSE4 <tomskside@gmail.com>
- simplify _system_registry_key
Signed-off-by: SSE4 <tomskside@gmail.com>
@danimtb
Copy link
Member

left a comment

LGTM, just small changes required 😃

Show resolved Hide resolved conans/client/tools/win.py Outdated

SSE4 and others added some commits Sep 26, 2018

- remove decorator
Signed-off-by: SSE4 <tomskside@gmail.com>
Merge pull request #9 from jgsogo/SSE-vcvars_clangcl
Try/catch for OpenKey and for QueryValue
Show resolved Hide resolved conans/client/conf/detect.py Outdated
- set detected compiler tuple properly
Signed-off-by: SSE4 <tomskside@gmail.com>
@jgsogo

jgsogo approved these changes Sep 26, 2018

@jgsogo jgsogo merged commit 2b3e0e3 into conan-io:develop Sep 27, 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 27, 2018

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

- allow vcvars for clang-cl (conan-io#3574)
* - allow vcvars for clang-cl

Signed-off-by: SSE4 <tomskside@gmail.com>

* - move _visual_compiler_last from detect.py into win.py, renaming to the latest_visual_studio_version_installed

Signed-off-by: SSE4 <tomskside@gmail.com>

* - look from the latest to oldest

Signed-off-by: SSE4 <tomskside@gmail.com>

* - add negative test case

Signed-off-by: SSE4 <tomskside@gmail.com>

* - handle latest_visual_studio_version_installed returns None

Signed-off-by: SSE4 <tomskside@gmail.com>

* - add system_registry_key context manager, ensure key is always closed

Signed-off-by: SSE4 <tomskside@gmail.com>

* - make system_registry_key private

Signed-off-by: SSE4 <tomskside@gmail.com>

* - return just Visual Studio version

Signed-off-by: SSE4 <tomskside@gmail.com>

* - simplify _system_registry_key

Signed-off-by: SSE4 <tomskside@gmail.com>

* - remove decorator

Signed-off-by: SSE4 <tomskside@gmail.com>

* try/catch for openKey and for QueryValue

* - set detected compiler tuple properly

Signed-off-by: SSE4 <tomskside@gmail.com>
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.