-
Notifications
You must be signed in to change notification settings - Fork 48
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
Migrate YCMBootstrap to use CMake's FetchContent #399
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main caveat I found on other projects that bootstrap similarly YCM is that if you use add_subdirectory
, all the targets of YCM are added to the the parent project. I saw that you just append the folders you need to the CMAKE_MODULE_PATH
variable, and it prevents what I just described.
Furthermore, this PR works fine unless YCM functionality is needed in the top-level CMake file. It does not seem to be the case though. Otherwise, you should have used PARENT_SCOPE
when extending CMAKE_MODULE_PATH
.
Beyond this clarification, I don't have any objections :)
The boostrap file is already included in the top-level CMake file, so I don't think it is need to specify the |
For some reason that I don't get the CI is not starting on this PR. |
This seems to be confirmed in the CI tests, in which the configuration time on Windows passed from ~1:20 seconds to ~55 seconds. |
It turns out that consuming |
Closing this PR as it is not possible to achieve this without modifying YCM itself. |
I actually did that in #1078 and it is working fine. |
The YCMBootstrap process has been historically quite error-prone. This PR substitute this process by a more straightforward use of the CMake's FetchContent repo.
With respect to the past, the main difference is the superbuild will not use exactly the same
YCM
downloaded inrobotology-superbuild/robotology/YCM
, as a private copy of YCM used only for the robotology-superbuild configuration will be downloaded inbuild/_deps/ycm-src
. However, as the new script still observers theYCM_TAG
variable, the version downloaded inbuild/_deps/ycm-src
will be exactly the same version of YCM that will be downloaded inrobotology-superbuild/robotology/YCM
.On my Windows machine, this decreases the time spent in the YCM bootstrap to ~5 seconds from ~20 seconds of the previous procedure.
If this new module will work fine, it will be proposed in YCM to fix robotology/ycm-cmake-modules#138 .