Skip to content
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

Introduce CMake (and removing all other project-related code) #7270

Open
wants to merge 8 commits into
base: master
from

Conversation

@TrueBrain
Copy link
Member

TrueBrain commented Feb 23, 2019

Fixes #6884
Fixes #7470

Stable release build
Nightly release build

This removes:

  • MSVC project files
  • Kdevelop generation files
  • configure / Makefile

It replaces them all with a single method: cmake. This should also remove any awk/vbs scripts we have around, meaning we will get a single method of doing stuff.

Currently tested with these 'IDEs':

  • VS2017 (yes, VS supports CMake)
  • VS2019 (yes, VS2019 supports CMake even better)
  • CMake, WSL Ubuntu 18.04

This is pretty much a Work In Progress, but I wanted to show others how it would look etc, so they can also test already a bit. Any feedback is welcome.

To test it out:

mkdir build
cd build
cmake ..
make

Or open the CMakeLists.txt in VS2017 / VS2019.

Things still to do:

  • Debian dependencies
  • rpm support
  • Linux install: menu entries
  • check if we are autodetecting everything
  • check all TODOs, and make sure they are fixed
  • validate this works on all current machines of all people that can compile OpenTTD now
  • validate this works on things like CLion, Visual Code Studio, Kdevelop, ..
  • rewrite Squirrel generation in CMake (done in a branch)
  • add the ability to make stable builds (NO_ASSERT, see MSVC changes for stable releases) via parameters
  • autodetect nforenum
  • autodetect grfcodec
  • add ICC support
  • add all the warning flags etc
  • add debug options
  • rewrite bundling code in CMake
  • rewrite baseset generation in CMake
  • rewrite regression in CMake
  • rewrite source.list in CMake
  • autodetect ICU
  • autodetect TiMiditity (libtimidity is no longer available on most OSes)
  • autodetect xdg-basedir
  • autodetect iconv
  • autodetect OSX stuff (Cocoa QuickDraw, Quartz, libraries, etc)
  • rewrite findversion in CMake (partially done here: LordAro@1ea407b)
    Currently it is calling findversion.sh, which only works on non-Windows. Porting it to CMake makes more sense (one single way of doing stuff).
  • add static builds (no static builds for now; we might reconsider later)
  • add CMake support for all CI targets
  • revisit dependencies for Dedicated Server (does it need SDL, Cocoa, ..)
  • fix CI for Linux targets
  • fix CI for Windows
  • fix CI for OSX
  • fix mingw
  • fix dedicated doesn't need a video driver
  • split CMakeLists.txt inside several files to increase readability
  • remove all old project files (including configure, Makefile, projects/, etc)
  • restore projects\os_versions.manifest usage in VS builds

(I am sure I am missing stuff .. but this is something to work on at least :D)

@TrueBrain TrueBrain force-pushed the TrueBrain:cmake branch from 2a97e01 to f6cd1ad Feb 23, 2019
CMakeLists.txt Outdated Show resolved Hide resolved
@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Feb 24, 2019

If CMake becomes the only (meta) build system, wouldn't it make sense to scrap the source.list file as well, and define that data via CMake syntax instead?

@TrueBrain TrueBrain force-pushed the TrueBrain:cmake branch from cfff2fc to a891bd6 Feb 24, 2019
@glx22

This comment has been minimized.

Copy link
Contributor

glx22 commented Feb 24, 2019

For MinGW directmusic and xaudio2 needs to be detected. MinGW has directmusic headers, but doesn't have xaudio2.

@TrueBrain

This comment has been minimized.

Copy link
Member Author

TrueBrain commented Feb 24, 2019

Added both @nielsmh and @glx22 points to the list at the top.

@orudge

This comment has been minimized.

Copy link
Contributor

orudge commented Feb 24, 2019

For MinGW directmusic and xaudio2 needs to be detected. MinGW has directmusic headers, but doesn't have xaudio2.

Just to clarify, if you manually copy over an xaudio2 header file then mingw should be happy with it - I did test that when I wrote it.

src/os/windows/ottdres.rc.in Outdated Show resolved Hide resolved
@glx22

This comment has been minimized.

Copy link
Contributor

glx22 commented Feb 26, 2019

I kept the manifest in projects, but it should probably move somewhere else

@glx22

This comment has been minimized.

Copy link
Contributor

glx22 commented Feb 27, 2019

For now we rely on cmake default CMAKE_CONFIGURATION_TYPES and CMAKE_XXXXX_FLAGS_<CONFIG>, but it should be better to manage that by ourselves

src/os/windows/ottdres.rc.in Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@TrueBrain

This comment has been minimized.

Copy link
Member Author

TrueBrain commented Feb 27, 2019

For now we rely on cmake default CMAKE_CONFIGURATION_TYPES and CMAKE_XXXXX_FLAGS_, but it should be better to manage that by ourselves

I disagree. The closer we stay to CMake, the easier our life will be. Let's try to do this, and only add exceptions if we really need to (and mark them as such). Otherwise we will have a config.lib in no time :D

@glx22

This comment has been minimized.

Copy link
Contributor

glx22 commented Feb 27, 2019

Indeed something is wrong in the generated project file for lzo and lzma
AdditionalDependencies for Release:
D:\developpement\GitHub\vcpkg\installed\x64-windows-static\lib\libpng16.lib
D:\developpement\GitHub\vcpkg\installed\x64-windows-static\lib\zlib.lib
D:\developpement\GitHub\vcpkg\installed\x64-windows-static\debug\lib\lzma.lib
D:\developpement\GitHub\vcpkg\installed\x64-windows-static\debug\lib\lzo2.lib
D:\developpement\GitHub\vcpkg\installed\x64-windows-static\lib\freetype.lib
D:\developpement\GitHub\vcpkg\installed\x64-windows-static\lib\bz2.lib

AdditionalDependencies for Debug:
D:\developpement\GitHub\vcpkg\installed\x64-windows-static\debug\lib\libpng16d.lib
D:\developpement\GitHub\vcpkg\installed\x64-windows-static\debug\lib\zlibd.lib
D:\developpement\GitHub\vcpkg\installed\x64-windows-static\debug\lib\lzma.lib
D:\developpement\GitHub\vcpkg\installed\x64-windows-static\debug\lib\lzo2.lib
D:\developpement\GitHub\vcpkg\installed\x64-windows-static\debug\lib\freetyped.lib
D:\developpement\GitHub\vcpkg\installed\x64-windows-static\debug\lib\bz2d.lib

@glx22

This comment has been minimized.

Copy link
Contributor

glx22 commented Mar 1, 2019

hmm that's weird, OSX cmake step now pass, lzo is found, compilation fails because it doesn't find an lzo header

@glx22

This comment has been minimized.

Copy link
Contributor

glx22 commented Mar 1, 2019

Ok there's an issue with lzo detection, I can compile with MinGW by luck, as the detected

-- LZO_INCLUDE_DIRS:H:/msys64/mingw64/include/lzo

is covered by H:/msys64/mingw64/include from other libs.
But on OSX

-- LZO_INCLUDE_DIRS: /usr/local/Cellar/lzo/2.10/include/lzo

is alone

@TrueBrain TrueBrain force-pushed the TrueBrain:cmake branch 2 times, most recently from 6164e63 to a25c911 Mar 2, 2019
@glx22

This comment has been minimized.

Copy link
Contributor

glx22 commented Mar 3, 2019

Stupid me failing in the commit message.

BTW I think test task for MacOS should probably be "make -C build test".

@TrueBrain TrueBrain force-pushed the TrueBrain:cmake branch 3 times, most recently from e5b7c05 to d71ac0a Mar 10, 2019
cmake/AutodetectSSE.cmake Outdated Show resolved Hide resolved
@glx22

This comment has been minimized.

Copy link
Contributor

glx22 commented Mar 12, 2019

regression works with MinGW without converting the exe, but it fails with many

6: -  SetCommandDelay: (null : 0x00000000)
6: +  SetCommandDelay: (null : 0x0000000000000000)'

I guess the lovely regex was needed :)

@TrueBrain

This comment has been minimized.

Copy link
Member Author

TrueBrain commented Mar 13, 2019

Yeah, but I refused to do that unreadable regex; so now I am just adding them one case at the time :D

@TrueBrain TrueBrain force-pushed the TrueBrain:cmake branch 5 times, most recently from 823f76d to 027a5b3 Mar 13, 2019
@glx22 glx22 force-pushed the TrueBrain:cmake branch 2 times, most recently from 3d63911 to 9414548 Apr 29, 2019
@glx22 glx22 force-pushed the TrueBrain:cmake branch from 9414548 to 6dd3e6b May 10, 2019
@glx22 glx22 force-pushed the TrueBrain:cmake branch 2 times, most recently from 87f9307 to eed958c May 24, 2019
@glx22

This comment has been minimized.

Copy link
Contributor

glx22 commented May 24, 2019

When doing the rebase I decided to conditionally exclude some library detection for windows as FreeType takes precedence over GDI, and its development files can be present on the system even if we don't want to use it.

@glx22 glx22 force-pushed the TrueBrain:cmake branch from eed958c to c255b8a Jul 6, 2019
@glx22 glx22 force-pushed the TrueBrain:cmake branch 2 times, most recently from 208df51 to 995bf50 Jul 18, 2019
@fsimonis

This comment has been minimized.

Copy link

fsimonis commented Jul 19, 2019

I may be a bit late to the party, but I started a CMake port of OpenTTD about one year ago. After reading his Trello board, I attempted to contact @TrueBrain back then via Gitter to join forces, but that sadly never happened. You can find my version on my fork.
It runs on VS and various Linux distros. I couldn't test on Mac due to a lack of hardware.

I stopped the port when I hit the wall of supporting all the OSes. Some of which CMake isn't even aware of.

Some lessons I have learned and general feedback to this port:

  1. autodetection of libraries is a total nightmare for package maintainers as one never really knows what the actual configuration looks like. Also, the maintainer won't see errors when the configuration did not find all expected dependencies.
    The clean way to handle this in CMake is to use options for every dependency (e.g. OPENTTD_ALLEGRO), always require the enabled ones and enable only the most general/cross-platform libraries by default.
    One can then use the CMake option to pre-populate the cache to load a configuration for a specific OS cmake -C linux-ubuntu-1804.cmake. This makes it also easier to keep the dependencies in sync with package dependencies such as Debian. Most importantly: this solution is super clean!

  2. As OpenTTD is a distributed executable, there is no reason to stick to an old CMake version. Installing CMake is as trivial as downloading, extracting an archive and maybe extending PATH. The end-user will never need to install CMake anyhow.
    Staying up2date has 3 major upsides:

    1. The FindModules are up2date and mostly provide targets. (You can find the missing modules on my fork)
    2. The general support for targets is better.
    3. The support for installing various file types is far better via the TYPE argument of install.
  3. Using targets to link dependencies allows to write sane CMake scripts. Everything else just silently fails and is a nightmare to debug.

  4. The third-party libraries squirrel, squirrel-stl, os2, and md5 can be separate targets. (The common string dependency is a nuisance, but not really a problem.)

  5. Prefixing all project-related options and cache variables with OPENTTD_ makes them easier to find/grep, will neatly align them in ccmake and group them in cmake-gui.

Thank you for doing this port!
The build system and SVN were the two reasons that always kept me from contributing to this project!

@DerDakon

This comment has been minimized.

Copy link

DerDakon commented Jul 19, 2019

Regarding your first concern, I suggest having a look at cmake --help-module FeatureSummary. I would very much suggest not to maintain a list of preconfigured caches. Either CMake is able to detect everything from distro packages, or you file a bug against CMake. Everything else just papers over existing bugs.

I agree on 2 and 3, and have no opinion about the others.

bundle/*
bundles/*
docs/aidocs/*
docs/gamedocs/*
docs/source/*
.kdev4
.kdev4/*
*.kdev4

This comment has been minimized.

Copy link
@DerDakon

DerDakon Jul 19, 2019

I would keep at least *.kdev4, this would still be relevant when importing the CMake project into KDevelop.

This comment has been minimized.

Copy link
@glx22

glx22 Jul 19, 2019

Contributor

That can be handle in the local gitignore. I see no real need to put it globally.

@@ -0,0 +1,12 @@
# Make the current version available to CPack
set(CPACK_PACKAGE_VERSION "@REV_VERSION@")

This comment has been minimized.

Copy link
@DerDakon

DerDakon Jul 19, 2019

It should be enough if you call project(OpenTTD VERSION 5.6.7) at the top level, that would make the variable OpenTTD_VERSION available, also to CPack.

This comment has been minimized.

Copy link
@fsimonis

fsimonis Aug 14, 2019

The REV_VERSION is based on the git tag(project version) or the git commit and can change between builds.
This is not pretty but necessary to make CPack behave correctly.

This comment has been minimized.

Copy link
@DerDakon

DerDakon Aug 14, 2019

Please not that this will usually not change between builds: CMake will not notice if your git status has changed, so it will not rerun CMake, so it will not rerun your execute_process().

# autodetect_editbin()
#
function(autodetect_editbin)
if (DEFINED EDITBIN_FOUND)

This comment has been minimized.

Copy link
@DerDakon

DerDakon Jul 19, 2019

You could just check on EDITBIN_EXECUTABLE here, CMake will automatically put that into cache in find_program(). And apart from the messages you do not need to guard this, if the variable is already set find_program() will just do nothing.

This comment has been minimized.

Copy link
@fsimonis

fsimonis Jul 22, 2019

We could refactor this entire function into very small and simple find module. Then one could use the more CMakeish find_package(EDITBIN).
You can have a look at the FindCCACHE on my fork for a minimal example.
This also keeps the error messages consistent with other find modules.

message(STATUS "Detecting editbin - found")

# Make sure these values are cached properly
set(EDITBIN_FOUND YES CACHE INTERNAL "")

This comment has been minimized.

Copy link
@DerDakon

DerDakon Jul 19, 2019

If you want to have this variable after all simply not cache it: the find_program() call will do nothing, EDITBIN_EXECUTABLE is set, so this variable get's set to YES anyway. No need to clutter the cache with it.


# Check if any of the argument is ENCOURAGED
list(FIND args ENCOURAGED ENCOURAGED)
if (ENCOURAGED LESS 0)

This comment has been minimized.

Copy link
@DerDakon

DerDakon Jul 19, 2019

if("ENCOURAGED" IN_LIST args)
set(DEFAULT_SHARED_DIR "(not set)")
set(DEFAULT_GLOBAL_DIR "${CMAKE_INSTALL_PREFIX}/share/games/openttd")
else ()
message(FATAL_ERROR "Unknown OS found; please consider creating a Pull Request to add support for this OS.")

This comment has been minimized.

Copy link
@DerDakon

DerDakon Jul 19, 2019

Again, just leave it as is. Let the *BSD folks do their experiment…

endif (NOT (${PARAM_CONDITION}))
endif (PARAM_CONDITION)

foreach(FILE ${PARAM_FILES})

This comment has been minimized.

Copy link
@DerDakon
set(PARAM_FILES "${PARAM_UNPARSED_ARGUMENTS}")

if (PARAM_CONDITION)
if (NOT (${PARAM_CONDITION}))

This comment has been minimized.

Copy link
@DerDakon

DerDakon Jul 19, 2019

Why this? I don't think you can ever reach the inner return() as an empty variable is just false.


get_property(SOURCE_PROPERTIES GLOBAL PROPERTY source_properties)

foreach(FILE ${PARAM_FILES})

This comment has been minimized.

Copy link
@DerDakon

if (CMAKE_SOURCE_DIR STREQUAL CMAKE_BINARY_DIR)
message(FATAL_ERROR "In-source builds not allowed. Please run \"cmake ..\" from the bin directory")
endif (CMAKE_SOURCE_DIR STREQUAL CMAKE_BINARY_DIR)

This comment has been minimized.

Copy link
@DerDakon

DerDakon Jul 19, 2019

I probably have said this before, but just remove the arguments to all end*() calls. It makes code much easier to maintain.

@fsimonis

This comment has been minimized.

Copy link

fsimonis commented Jul 22, 2019

@DerDakon I have the feeling that we are talking past each other.
I am talking about using the precaches solely for explicitly en/disabling features. This matter is orthogonal to how those features are detected. I totally agree that using the precache to influence library detection is a very very bad idea.

Explicit dependencies vs autodetection

My actual concern is that we should use explicit bindings options for dependencies. Autodetecting every library makes life more difficult for packagers and developer.

Here my reasoning:

  1. With hard autodetection, CMake will always configure the version with the most detected libraries.
  2. CMake will configure successfully even if an installed library was not found. A developer thus always has to check the logs to validate that all available libraries were detected as expected.
  3. As a developer you may want to debug a configuration with a specific feature-set to reproduce or locate a bug. Hard autodetection makes this impossible.
  4. In CI you may want to run tests on OpenTTD with a fixed feature set, such as only a single video and audio driver enabled or even multiple configuration matrices.

I propose to do the following:

  1. Add a boolean option for every external library.
  2. Explain why we need it. (@DerDakon Thanks, I was not aware of this module!)
  3. Always fail if the library was not found.

Example for zlib:

# Options
option(OPENTTD_ZLIB "Enable ZLib" ON`)
add_feature_info(ZLib OPENTTD_ZLIB "Required for (de)compressing of old (0.3.0-1.0.5) savegames, content downloads, and heightmaps.")

# ...

# Package discovery
if(OPENTTD_ZLIB)
  find_package(ZLIB REQUIRED)
endif()

# ...

Running CMake without a preset of features will likely fail during the first configuration attempt, but then provide a full list of options to ccmake/cmake-gui etc.

Feature set using pre-cache population

Example: lets consider the different compression libraries used in OpenTTD. Then the options and the defaults for those features could look as follows:

Library CMake Option Default
zlib OPENTTD_ZLIB ON
LIBLZO2 OPENTTD_LIBLZO2 ON
libzlma OPENTTD_LIBZLMA ON

So for Ubuntu 18.04 we know that all libraries are available, thus we could explicitly enable them as such:

# linux-ubuntu-1804.cmake
set(OPENTTD_ZLIB ON CACHE BOOLEAN "")
set(OPENTTD_LIBLZO2 ON CACHE BOOLEAN "")
set(OPENTTD_LIBZLMA ON CACHE BOOLEAN "")

On some other system, liblzio2 may not be available, thus we could explicitly disable it:

# some-other-system.cmake
set(OPENTTD_ZLIB ON CACHE BOOLEAN "")
set(OPENTTD_LIBLZO2 OFF CACHE BOOLEAN "")
set(OPENTTD_LIBZLMA ON CACHE BOOLEAN "")

This way, the CI configuration for a specific distro boils down to: cmake -C $SRC/.../linux-ubuntu-1804.cmake $SRC.

@DerDakon

This comment has been minimized.

Copy link

DerDakon commented Jul 22, 2019

I would use options for feature sets, and based on that require on or another library. Even if Ubuntu has Lzo2, it doesn't mean that the user has it installed. What you could do is to give the user a list of packages for a given distro that should ideally be installed.

@fsimonis

This comment has been minimized.

Copy link

fsimonis commented Aug 10, 2019

So a possible feature-set based on the README would look like this:

Feature option Required libraries
OPENTTD_BACKEND_SDL libsdl
OPENTTD_BACKEND_ALLEGRO liballegro
OPENTTD_SAVEGAME_ANCIENT liblzo2
OPENTTD_SAVEGAME_OLD zlib
OPENTTD_SAVEGAME liblzma
OPENTTD_CONTENT_DOWNLOAD zlib
OPENTTD_HEIGHTMAP zlib, libpng
OPENTTD_SCREENSHOT libpng
OPENTTD_FONTS libfreetype, libfontconfig
OPENTTD_RIGHT_TO_LEFT libico

@DerDakon Is that what you had in mind?

@DerDakon

This comment has been minimized.

Copy link

DerDakon commented Aug 10, 2019

Basically. And the code for that could look like this:

unset(zlib_required)
option(OPENTTD_SAVEGAME_OLD "foo" Off)
option(OPENTTD_HEIGHTMAP "bar" On)

if (OPENTTD_HEIGHTMAP OR OPENTTD_SAVEGAME_OLD OR …)
  set(zlib_required "REQUIRED")
endif()

find_package(ZLIB ${zlib_required})
@glx22

This comment has been minimized.

Copy link
Contributor

glx22 commented Sep 20, 2019

I think I properly handled integration of 2d27e8e and c655f89

@glx22 glx22 force-pushed the TrueBrain:cmake branch from 6f09f68 to 6575eeb Nov 3, 2019
TrueBrain added 8 commits Apr 7, 2019
This prepares for the switch to CMake, which takes over all current
exisiting forms of project files.
… script

The tst_stationlist savegame had to be changed to start the correct
AI. In the old setup, all regression AIs had the same name, which
made it impossible to run both regressions in parallel. With the new
setup this is possible.

Although all files are available to run the regression, it won't
really work till CMake is introduced (which will happen in a few
commits from here)
For grfs, it now uses CMake scripts to do its job, and both grf
files are split into their own folder to make more clear what is
going on. Additionally, it no longer builds in-source (although the
resulting grf is copied back in the source folder).

For ob[msg] it now uses CMake scripts to generate the translation
files; the result is no longer stored in-source (but in the build
folder).

Although all files are available to create the GRFs and basesets, it
won't really work till CMake is introduced (which will happen in a
few commits from here)
CMake works on all our supported platforms, like MSVC, Mingw, GCC,
Clang, and many more. It allows for a single way of doing things,
so no longer we need shell scripts and vbs scripts to work on all
our supported platforms.

Additionally, CMake allows to generate project files for like MSVC,
KDevelop, etc.

This heavily reduces the lines of code we need to support multiple
platforms from a project perspective.

Addtiionally, this heavily improves our detection of libraries, etc.
CPack works closely together with CMake to do the right thing in
terms of bundling (called 'package'). This generates all the
packaging we need, and some more.
This also means dropping Debian/jessie, as it has a CMake that is
too old (3.0), with no real path to upgrade.
With CMake, these files are simply not compiled to start with.
@glx22 glx22 force-pushed the TrueBrain:cmake branch from 6575eeb to 80db1be Nov 16, 2019
@LordAro LordAro added this to the 1.11.0 milestone Nov 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.