-
Notifications
You must be signed in to change notification settings - Fork 2.6k
CMake modernization #5284
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
CMake modernization #5284
Conversation
643676a to
0f3866d
Compare
tiennou
left a comment
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.
Kudos for this @pks-t, this looks awesome! I have only looked at diffs, made no runtime checks yet, and found a few things here and there.
| ENDIF () | ||
|
|
||
| # Collect sourcefiles | ||
| FILE(GLOB LIBGIT2_SOURCES |
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.
I want to point out that file(GLOB …) warns that :
If no CMakeLists.txt file changes when a source is added or removed then the generated build system cannot know when to ask CMake to regenerate.
This is somewhat unexpected, and sounds eerily like #4617, as the testsuite is also globbed. Maybe it would be worth to just flatten the list here ?
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 commit message is missing a word at the end of the 3rd line)
|
|
||
| # Add find modules to the path | ||
| LIST(APPEND CMAKE_MODULE_PATH ${libgit2_SOURCE_DIR}/cmake) | ||
| # Prepend our own modules to the path |
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.
I suspect something went wrong with the commit message here.
As we need to override some modules with our own version,
make sure we insert our custom location to CMake's module
path first.
src/CMakeLists.txt
Outdated
|
|
||
| Include(SelectHTTPSBackend) | ||
| LINK_DEPENDENCY(git2internal PUBLIC WinHTTP) | ||
| LINK_DEPENDENCY(git2internal PUBLIC HttpsBackend) |
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.
I may have missed the commit where LINK_DEPENDENCY was introduced…
| @@ -0,0 +1,4 @@ | |||
| FUNCTION(LINK_DEPENDENCY TARGET SCOPE DEPENDENCY) | |||
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.
Called it 😉.
| ADD_DEFINITIONS(-DGIT_DEPRECATE_HARD) | ||
| ENDIF() | ||
|
|
||
| # Installation paths |
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.
Does it matter if you move it higher up ? I kinda like that the whole option setup is around the top of the file, as a documentation source. Maybe gathering all known usable knobs would ease some issues ? That's completely cosmetic though, but given that CMake code has a tendency to melt make you doubt from where it's coming from. Here, I was kinda hoping those variables were global CMake variables we're defining for clarity, now I don't know 😉.
src/CMakeLists.txt
Outdated
|
|
||
| ADD_LIBRARY(git2internal OBJECT) | ||
| # This is a workaround for old CMake versions which didn't yet | ||
| # allow libraries without any source files. |
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.
I'm somewhat surprised you need more dummies, for platforms others than macOS — that workaround is part of #4439, and it's unknown if recent versions of the Xcode generator work without it.
src/CMakeLists.txt
Outdated
| SET_PROPERTY(TARGET ${TARGET} APPEND PROPERTY INTERFACE_LINK_LIBRARIES ${ARG}) | ||
| ENDFOREACH() | ||
| ELSE() | ||
| TARGET_LINK_LIBRARIES(${TARGET} ${SCOPE} ${ARGN}) |
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.
I don't understand where ${SCOPE} is coming from ?
src/CMakeLists.txt
Outdated
| ### Detect and link against dependencies | ||
| ### | ||
|
|
||
| FUNCTION(OBJECT_LIBRARY_LINK TARGET) |
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.
I don't like the OBJECT_LIBRARY_LINK name, as we already have too much TARGET LINK and LIBRARY around (after just having read about the new TARGET_LINK_OBJECTS 🤣). I'd recommend sticking closer to the known TARGET_LINK_LIBRARIES, so LG2_TARGET_LINK_LIBRARIES ?
a5ba6f5 to
79c273a
Compare
The cred.h header adds forwards declarations for libssh2 structures if libssh2 is not being used. The decision is based on whether or not LIBSSH2_VERSION is defined, which is in turn declared by <libssh2.h>. But as we never include this header, it is a matter of luck whether anybody else has included it transitively already. Fix the issue by including <libssh2.h> if GIT_SSH is defined.
Since ebabb88 (cmake: update minimum CMake version to v3.5.1, 2019-10-10), we have upgraded CMake to require at least v3.5.1. As the command `CMAKE_MINIMUM_VERSION(VERSION 3.5.1)` implicitly calls `CMAKE_POLICY(VERSION 3.5.1)`, all policies known until v3.5.1 get set to the new behaviour automatically. As the newest policy we have been setting to the new behaviour is policy 54, which was introduced in CMake 3.1, we can simply remove all our current calls to `CMAKE_POLICY`, as they are enabled anyway.
The CMake module CheckPrototypeDefiniton has been introduced in CMake v3.0, which is why we had to include a fallback copy of it in our own modules in case somebody was compiling libgit2 with an older version of CMake back when the minimum version was v2.8.11. As we have since raised the minimum version to v3.5.1, we can now remove the module from our own modules and simply always use the one provided by upstream.
All of our CMake modules are currently located in a directory "cmake/Modules". It is not expected that there will ever be any CMake files to distribute besides modules, which is why the "Modules" intermediate directory is completely useless. Move all modules directly into "cmake/".
Technically speaking, it is necessary to always set the minimum required CMake version first before calling any other function in a CMake project. Thus we should move the call to `CMAKE_MINIMUM_REQUIRED` before the call to `PROJECT`, especially so as `PROJECT` may use features of CMake which have been introduced with a certain version, only.
Currently, the project version is hidden inside of our "version.h" header file and CMake extracts required information out of that file via a regular expression. This is unwieldy and completely backwards, as it should be the CMake file that sets the project's version and then puts it into a configured file. Move version information into the call to `PROJECT` and use `CONFIGURE_FILE` to configure the "version.h.in" header. The configured header will now be configured into a "include/git2/" directory together with the "features.h" header and then finally gets installed into the target directory. This also removes the awful hack of directly adding our binary directory into the include paths, as there is now a subdirectory "include" in our binary directory for configured headers.
While historic CMake code usually worked a lot with instructions that modified global state, modern style works via targets. Thus instead of first setting up the state before adding the actual target, which made sense from a logical viewpoint with previous style, doesn't make a lot of sense anymore for the new style. Instead, one would now typically first declare the targets and then afterwards call functions to modify these targets. We're on the verge to migrate to this modern style of CMake. As a first preparatory step, let's declare the library targets up front and set up sources via `TARGET_SOURCES` instead of first gathering it in a global variable. As this function has been introduced in CMake v3.1, we're good to use it.
The previous commit already started by necessity to use `TARGET_SOURCES`
to set up the library's source files, as we started declaring the
library target up-front without any added sources yet. We can make our
easier to grasp though by going all-in with `TARGET_SOURCES`, thus
replacing use of all these variables like ${SRC_SSH} or ${SRC_SHA1} and
instead directly adding them in-place to the respective target. This not
only makes code easier to understand, but also makes sure that we cannot
forget to include any of these variables.
The libgit2 library has quite a lot of different dependencies that may or may not be optional. Currently, the dependency handling code is a _huge_ mess which nobody is able to grasp at all. Most of it is currently done in the CMake build instructions of the source directory, but they are split up to multiple locations, use global tracking via weird variables and generally have been a source for confusion. Modern CMake actually helps to sort out this mess by using targets all the way through. Instead of manually tracking include directories, link directories and flags, we can just set up a target for each dependency that is completely self contained and has all information required to link against it. Introduce a new helper function `LINK_DEPENDENCY` that aims to help migrating towards that target. The assumption is quite simple: for each dependency, there will be a "cmake/DependencyFoo.cmake" file that gets included by the helper. This dependency file is expected to _always_ export a target, even for dependencies which are not going to be used. Thus, in the case where a dependency is found, the module should export a normal target that contains linking and preprocessor information. In the case where no dependency is found or in the case where it is not to be used, the module is expected to export an empty target that simply does nothing. Like this, every submodule can be completely self-contained and we do not need to treat them specially in the main build module.
Now that all of our dependencies have been migrated to use targets, we do not need to set up include or link directories in any of our dependents anymore. Instead, we just let them link against the git2internal target, which includes all necessary information about necessary build setup of itself and of its transitive dependencies. As such, we can get rid of the respective instructions in our examples, fuzzers and test code and also of the global lists LIBGIT2_OBJECTS, LIBGIT2_INCLUDES, LIBGIT2_SYSTEM_INCLUDES and LIBGIT2_LIBS.
This is a final cleanup step that shifts around various bits in our library's CMake build instruction to group them together in a logical way.
79c273a to
779a660
Compare
|
Closing as I've chosen a more iterative approach |
This morning, I set out to do some small cleanups for our CMake code. Naturally, it turned into a full-blown refactoring doing many things I didn't intend in the beginning. But if you ask me, the results are hugely beneficial and make our build code much much easier to read, self contained and correct.
So what changed? I've mostly modified all of our code to make heavy use of library targets. So instead of globally setting up required build options, link options are tied to each of these targets and will get applied when using
TARGET_LINK_LIBRARIES, only.Unfortunately, I realized later on that my code currently depends on features introduced in CMake v3.12, only. Most importantly,
TARGET_LINK_LIBRARIESdidn't work with object libraries before that, breaking one of my main assumptions. Thus CI builds will probably fail, too. I'm quite sure that I'm actually able to work around that issue on older versions, though, but I first wanted to get some comments.As usual, I highly recommend to go through commit-wise. I think otherwise, you'll quickly get lost due to the huge diff.