Skip to content

Conversation

craigscott-crascit
Copy link
Contributor

Fixes: #704

@craigscott-crascit
Copy link
Contributor Author

The description of #704 suggests we could update conan_args like so:

set(conan_args -of=${conan_output_folder} ${ARGN})

I've decided not to do that in this PR, as I will include that change as part of the fix for #706. I need to wait for #707 to be merged before I can put up that change though, so I'm getting this part out of the way now as a smaller, more focused PR.

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 very much for your contribution @craigscott-crascit

I see what is the issue, and I agree with your issue report that it probably would be better to clean up the code a bit further to clarify the intention, as the set(conan_args ${conan_args} -of=${conan_output_folder}) has no sense if ${conan_args] is empty. And it would be better to clearly define conan_args with the -of + $ARGN as you suggested.

What do you think?

@craigscott-crascit
Copy link
Contributor Author

craigscott-crascit commented Sep 12, 2025

I'm going to change that line as soon as #707 is merged. I want to add the build_type as a function argument, then include that in conan_args. The code I have waiting looks something like this:

function(conan_install build_type)   # <--- note extra function argument added
    set(conan_output_folder ${CMAKE_BINARY_DIR}/conan)
    # Invoke "conan install" with the provided arguments
    set(conan_args -of=${CONAN_OUTPUT_FOLDER})
    # New code from here down
    if(build_type STREQUAL "")
        # We can't assume the build type will be ${CMAKE_BUILD_TYPE} because it may be specified
        # as something else by a profile provided by the project or user through CONAN_INSTALL_ARGS.
        set(conan_stdout_file ${CONAN_OUTPUT_FOLDER}/conan_install_log.json)
    else()
        set(conan_stdout_file ${CONAN_OUTPUT_FOLDER}/conan_install_log-${build_type}.json)
        list(APPEND conan_args -s build_type=${build_type})
    endif()

Then later on, I write out the contents of the ${conan_stdout} variable to the file at ${conan_stdout_file}. The ${ARGN} has to come after the -s build_type=${build_type} part, so I wanted to hold off appending it to conan_args until I do the above. That will reduce the churn in the git diffs around this area.

EDIT: I will probably end up with a simpler version of the above, since we will be able to guarantee build_type won't be empty. But I've copy-and-pasted the above from an earlier version I had when working on this a few months ago. The final version will be simpler, but there will need to be some decisions made as part of addressing #706.

@jcar87 jcar87 force-pushed the unused-cmake_parse_arguments-call branch from d0196af to e5992cc Compare September 22, 2025 12:23
@jcar87
Copy link
Contributor

jcar87 commented Sep 22, 2025

LGTM! Rebased on top of most recent develop2 to sync up with fixed github actions

@jcar87 jcar87 merged commit b522d5a into conan-io:develop2 Sep 22, 2025
5 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.

Malformed cmake_parse_arguments() at start of conan_install() function

3 participants