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

feat: set tool path #526

Open
wants to merge 1 commit into
base: develop2
Choose a base branch
from
Open

Conversation

tim-goto
Copy link
Contributor

Scenario: I want to install build requirements with conan. In order to use them in my CMake files after the first find_package, I have to get the conan install path into CMake context.

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.

Hi @tim-goto

Thanks for your contribution.
I think it would be worth to try to consider this first as a Conan built-in feature, as the usage of environment vars like PATH things in pure CMake is a relevant use case also outside of CMake-Conan.

find_package(hello REQUIRED)
find_program(
NINJA_EXECUTABLE ninja
PATHS ${CONAN_TOOL_PATH} REQUIRED
Copy link
Member

Choose a reason for hiding this comment

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

This sounds a bit against the intended "transparent" integration design.
Maybe it makes sense to add things directly to the path?

This might be related to a Conan feature we have thought a couple of times: generating CMake "env" files instead of shell script or .bat files with the environment definitions. Maybe worth giving it a try as a Conan built-in feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, it adds implicit manipulation of state. I could not think of any other way to do this. If you think it is better suited as a conan builtin, I can also create a PR there. Would this be a separate cmake-env generator?

Copy link
Member

Choose a reason for hiding this comment

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

That is a very good question, I don't know what the best interface would be, maybe an opt-in in CMakeDeps, like it shouldn't be necessary to modify all recipes for this to be active. It also depends on how the consumption story looks like.
Maybe start from a quick and dirty proof of concept as a new generator (cleaner to develop and review, and we can think later about the best UI)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, of course, if you want to give it a try in Conan, go ahead. Just beware that this is pure experimentation, no guarantees that it can be moved forward, it will depend on some factors like space for future clean evolution, potential risks of being trapped in a local maxima, maintainability, etc. But if you are willing to try, that would be very welcome, of course! 🙂

@jcar87
Copy link
Contributor

jcar87 commented Sep 22, 2023

Hi @tim-goto, thank you for your contribution.

@memsharded - the proposed logic more or less matches what the CMakeToolchain does by setting CMAKE_PROGRAM_PATH. This overcomes one of the limitations mentioned in https://github.com/conan-io/cmake-conan/tree/develop2#known-limitations-with-conan-20, where find_program() doesn't work. It may not be 1:1 equivalent to what CMakeToolchain does, but it is a good approximation.

This is more or less in line with what I had in mind. @tim-goto, my suggestion (and also to echo @memsharded's concerns) is that we should be appending to CMAKE_PROGRAM_PATH, rather than create a custom CONAN_TOOL_PATH. This would enable running find_program() without any additional changes.

Although one must note that this will not work for any tool that needs to be located "during" a project() call (like a compiler, linker, build tool, etc) - but should work for anything that happens after the first find_package() call.

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