Conversation
pks-t
force-pushed
the
pks/cmake-cleanups
branch
3 times, most recently
from
63802fa to
b639d34
Compare
src/CMakeLists.txt
Outdated
|
|
||
| CONFIGURE_FILE(features.h.in git2/sys/features.h) | ||
| # Generate feature headers | ||
| CONFIGURE_FILE(features.h.in ${libgit2_BINARY_DIR}/include/git2/sys/features.h) |
There was a problem hiding this comment.
features.h is actually not installed at present, this is something that's only built and used locally. Let's keep it private API without a compelling reason to promote it. (I think that it was intended to be public, once, but now that it's not, I have second thoughts about making it so.)
There was a problem hiding this comment.
Fair enough, makes sense 👍
src/CMakeLists.txt
Outdated
| CONFIGURE_FILE(features.h.in git2/sys/features.h) | ||
| # Generate feature headers | ||
| CONFIGURE_FILE(features.h.in ${libgit2_BINARY_DIR}/include/git2/sys/features.h) | ||
| CONFIGURE_FILE(version.h.in ${libgit2_BINARY_DIR}/include/git2/version.h) |
There was a problem hiding this comment.
Generating this file will mean that people must use CMake to build us. At the moment, at least TortoiseGit uses a build step for us that doesn't involve CMake.
I think that they include us as a submodule. Then they build us by defining the necessary things (eg -DGIT_TRACE=1) directly. This, and the git2.rc changes will definitely break their ability to do this.
What's the motivation for defining the version in cmake instead of version.h?
There was a problem hiding this comment.
The perl bindings also don't use CMake to build the library.
There was a problem hiding this comment.
@ethomson The motivation is so that we can use it for the PROJECT() setup function. I'm not sure whether you're allowed to do things like extracting the version previous to doing the PROJECT() call, but we should definitely put the version in there. I'll see whether it's possible to do so with our current version.h regex.
There was a problem hiding this comment.
Forcing users to use CMake isn't a good idea.
|
I've removed the version thing for now and will move it into a separate PR so that we should be left with the non-controversial bits, only. |
|
Rebased to fix conflicts. |
Our custom CMake module currently live in "cmake/Modules". As the "cmake/" directory doesn't contain anything except the "Modules" directory, it doesn't really make sense to have the additional intermediate directory. So let's instead move the modules one level up into the "cmake/" top level directory.
We currently have support for generating tags via ctags as part of our build system. We aren't really in the place of supporting any tooling that exists apart from the actual build environment, as doing so adds additional complexity and maintenance burden to our build instructions. This is in fact nicely demonstrated by this particular option, as it hasn't been working anymore since commit e5c9723 (cmake: move library build instructions into subdirectory, 2017-06-30). As a result, this commit removes support for building CTags
We currently have an option that adds options for profiling to both our CFLAGS and LDFLAGS. Having such flags behind various build options is not really sensible at all, since users should instead set up those flags via environment variables supported by CMake itself. Let's remove this option.
The `CMAKE_MINIUM_REQUIRE()` function not only sets up the minimum required CMake version of a project, but it will also at the same time set the CMake policy version. In effect this means that all policies that have been introduced before the minimum CMake version will be enabled automatically. When updating our minimum required version ebabb88 (cmake: update minimum CMake version to v3.5.1, 2019-10-10), we didn't remove any of the policies we've been manually enabling. The newest CMake policy we've been enabling is CMP0054, which was introduced back in CMake v3.1. As a result, we can now just remove all manual calls to `CMAKE_POLICY()`.
We currently disable deprecation synchronization warnings in case we're building with Clang. We check for Clang by doing a string comparison on the compiler identification, but this seems to have been broken by an update in macOS' image as the compiler ID has changed to "AppleClang". Let's just unconditionally disable this warning on Unix platforms. We never add the deprecated attribute anyway, so the warning doesn't help us at all.
5 participants
While I've outlined my greater vision of where our CMake setup should move in #5284, I've realized it's much too big to review it properly and thus decided to take a more iterative approach to modernizing our CMake build instructions. This is the first set of commits towards that goal and an initial spring cleaning. All changes should probably be uncontroversional and make sense on their own.