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

Add tool configuration to specify CMake executable path (#12701) #13940

Merged
merged 1 commit into from
May 24, 2023

Conversation

iskunk
Copy link
Contributor

@iskunk iskunk commented May 23, 2023

Changelog: Feature: Add a tools.cmake:cmake_program configuration item to allow specifying the location of the desired CMake executable.
Docs: conan-io/docs#3232

Fixes: #12701

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

@CLAassistant
Copy link

CLAassistant commented May 23, 2023

CLA assistant check
All committers have signed the CLA.

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.

Thanks for your contribution.

Could you please add some unit test that will cover this new conf functionality? One in the unittest folder could be enough

If you want this functionality to be included in next 2.0.X releases, please target the PR to the release/2.0 branch, it can be added there, no risk.

Also, add a unit test for the new configuration item.
@iskunk iskunk force-pushed the feature/cmake-tool-conf branch from bf453d5 to 1141275 Compare May 23, 2023 19:09
@iskunk iskunk changed the base branch from develop2 to release/2.0 May 23, 2023 19:10
@iskunk
Copy link
Contributor Author

iskunk commented May 23, 2023

All right, I've re-targeted the PR to release/2.0, and added my best guess at a unit test for the new configuration item. Please double-check it.

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.

Looks good.
The only remaining issue is that moving the branches targets for the PR created some confussion for the CLA-assistant. If it doesn't automatically resolve (sometimes after some time CLA-assistant is able to figure out the contributors correctly), it might need to close it and open a new PR, lets see how it goes.
Thanks again for your contribution.

@memsharded memsharded added this to the 2.0.6 milestone May 24, 2023
@memsharded memsharded requested a review from AbrilRBS May 24, 2023 08:09
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.

[bug] CONAN_CMAKE_PROGRAM not respected for CMake Toolchain
5 participants