Skip to content

Set RULE_LAUNCH_COMPILE/LINK to ccache if CMAKE_VERSION < 3.4 #2738

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

furushchev
Copy link
Member

Since the use of RULE_LAUNCH_COMPILE / RULE_LAUNCH_LINK is not recommended at least from CMake version 3.4, I added a condition to use set_property only if the cmake version is more than or equal to 3.4, where the variable CXX_COMPILE_LAUNCHER is introduced.

ref:
https://cmake.org/cmake/help/latest/prop_gbl/RULE_LAUNCH_COMPILE.html
https://cmake.org/cmake/help/latest/prop_tgt/LANG_COMPILER_LAUNCHER.html#prop_tgt:%3CLANG%3E_COMPILER_LAUNCHER

@furushchev furushchev changed the title set RULE_LAUNCH_COMPILE/LINK to ccache if CMAKE_VERSION < 3.4 Set RULE_LAUNCH_COMPILE/LINK to ccache if CMAKE_VERSION < 3.4 Nov 2, 2022
@mqcmd196
Copy link
Member

Thank you for your contribution. I think the approach like https://github.com/apache/tvm/pull/8373/files , adding CXX_COMPILER_LAUNCHER is better instead of removing ccache flag if CMake >= 3.4. What do you think?

@furushchev
Copy link
Member Author

furushchev commented Dec 26, 2022

@mqcmd196 Thank you for the reply.

adding CXX_COMPILER_LAUNCHER is better instead of removing ccache flag

This is for sure another way for the update, however it overwrites the variable to ccache and affects all packages that depend on the packages in this repository.
As we can specify the default arguments on running cmake internally in catkin build command by setting, for instance, catkin config --cmake-args -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DCMAKE_C_COMPILER_LAUNCHER=ccache, I think it's nice to leave the CXX_COMPILER_LAUNCHER undefined in CMakeLists.txt and configurable by developers.

@mqcmd196
Copy link
Member

I see, so you mean that advanced users who want to use ccache can specify ccache on the command line themselves. And this PR only guarantees backward compatibility, right?

@furushchev
Copy link
Member Author

I see, so you mean that advanced users who want to use ccache can specify ccache on the command line themselves. And this PR only guarantees backward compatibility, right?

Yes. If you think this change is inconvenient for the existing users, it's also a good idea to add a variable USE_CCACHE to switch the prefix as seen in the link you shared.

Copy link
Member

@mqcmd196 mqcmd196 left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants