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

Inject in the PATH the folder from the CMake exe that invoked the provider if cmake is not found in the PATH #593

Merged
merged 15 commits into from
Jan 22, 2024

Conversation

czoido
Copy link
Contributor

@czoido czoido commented Nov 17, 2023

Related to: conan-io/conan-center-index#21068

The issue arises with the CLion plugin concerning the invocation of CMake. CLion calls CMake with a command like /Applications/CLion.app/Contents/bin/cmake/mac/bin/cmake -DCMAKE_BUILD_TYPE=Debug ..., but it does not include the invoked CMake in the environment in any manner. This leads to a problem: if CMake is not installed system-wide, the plugin fails. If there is a system-wide CMake installation, that version is used instead of the one bundled with CLion. Although using a different CMake version is not inherently problematic, it could lead to inconsistencies.

The proposed solution is to check if CMake is in the path and if it's not, then inject to the PATH the same cmake executable that was used to invoke the provider, then Conan will find that binary and proceed with the installation of packages.

I had to remove the test, apparently there's something that Github Actions does not like about the test and won't pass. I tested it locally and apparently it works file, maybe we can find an alternative way of testing this.

@czoido czoido changed the title Let the user pass a CMake exe that's not the one in the path Always inject in the PATH the folder from the CMake exe that invoked the provider Nov 17, 2023
tests/test_smoke.py Outdated Show resolved Hide resolved
@czoido czoido marked this pull request as ready for review November 17, 2023 16:01
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.

LGTM, simpler and also the most expected behavior that users will want.

@jcar87
Copy link
Contributor

jcar87 commented Nov 17, 2023

Although using a different CMake version is not inherently problematic, it could lead to inconsistencies.

Any potential examples on the potential inconsistencies?

We could have a scenario where:

  • CLion invokes cmake-conan, which invokes conan install --build missing and builds the dependencies
  • A CMake installation is available system-wide, but these dependencies are built with CLion's bundled cmake (as per this PR)
  • There is an issue building one of the dependencies, and the user attempts to debug this by calling conan install or conan create themselves, directly from the command line.

In the scenario above, the users commands would run with a different version of CMake than the one being propagated by cmake-conan. - This this happened to me I would certainly find it very confusing. I don't think developers are aware that CLion has its own bundled CMake configured by default? I could be wrong - but I would keep this in mind.

Either way I think I would constrain this a little bit, and only propagate the modified PATH environment variable if the directory containing CMake is not already there. The reason for this is that if Cmake is alread in the PATH, we are still reordering the entires of PATH, and the directory that contains CMake will now be "first". If that directory contains other things, that may be overriding the location of the tools in the system, if there were tools in other places.

conan_provider.cmake Outdated Show resolved Hide resolved
@czoido czoido changed the title Always inject in the PATH the folder from the CMake exe that invoked the provider Inject in the PATH the folder from the CMake exe that invoked the provider if cmake is not found in the PATH Nov 20, 2023
@jcar87 jcar87 merged commit f68385d into conan-io:develop2 Jan 22, 2024
4 checks passed
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.

3 participants