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

Issue on cmake_install.cmake with uninstall.cmake #617

Closed
olivier-stasse opened this issue Sep 4, 2023 · 5 comments
Closed

Issue on cmake_install.cmake with uninstall.cmake #617

olivier-stasse opened this issue Sep 4, 2023 · 5 comments

Comments

@olivier-stasse
Copy link
Member

Steps to reproduce the problem

Let us use a package using jrl-cmake modules as a cmake git submodule, for instance master_board_sdk, and a ros humble distro.

mkdir -p /tmp/test_ws/src
cd /tmp/test_ws/src
git clone https://github.com/open-dynamic-robot-initiative/master-board.git --recursive
cd ..
colcon build --packages-select master_board_sdk --executor sequential

Compiling is ok, and the installation is going fine, but the output displays this :

3 warnings generated.


Usage: cmake --build <dir>             [options] [-- [native-options]]
       cmake --build --preset <preset> [options] [-- [native-options]]
Options:
  <dir>          = Project binary directory to be built.
  --preset <preset>, --preset=<preset>
                 = Specify a build preset.
  --list-presets[=<type>]
                 = List available build presets.
  --parallel [<jobs>], -j [<jobs>]
                 = Build in parallel using the given number of jobs. 
                   If <jobs> is omitted the native build tool's 
                   default number is used.
                   The CMAKE_BUILD_PARALLEL_LEVEL environment variable
                   specifies a default parallel level when this option
                   is not given.
  -t <tgt>..., --target <tgt>...
                 = Build <tgt> instead of default targets.
  --config <cfg> = For multi-configuration tools, choose <cfg>.
  --clean-first  = Build target 'clean' first, then build.
                   (To clean only, use --target 'clean'.)
  --resolve-package-references={on|only|off}
                 = Restore/resolve package references during build.
  -v, --verbose  = Enable verbose output - if supported - including
                   the build commands to be executed. 
  --             = Pass remaining options to the native tool.
---
Finished <<< master_board_sdk [1min 23s]

Analysis

The error :

CMake Error: Invalid value used with --config

disappear if one comments the line

execute_process(COMMAND "/Volumes/Devel/miniconda_env/miniconda3_env/envs/ros_humble_dev_p_3_10_env/bin/cmake" --build "" --config ${CMAKE_INSTALL_CONFIG_NAME} --target uninstall)

in ./build/master_board_sdk/cmake_install.cmake

It seems to be generated by the file uninstall.cmake and more specifically

CODE "execute_process(COMMAND \"${CMAKE_COMMAND}\" --build \"${PROJECT_BINARY_DIR}\" --config \${CMAKE_INSTALL_CONFIG_NAME} --target uninstall)"

When not specifying a CMAKE_BUILD_TYPE this line fails.

I have a fix here:
https://github.com/olivier-stasse/jrl-cmakemodules/blob/cf58b6616b8c28e337d1b55fd3a1e4708b6711d0/uninstall.cmake#L55

Basically if the configuration is empty, the line is not called.
Maybe it would be better to not specify the --config option if the variable is empty.

WDYT ?

@gergondet
Copy link
Member

Hi @olivier-stasse

Your suggested patch would disable the auto-uninstall feature in this scenario.

I suggest (not tried):

if(DEFINED CMAKE_CONFIGURATION_TYPES)
  set(UNINSTALL_CONFIG_ARG "--config \${CMAKE_INSTALL_CONFIG_NAME}")
endif()
install(
  CODE "execute_process(COMMAND \"${CMAKE_COMMAND}\" --build \"${PROJECT_BINARY_DIR}\" ${UNINSTALL_CONFIG_ARG} --target uninstall)"
)

Since the --config is only really necessary with multi-config generators (i.e. those that have CMAKE_CONFIGURATION_TYPES defined) and can be omitted otherwise. In a multi-config generator I don't think it's possible to get an empty CMAKE_INSTALL_CONFIG_NAME

@olivier-stasse
Copy link
Member Author

I will try and keep you posted.

@olivier-stasse
Copy link
Member Author

Thanks this is working.

@gergondet
Copy link
Member

I'm closing since this is fixed in #618

@olivier-stasse
Copy link
Member Author

olivier-stasse commented Sep 5, 2023

Yes. Thx. sorry I should have close it.

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

No branches or pull requests

2 participants