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: use self._run instead of self._conanfile.run to ensure vcvars apply for Ninja/NMake generators #3803

Merged
merged 2 commits into from Oct 23, 2018

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Oct 19, 2018

follow up on #2803

when using ninja_installer/1.8.2@bincrafters/stable, right now the following code is required:

        if self.settings.compiler == 'Visual Studio':
            with tools.vcvars(self.settings, force=True, filter_known_paths=False):
                self.build_cmake()
        else:
            self.build_cmake()

I tried to simplify it in our zmq/4.2.2@bincrafters/stable recipe, but it currently fails, because there are places in CMake helper which use self._conanfile.run directly instead of self._run:

https://github.com/conan-io/conan/blob/1.8.3/conans/client/build/cmake.py#L183
https://github.com/conan-io/conan/blob/1.8.3/conans/client/build/cmake.py#L185

Changelog: Fix: vcvars is also called in the CMake() build helper when using Ninja or NMake generators.

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

@ghost ghost added the contributor pr label Oct 19, 2018
@SSE4 SSE4 changed the title - use self._run instead of self._conanfile.run to ensure vcvars apply… Fix: use self._run instead of self._conanfile.run to ensure vcvars apply… Oct 19, 2018
@SSE4 SSE4 changed the title Fix: use self._run instead of self._conanfile.run to ensure vcvars apply… Fix: use self._run instead of self._conanfile.run to ensure vcvars apply for Ninja/NMake generators Oct 19, 2018
- ensure vcvars apply for Ninja/NMake generators

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

@danimtb danimtb left a comment

Choose a reason for hiding this comment

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

I remember this change was done some time ago. Was this tested somewhere? If not, we should write a test for it

Copy link
Member

@danimtb danimtb left a comment

Choose a reason for hiding this comment

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

I remember this change (using vcvasr for ninja) was done some time ago. Was it tested somewhere? If not, it will require a proper test

@memsharded memsharded added this to the 1.9 milestone Oct 22, 2018
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

This makes sense, but it would deserve (a unit test, no need full integration test) a test

@SSE4
Copy link
Contributor Author

SSE4 commented Oct 22, 2018

I am adding the test

Signed-off-by: SSE4 <tomskside@gmail.com>
@SSE4
Copy link
Contributor Author

SSE4 commented Oct 22, 2018

unit test has been added

@SSE4
Copy link
Contributor Author

SSE4 commented Oct 22, 2018

I am not sure about CI, it fails with strange error:

ModuleNotFoundError: No module named 'semver'

not sure how is it related to my changes, probably an infrastructure error, so please re-start build

@memsharded memsharded merged commit 487ff56 into conan-io:develop Oct 23, 2018
@ghost ghost removed the contributor pr label Oct 23, 2018
@SSE4 SSE4 mentioned this pull request Oct 23, 2018
5 tasks
grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018
Fix: use self._run instead of self._conanfile.run to ensure vcvars apply for Ninja/NMake generators
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.

None yet

3 participants