Skip to content

- allow vcvars for clang-cl - #3574

Merged
jgsogo merged 13 commits into
conan-io:developfrom
SSE4:vcvars_clangcl
Sep 27, 2018
Merged

jgsogo merged 13 commits into
conan-io:developfrom
SSE4:vcvars_clangcl

Conversation

@SSE4

@SSE4 SSE4 commented Sep 19, 2018

Copy link
Copy Markdown
Contributor

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

related to the:
#1839
https://github.com/conan-community/community/issues/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
Comment thread conans/test/util/vcvars_clangcl_test.py Outdated
Comment thread conans/test/util/vcvars_clangcl_test.py Outdated
Comment thread conans/test/util/vcvars_clangcl_test.py Outdated
Signed-off-by: SSE4 <tomskside@gmail.com>

@lasote lasote left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 jgsogo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment thread conans/client/tools/win.py Outdated
Comment thread conans/client/tools/win.py Outdated
Comment thread conans/client/tools/win.py Outdated
…the latest_visual_studio_version_installed

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

SSE4 commented Sep 24, 2018

Copy link
Copy Markdown
Contributor Author

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

Comment thread conans/client/tools/win.py Outdated
Comment thread conans/client/tools/win.py Outdated
Comment thread conans/client/tools/win.py Outdated
@lasote

lasote commented Sep 25, 2018

Copy link
Copy Markdown
Contributor

Pending to open a PR in the docs with:

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

@jgsogo

jgsogo commented Sep 25, 2018

Copy link
Copy Markdown
Contributor

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

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

SSE4 commented Sep 26, 2018

Copy link
Copy Markdown
Contributor Author

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

@danimtb danimtb left a comment

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.

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

Comment thread conans/client/tools/win.py Outdated
Comment thread conans/client/tools/win.py
Signed-off-by: SSE4 <tomskside@gmail.com>
Signed-off-by: SSE4 <tomskside@gmail.com>

@danimtb danimtb left a comment

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.

LGTM, just small changes required 😃

Comment thread conans/client/tools/win.py Outdated
SSE4 and others added 2 commits September 26, 2018 15:57
Try/catch for OpenKey and for QueryValue
Comment thread conans/client/conf/detect.py Outdated
Signed-off-by: SSE4 <tomskside@gmail.com>
@jgsogo
jgsogo merged commit 2b3e0e3 into conan-io:develop Sep 27, 2018
@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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants