Skip to content

Conversation

@pks-t
Copy link
Member

@pks-t pks-t commented Oct 25, 2019

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_LIBRARIES didn'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.

@pks-t pks-t force-pushed the pks/cmake-modernization branch 4 times, most recently from 643676a to 0f3866d Compare October 26, 2019 09:57
Copy link
Contributor

@tiennou tiennou left a 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
Copy link
Contributor

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 ?

Copy link
Contributor

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
Copy link
Contributor

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.


Include(SelectHTTPSBackend)
LINK_DEPENDENCY(git2internal PUBLIC WinHTTP)
LINK_DEPENDENCY(git2internal PUBLIC HttpsBackend)
Copy link
Contributor

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)
Copy link
Contributor

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
Copy link
Contributor

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 😉.


ADD_LIBRARY(git2internal OBJECT)
# This is a workaround for old CMake versions which didn't yet
# allow libraries without any source files.
Copy link
Contributor

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.

SET_PROPERTY(TARGET ${TARGET} APPEND PROPERTY INTERFACE_LINK_LIBRARIES ${ARG})
ENDFOREACH()
ELSE()
TARGET_LINK_LIBRARIES(${TARGET} ${SCOPE} ${ARGN})
Copy link
Contributor

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 ?

### Detect and link against dependencies
###

FUNCTION(OBJECT_LIBRARY_LINK TARGET)
Copy link
Contributor

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 ?

@pks-t pks-t force-pushed the pks/cmake-modernization branch 2 times, most recently from a5ba6f5 to 79c273a Compare November 9, 2019 14:04
pks-t added 21 commits November 9, 2019 15:37
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.
@pks-t pks-t force-pushed the pks/cmake-modernization branch from 79c273a to 779a660 Compare November 9, 2019 21:41
@pks-t pks-t mentioned this pull request Apr 3, 2020
@pks-t
Copy link
Member Author

pks-t commented Jun 17, 2020

Closing as I've chosen a more iterative approach

@pks-t pks-t closed this Jun 17, 2020
@pks-t pks-t deleted the pks/cmake-modernization branch June 17, 2020 16:09
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.

2 participants