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

Compute CMAKE_PROGRAM_PATH from returned json #626

Draft
wants to merge 2 commits into
base: develop2
Choose a base branch
from

Conversation

jcar87
Copy link
Contributor

@jcar87 jcar87 commented Mar 11, 2024

This might work to set this after the first call to find_package (as the known limitation).

The implementation in CMakeToolchain is:

        build_req = self._conanfile.dependencies.build.values()
        build_bin_paths = []
        for req in build_req:
            cppinfo = req.cpp_info.aggregated_components()
            build_paths.extend(cppinfo.builddirs)
            build_bin_paths.extend(cppinfo.bindirs)

This is just an approximation by relying on the json alone.

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.

I think it is fine, can be moved forward.
Maybe we want to consider at some point having something built-in, a new generator or something that can just generate the information in a way that it can be consumed more easily by the provider?

endfunction()

macro(conan_set_program_path)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be a bit cleaner to do a function and "return" the variable to be used later

string(JSON _dep_binary GET ${conan_stdout} graph nodes ${_node_number} binary)
string(JSON _dep_context GET ${conan_stdout} graph nodes ${_node_number} context)

if(NOT _dep_binary STREQUAL "Cache" OR NOT _dep_context STREQUAL "build")
Copy link
Member

Choose a reason for hiding this comment

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

"Downloaded" or "Updated" will also be available to use, why ignoring them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see! Yeah I need to add those - the ones that have "skip" still have the fields but they are null, so I need a way to filter out anything that isn't valid. Perhaps this isn't the best field to check if the binary package is in the cache (I'm assuming that Cache, Download and Updated for the binary field ALL mean they are in the cache with valid paths?)

Copy link
Member

Choose a reason for hiding this comment

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

Isn't it enough to filter out the ones with binary=="Skip"? that should work I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmm what would happen if we have different versions of the same dependency (via tool requires), and tools.graph:skip_binaries=False ? would there be a chance that we add bin dirs for different versions of the samae dependency in this case?

Copy link
Member

Choose a reason for hiding this comment

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

You can check the node context attribute, it will be "host" and "build", you can differentiate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can go with the condition "binary is not skip" and "context is build" - hopefully that covers all cases correctly!

Copy link
Member

Choose a reason for hiding this comment

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

I think so

Comment on lines +479 to +486
foreach(_component RANGE ${_current_node_components})
string(JSON _current_component_name MEMBER ${conan_stdout} graph nodes ${_node_number} cpp_info ${_component})
string(JSON _current_component_bindirs GET ${conan_stdout} graph nodes ${_node_number} cpp_info ${_current_component_name} bindirs)
string(JSON _bindirs_length LENGTH ${_current_component_bindirs})
math(EXPR _bindirs_length "${_bindirs_length} - 1")
foreach(_bindir_index RANGE ${_bindirs_length})
string(JSON _bindir GET ${_current_component_bindirs} ${_bindir_index})
list(APPEND _CONAN_PROGRAM_PATH ${_bindir})
Copy link
Member

Choose a reason for hiding this comment

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

Master of CMake! I am happy that I didn't have to write this code😄

@@ -470,7 +470,7 @@ macro(conan_set_program_path)
string(JSON _dep_binary GET ${conan_stdout} graph nodes ${_node_number} binary)
string(JSON _dep_context GET ${conan_stdout} graph nodes ${_node_number} context)

if(NOT _dep_binary STREQUAL "Cache" OR NOT _dep_context STREQUAL "build")
if(_dep_binary STREQUAL "Skip" OR NOT _dep_context STREQUAL "build")

Choose a reason for hiding this comment

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

Wouldn't you play safer doing a comparison against a lowercase value?:

if(_dep_binary_lowercase STREQUAL "skip" OR NOT _dep_context_lowercase STREQUAL "build")

Choose a reason for hiding this comment

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

Or even defining those "Skip", "build"... somewhere as constants. Although that may be overkill. 😄 This type of code always looks to me like a bit like "magic numbers".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this part of Conan's json formatter, so it's essentially a public API - if this ever changed in Conan, our tests (in Conan) would fail straight away so we would know that would be a breaking change (minimising the chance of this ever happening - so not sure we can something by doing lowercase).

they could be constants but then we would have more indirection given how CMake expands variables but I'll consider that

Choose a reason for hiding this comment

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

Thanks for the explanation! And roger that CMake code is by itself complicated enough to introduce further indirections.

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