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.

Sign up for GitHub

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

Jump to bottom

Compute CMAKE_PROGRAM_PATH from returned json #626

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

Compute CMAKE_PROGRAM_PATH from returned json #626

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
716651e
Compute CMAKE_PROGRAM_PATH from returned json
jcar87 Mar 11, 2024
f6be8ef
fixes
jcar87 Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions conan_provider.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link

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")

Copy link

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

Copy link

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.

continue()
endif()

Expand All @@ -488,7 +488,6 @@ macro(conan_set_program_path)
endforeach()
endforeach()

message("conan_program_path: ${_CONAN_PROGRAM_PATH}")
list(REMOVE_DUPLICATES _CONAN_PROGRAM_PATH)
endmacro()

Expand Down
Loading