News:

Simutrans Forum Archive
A complete record of the old Simutrans Forum.

CMake build support

Started by ceeac, August 05, 2018, 08:50:34 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Roboron

Quote from: prissi on June 02, 2021, 11:45:54 AMHow does the neccessary freetype libaries are found in linux (or whatever your build system uses?) without a configure script?

The FindLibrary.cmake files does this. They are called by find_package in SimutransDependencies.cmake. Most of those files (Miniupnpc, ZSTD...) were created by ceeac in the original contribution, some others (like Freetype)  are imported from cmake defaults (it should be in /usr/share/cmake).

But the problem here is not that cmake doesn't find, Freetype or others (it does), but that it doesn't set the linker flag to static.

prissi

#36
It does not set any libaries, also for linking SDL2 the libaries are set by hand.

EDIT:
I had a look at the cmakelist.txt of other projects, and usually they do not include these many specific module. and specific linker flags. I wonder if we did something wrong.

Roboron

#37
Quote from: prissi on June 02, 2021, 12:03:59 PM
It does not set any libaries

Yes, i does, look at the comment at the top of each file:

Quote# This module defines
#  MiniUPNP_FOUND, if miniupnp library and headers have been found
#  MiniUPNP_LIBRARY, the miniupnp variant
#  MiniUPNP_INCLUDE_DIR, where to find miniupnpc.h and family)
#  MiniUPNP_VERSION, the API version of MiniUPNP

Moreover, looking at MiniUPNP specifically, it seems that it also set the MiniUPNP_STATIC_LIBRARIES variable (but it is usually not done, and definitely no on the default FindFreetype.cmake file, I have taken a look before).

find_library(MiniUPNP_STATIC_LIBRARY libminiupnpc.a
HINTS $ENV{MINIUPNP_STATIC_LIBRARY}
)
set(MiniUPNP_STATIC_LIBRARIES ${MiniUPNP_STATIC_LIBRARY})


But just looking at it I'm not sure this is enough to link statically, I'll have to test.

Quote from: prissi on June 02, 2021, 12:03:59 PMalso for linking SDL2 the libaries are set by hand.

Yes, that needs to change.

Quote from: prissi on June 02, 2021, 12:03:59 PMI had a look at the cmakelist.txt of other projects, and usually they do not include these many specific module. and specific linker flags. I wonder if we did something wrong.

I wonder if they use mingw static linking. Statically linking with MSVC + vcpkg didn't require extra flags, and maybe most projects use that workflow nowadays.

prissi

Other projects seem to use just (as we do too)

if(MINGW)
# Let executables run outside MinGW environment
# Force searching static libs
set(CMAKE_FIND_LIBRARY_SUFFIXES ".a" ON)

# Force static linking
link_libraries(-static -static-libgcc -static-libstdc++)
endif()


No matter if static or dynamic linking, the libaries must be defined at link time, even DLLs or the linking fails. VCPKG has cmakelist for all libaries, so the configure of the dependencies is delegated to them.

Roboron

#39
I did some investigation, looking at the top projects that use CMake. Examining the CMake files of OpenRCT2 I've found a pretty similar scheme to what I previously proposed. I'll show some relevant lines I found on OpenRCT2/src/openrct2/CMakeLists.txt (only showing libpng for simplicity)

# Third party libraries
if (MSVC)
    find_package(png 1.6 REQUIRED)
else ()
    PKG_CHECK_MODULES(PNG IMPORTED_TARGET libpng>=1.6)
endif ()

if (STATIC)
    target_link_libraries(${PROJECT_NAME} ${PNG_STATIC_LIBRARIES})
else ()
    if (NOT MSVC)
        target_link_libraries(${PROJECT_NAME} PkgConfig::PNG)
    else ()
        target_link_libraries(${PROJECT_NAME} ${PNG_LIBRARIES})
    endif ()
endif ()


So the process is pretty much like that:

1. Are we building for MSVC -> find_package (what we currentli use)
2. If not -> PKG_CHECK_MODULES (in other words, pkg-config)
3. If STATIC is set, they also use pkg-config defined variables like we did for Freetype

We don't have the need to do #2 as everything is working fine with find_package on every system, but the solution of static linking seems to be #3, at least according to this project.

Did you find any other example?

prissi

After more or less removing everything unnecessary until I got to build all variants under Msys2 this is the state. Tested with GDI, freetype, ZSTD, SDL2 and fluidsynth. There was also a lot of wrong and unneeded option cause for example the visual buffer update display.

MSVC is still no cooperative in actually debugging, it is fixed on the compile path. At least mingw builds fine, also static. The question is, if it builds for you and on Mac ...

Roboron

QuoteCMake Error at CMakeLists.txt:20 (include):
  include could not find requested file:

    simutrans-revision

1. Fixed and renamed to SimutransRevision.cmake for consistency.

2. include(SimutransCompileOptions) was imported AFTER some options were already evaluated (like SIMUTRANS_USE_FREETYPE), so it was compiling without those options defined. I moved it to the top (next to backend selection) in attached patch.

3. Those last lines lines were wrong. First because Freetype_FOUND (as used later) would not be the same as FREETYPE_FOUND, and also it was duplicated probably because you copy-pasted ;-)
if (MSVC)
find_package(Freetype)
find_package(FluidSynth)
else ()
pkg_check_modules(FREETYPE IMPORTED_TARGET freetype2)
pkg_check_modules(FREETYPE IMPORTED_TARGET fluidsynth)
endif ()


For the same reason this needed to change (the same for FluidSynth):

if (SIMUTRANS_USE_FREETYPE)
  target_include_directories(simutrans PRIVATE ${FREETYPE_INCLUDE_DIRS})
  if (MINGW)
    target_link_libraries(simutrans PRIVATE ${FREETYPE_STATIC_LIBRARIES})
  else ()
    target_link_libraries(simutrans PRIVATE ${FREETYPE_LIBRARIES})
  endif (MINGW)
  target_compile_definitions(simutrans PRIVATE USE_FREETYPE=1)
endif (SIMUTRANS_USE_FREETYPE)


After I changed those two things (included in the patch)  it successfully compiled and linked in my main machine (Linux x64).

4. Linking statically SDL2 on MinGW failed. The same solution applied to Freetype and FluidSynth was necessary to build. Added.

5. There's a problem when linking statically the fluidsynth library, but it is not ours. The static library doesn't include the static dependencies, and therefore pkg-config only returns -lfluidsynth when searching for them. I'll report it to the devs, but until them, we will need to list the libraries ourselves just like it was before.

6. Please keep the optional CCache. It improves the time of (re)compilation by a lot. Instead o waiting for minutes it only takes a few seconds to recompile. I re-added it.

7. I've created a new file (SimutransSourceList.cmake) containing the list of sources, that is included in CMakeLists.txt. I think this improves the readability of the main file without 300 lines dedicated to it.

I think the debug options still need some polishing. Particularly the debug level is unset by default, which causes warning while building for Release. But I'll leave it for today, I made enough progress.

prissi

Thank you for your work, I will test it. In the past static linking for SDL2 produced huge non-working binaries (same for pthread) so I think I did not realise that it was not statically linked since there was a SDL2.dll in the folder anyway.

I think after some more work on the cmake files, it may indeed become a working alternative. At least I feel comfortable enough to do something with cmake files. I still think their system is quite broken, and mostly either geared towards building everything from source or making libaries and the main use of programs is rather an afterthough. Also I still have to get it work (for debugging) on MSVC.

I am not sure whether it need to rebuilt everything after each checkout to get the revision right. That is ok for the nightly but for programming it would be a nuissance. Also the revision is built with a script on MSVC inside the project files, so it is not there yet.

Roboron

Now trying to compile on MinGW (x64) on MSYS, it failed at the end:

C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.3.0/../../../../x86_64-w64-mingw32/bin/ld.exe: CMakeFiles/simutrans.dir/objects.a(simrandom.cc.obj): in function `log2(unsigned int)':
C:/msys64/home/Rober/trunk/utils/simrandom.cc:310: multiple definition of `log2(unsigned int)'; CMakeFiles/simutrans.dir/objects.a(schedule.cc.obj):C:/msys64/home/Rober/trunk/utils/int_math.h:30: first defined here
collect2.exe: error: ld returned 1 exit status


It is true that there is a duplicated function, see:

https://github.com/aburch/simutrans/blob/master/utils/int_math.h#L29-L42
https://github.com/aburch/simutrans/blob/master/utils/simrandom.cc#L309-L325

But this error did not show up before, so I wonder if I did something wrong. EDIT: This error only happened when building for Debug, but not Release. (???)

Also about your last commit (probably related to this) "change order of defines to keep gcc happy" https://github.com/aburch/simutrans/commit/fe55c7ab1e373e267bbc0f0ba42c13600fb2e084
GCC is still not happy, it warnings that _XOPEN_SOURCE is not defined when evaluated. Have you considered this instead?

#if !defined(__APPLE__)  &&  ( !defined(_XOPEN_SOURCE) || _XOPEN_SOURCE < 600 )

Quote from: prissi on June 03, 2021, 11:26:58 PMI am not sure whether it need to rebuilt everything after each checkout to get the revision right.

I'm pretty sure one would need to re-generate the build files, since the revision is obtained and cached during this process. I've made some investigation, and yes, there are ways to get the revision also when building, although they are not so simple. I'll leave this for reference:

=> https://stackoverflow.com/questions/3780667/use-cmake-to-get-build-time-subversion-revision

Wether or not is worth to include such things may depend on how you think CMkae should be used: if as a replacement for the current build system or only as an alternative. There's still work to do for the first possibility, so we can left this for later in any case.

Quote from: prissi on June 03, 2021, 11:26:58 PMAlso the revision is built with a script on MSVC inside the project files, so it is not there yet.

Do you mean the current project files? There's no problem setting the revision with cmake on MSVC, as long as svn is found (for me it worked after attached patch) and there's no script inside the generated project files that I see, so I don't see why we need such a script.

Added in this patch only 3 small changes:

* SIMUTRANS_WC_REVISION was defined but never used: in other words, the REVISION compile optionwas not set, it disappeared at some opint. Re-added.
* Now setting DEBUG to 0 for Release and MinSizeRel builds
* Also required minimum fluidsynth version 2.1.0 - to prevent older linux distributions from compiling without SDL2 audio driver support.

prissi

I have a working configuration for MSVC project files, direct builds and Mingw builds. I will submit it into the SVN, so we can continue from there. This required a lot of changes beyond a few lines (especially making sure of the VCPKG triplets etc).

There is still the annoying console window popping up for releases and the revision number is not contained in the window title in MSVC builds.

Andarix

Quote from: prissi on June 05, 2021, 02:44:14 PM
...
There is still the annoying console window popping up for releases and the revision number is not contained in the window title in MSVC builds.

for this used revision.h

command line script windows


git svn log --oneline --limit=1 > status.txt

set /p string=<status.txt

echo #define REVISION %string:~1,5% > revision.h



not work on git action (git version not work command 'git svn log', to old version)

Roboron

#46
Thanks, it will be much easier to apply, review and create patchs now.

Quote from: prissi on June 05, 2021, 02:44:14 PMThere is still the annoying console window popping up for releases

This seems to work (building for release = no console, else console). You can change it and make an option instead if you want.

#
# target & sources
#
if (CMAKE_BUILD_TYPE STREQUAL "Release" OR CMAKE_BUILD_TYPE STREQUAL "MinSizeRel")
add_executable(simutrans WIN32 MACOSX_BUNDLE)
else ()
add_executable(simutrans)
endif ()
include(SimutransSourceList)


Notice one can add MACOSX_BUNDLE because (I read on the documentation) those variables are only taken in account depending on the OS we are building on, so it has no effect on WIN32. But I have yet to test if it gerenerates the bundle (probably some configuration is needed). I'll work on it later, remove it if you want.

Quote from: prissi on June 05, 2021, 02:44:14 PMthe revision number is not contained in the window title in MSVC builds.

Removing the if case for Visual Studio here worked for me:

string(FIND ${CMAKE_GENERATOR} "Visual Studio" VS )
if (${VS} EQUAL 0)
# using a script for MSVC project files
file(WRITE ${CMAKE_SOURCE_DIR}/revision.h "#define REVISION \n")
add_custom_command(TARGET simutrans PRE_BUILD WORKING_DIRECTORY ${CMAKE_SOURCE_DIR} COMMAND cscript.exe /Nologo revision.jse COMMENT "Find SVN revision")

else ()
# using custom target svnhear that is always built to update the revision.h file using cmake script
add_custom_target(svnheader ALL DEPENDS svn_header)
add_custom_command(OUTPUT svn_header ${CMAKE_SOURCE_DIR}/revision.h COMMAND ${CMAKE_COMMAND} -DSOURCE_DIR=${CMAKE_SOURCE_DIR} -P ${CMAKE_MODULE_PATH}/SimutransRevision.cmake)
set_source_files_properties(${CMAKE_SOURCE_DIR}/revisiom.h PROPERTIES GENERATED TRUE HEADER_FILE_ONLY TRUE)
add_dependencies(simutrans svnheader)
target_compile_definitions(simutrans PRIVATE REVISION_FROM_FILE)

endif ()


In other words, the else() code seems to be valid also for MSVC builds. Why try to force a different method? Is it not working for you? (One thing I noticed is that it is generating the revision.h three times, or so is telling me the MinGW console when building... but I don't know why).

I added those changes in this patch, and included the last two from my last message.

prissi

#47
I tried this (adding WIN32) and it did not work or got not added. It is really hard to debug. I finally had to modify the source to change the subsystem.

The generator for MSVC is actually "Ninja *", "Visual Studio *" generates classical project files. The cannot assum cmake, hence the old script code.

However, generating the MSVC project files is the worst thing for a clicky windows hero:

mkdir out
cd out
cmake .. -A Win32 -DCMAKE_TOOLCHAIN_FILE=C:\Users\xxx\Documents\GitHub\vcpkg\scripts\buildsystems\vcpkg.cmake

even though I removed the need for triplet. Type one letter wrong in the CMAKE part and it does not generate an error message but just does not generate a makefile. This is absolutely bad design, so no user without reading half the internet will get this ri\ght. Furthermore, if you use the wrong MSVC command line like x64 or Win32 on the respective other, it will also fail. Same if you misspell the triplet (like x64-window-static) it will generate a file that builds but not links with cryptic error messages.

Also missing a way to force fresh generation without deleting the entire out folder each time would be nice. Deleting the cache alone generate spurious errors.

EDIT:
your revision code on git just returns nightly instead a proper revision number. Cange the git code to work also with github.

Also fixed the double build of the revision.

Roboron

#48
1. When defining a toolchain file, but not the vcpkg one (in my case, for cross-compilation), it failed because vcpkg triplet can't be found.

Quotecmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_TOOLCHAIN_FILE=~/toolchain-mingw32.cmake ..
CMake Error at cmake/SimutransVcpkgTriplet.cmake:44 (message):
  Please specify VCPKG triplet!
Call Stack (most recent call first):
  CMakeLists.txt:18 (include)


-- Configuring incomplete, errors occurred!

Fixed by including SimutransVcpkgTriplet only when on MSVC.


if (MSVC)
include(SimutransVcpkgTriplet)
endif ()


2. Since you removed the FindZSTD.cmake, it is necessary to search it with pkgconf when not on MSVC, added.

Quote from: prissi on June 06, 2021, 12:38:37 AMI tried this (adding WIN32) and it did not work or got not added. It is really hard to debug. I finally had to modify the source to change the subsystem.

3. Your condition was not working, but even if it were, it would throw an error because add_executable was called twice (hence the change to SimutransSourceList). Use message warnings to debug and check if the conditions are working. You can also output anything to cache. The generated CMakeCache.txt usually has all the info you need to debug (for example that's where I found fluidsynth wasn't exporting the static libraries). Just like your previous one, this condition always fail for me (not on MSVC):

if ($<CONFIG:Release> OR $<CONFIG:MinSizeRel>)
message(WARNING "building release")
add_executable(simutrans WIN32 MACOSX_BUNDLE)
else ()
message(WARNING "NOT building release")
add_executable(simutrans)
endif ()


Quotecmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_TOOLCHAIN_FILE=~/toolchain-mingw32.cmake ..
CMake Warning at CMakeLists.txt:92 (message):
  NOT building release

If this condition is needed for MSVC, maybe keep both?

if ( ($<CONFIG:Release> OR $<CONFIG:MinSizeRel>) OR (CMAKE_BUILD_TYPE STREQUAL "Release" OR CMAKE_BUILD_TYPE STREQUAL "MinSizeRel") )

4. The SimutransRevision.cmake is not working properly, it is generating junk

Quote from: revision.h#define REVISION Ruta:

Note that there are a variable called SIMUTRANS_WC_REVISION and another called SIMUTRANS_GIT_WC_REVISION, and both are used, I think the problem is there. I tried to rename them in my last patch, and that was working. Please re-check!

QuoteThe generator for MSVC is actually "Ninja *", "Visual Studio *" generates classical project files. The cannot assum cmake, hence the old script code.

Oh, that explain things...

QuoteThis is absolutely bad design, so no user without reading half the internet will get this ri\ght

I think it's just a matter of documenting the process definedly and clearly. I don't expect a user will try to build Simutrans without previously checking the build instructions, and that is were all those details will need to be explained step by step.

Quote from: prissi on June 06, 2021, 12:38:37 AMAlso missing a way to force fresh generation without deleting the entire out folder each time would be nice. Deleting the cache alone generate spurious errors.

The CMakeCache.txt alone isn't enough, you also need to remove CMakeFiles directory.

prissi

Quote from: Roboron on June 06, 2021, 11:07:15 AM
If this condition is needed for MSVC, maybe keep both?
According to documentation, STREQUAL Release does not compare true for RELEASE while the other construct does.

Quote
4. The SimutransRevision.cmake is not working properly, it is generating junk
It works fine on the aburch from github and the svn. Your code returned had "git.exe git ..which did not work for sure, and after removing the double git always retuned  "Nightly" instead of a revision umber. Because this is the only tag in aburch. What is the actual printout from "git log -1 -pretty" from you repo then?

It should look like
commit dd4efdf6a17078fc927619f5e1310747045be92d
Author: Markus Pristovsek <prissi@physik.tu-berlin.de>
Date:   Sun Jun 6 03:16:48 2021 +0000

    Several fixes to cmake builds

    git-svn-id: svn://tron.homeunix.org/simutrans/simutrans/trunk@9867 8aca7d54-2c30-db11-9de9-000461428c89


Quote
I think it's just a matter of documenting the process definedly and clearly. I don't expect a user will try to build Simutrans without previously checking the build instructions, and that is were all those details will need to be explained step by step.
I think windows users better stay with the built MSVC cmkae, ad bad as it is. Still it would need three pages of screenshots.

Quote
The CMakeCache.txt alone isn't enough, you also need to remove CMakeFiles directory.
That why even with MSVC I had a mingw terminal open for "rm -rf out"


Roboron

#50
Quote from: prissi on June 06, 2021, 11:48:51 AMAccording to documentation, STREQUAL Release does not compare true for RELEASE while the other construct does.

Can you reference it? Because the behaviour I'm seeing is the opposite: the STREQUAL comparation does work, while the $<CONFIG:Release> does not, it is never true for Release (just check my previous output).

Quote from: prissi on June 06, 2021, 11:48:51 AMIt works fine on the aburch from github and the svn.

That's not the problem, prissi. I'm not using git, but svn. The command works fine. The problem has to do with the use of variables, see:

if ( NOT SIMUTRANS_WC_REVISION AND Subversion_FOUND )
execute_process(WORKING_DIRECTORY ${SOURCE_DIR} COMMAND ${SVN_EXECUTABLE} svn info RESULT_VARIABLE res_var OUTPUT_VARIABLE SIMUTRANS_GIT_WC_REVISION)
if ( ${res_var} EQUAL 0 )
        message( "svn ok:" ${SIMUTRANS_GIT_WC_REVISION})
string( REGEX REPLACE "\n" " " TEMP1 ${SIMUTRANS_GIT_WC_REVISION})
string( REGEX REPLACE "^.*Revision: " "" TEMP2 ${TEMP1})
string( REGEX REPLACE " .*$" "" SIMUTRANS_WC_REVISION ${TEMP2})
endif()
endif ()
if (SIMUTRANS_WC_REVISION)
# write a file with the SVNVERSION define
file(WRITE revision.h.txt "#define REVISION ${SIMUTRANS_WC_REVISION}\n")
...


1. First if checks for SIMUTRANS_WC_REVISION
2. Output the result to SIMUTRANS_GIT_WC_REVISION. What?
3. Show on console SIMUTRANS_GIT_WC_REVISION. It shows the correct revision, of course. But...
4. Write junk to SIMUTRANS_WC_REVISION ("Ruta" in thsi case).
5. Check again for SIMUTRANS_WC_REVISION (it is defined, but is is not valid)
6. Finally write the invalid content on SIMUTRANS_WC_REVISION to the file...

This is what is failing.

prissi

#51
I was just reusing variables from the copying code. The code f\works fine

execute_process(WORKING_DIRECTORY ${SOURCE_DIR} COMMAND ${SVN_EXECUTABLE} svn info RESULT_VARIABLE res_var OUTPUT_VARIABLE SIMUTRANS_RAW_WC_REVISION)
if ( ${res_var} EQUAL 0 )
message( "svn ok:" ${SIMUTRANS_RAW_WC_REVISION})
string( REGEX REPLACE "\\n" " " TEMP1 ${SIMUTRANS_RAW_WC_REVISION})
string( REGEX REPLACE "^.*Revision: " "" TEMP2 ${TEMP1})
string( REGEX REPLACE " .*$" "" SIMUTRANS_WC_REVISION ${TEMP2})
endif()

Ok, maybe a better name for the first output. Let's call it SIMUTRANS_RAW_WC_REVISION.
Replace all "\n" (overwise regex fails for unknowns reasons) and put results in TEMP1. Remove everything until "...Revision: " and put results in TEMP2. Remove the trailing junk and put result in SIMUTRANS_WC_REVISION .

Maybe you have a different separtor. What is the output of snv:ok? Maybe one need to replace in the first step "[\n\r\t]"?

In my case it is
Working Copy Root Path: /home/Markus/simutrans
URL: svn://servers.simutrans.org/simutrans/trunk
Relative URL: ^/simutrans/trunk
Repository Root: svn://servers.simutrans.org
Repository UUID: 8aca7d54-2c30-db11-9de9-000461428c89
Revision: 9867
Node Kind: directory
Schedule: normal
Last Changed Author: prissi
Last Changed Rev: 9867
Last Changed Date: 2021-06-06 12:16:48 +0900 (Sun, 06 Jun 2021)


-- Compiling Simutrans revision 9867 ...


EDIT:
Or could it be that the output in spain is not revision?

EDIT2:
I tried the "svn info --show-item revision" command instead. Maybe this work for any language. I had to include a second svn in the command, for unknown reasons ...

Roboron

Quote from: prissi on June 06, 2021, 12:51:57 PMOr could it be that the output in spain is not revision?

Yes it is! You nailed it. The new solution is working prefectly, thank you.

There's also a git command that returns the revision number directly (git rev-list --count HEAD), but sadly this can't be used as the revision from git is different from the svn :-(

if (CMAKE_BUILD_TYPE EQUAL "DEBUG")

This doesn't work for me, it always evaluate to false :-/

CMAKE_BUILD_TYPE vs $<CONFIG:>, I think I now understand the difference.

- CMAKE_BUILD_TYPE is set ONLY for single-generators (Unix makefiles). It is NOT set for multi-generators (Visual Studio). So this is not what we should use.
- $<CONFIG:Release> is valid for both, BUT IT CAN NOT BE EVALUATED ON IF STATEMENTS!!!. That's why it wasn't working!

Instead, they are evaluated "on the fly" on target properties. For example, for our case:

add_executable(simutrans)
set_target_properties(simutrans PROPERTIES WIN32_EXECUTABLE $<CONFIG:Release>)


Is what we were looking for! Please test with this.

EDIT: More info about "generator expressions" https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html#manual:cmake-generator-expressions(7)

TurfIt

Why is the exisiting working revision build system being broken?

===> HOSTCXX dataobj/environment.cc
In file included from dataobj/environment.cc:10:
dataobj/../simversion.h:10:10: fatal error: revision.h: No such file or directory
   10 | #include "revision.h"
      |          ^~~~~~~~~~~~
compilation terminated.
make: *** [common.mk:51: /r/build/dataobj/environment.o] Error 1


r9868 simversion.h:
--#if defined(REVISION_FROM_FILE)  &&  !defined(REVISION)
// include external generated revision file
#include "revision.h"
--#endif




prissi

Later actually windres should choke on the same position. Should be fixed in r9873

Roboron

I worked on the Mac support, and it went so well it is definitely an improvement over the current system. This patch contains the following:

* Enable the build on Mac (it was just a matter of manually setting the homebrew lib directory for linking).
* Generate a Mac App Bundle. This is done from cmake itself, it doesn't use the current script.
* Add linked libraries to the bundle (when running install). This is the major improvement over the current system, since this is done "automagically" with the linked libraries, instead of having to manually add them one-by-one like we do now (without option to exclude/include only what you need).
* Minor fixes for compilation warnings.
* Added EXCLUDE_FROM_ALL to makeobj and nettools so they are not compiled unless specified otherwise. This is because, even if you set the target build to simutrans only, they are built anyway when running install.

Note that the function used for including dynamic linked libraries into the Mac bundle (fixup_bundle) may also be used for including dynamic libs on other systems (or so I have read).

install(CODE "
include(BundleUtilities)
fixup_bundle(\"${CMAKE_BINARY_DIR}/simutrans/simutrans.app\" \"\" \"\")
")


But since that was not my target this time I didn't dig further. However, this could be beneficial when packaging the Steam (linux) version, since last time I had to do it manually. I'll probably revisit it later.

To build on Mac just do the usual
cmake ..
sudo cmake --build . --target install


(sudo is necessary to link some homebrew libraries, but not all)

--target install will install simutrans on /usr/local (or /usr/share I don't remember) by default, but it is necessary to link the libraries to the bundle. Maybe changing the default installation directory to the build directory is a good idea, so the user can copy it whenever it wants - Simutrans is not usually installed system-wide anyway.

prissi

Does the bundle works also on ACs without homebrew installed? This was the reason why it was not used in the current build system on github.

Roboron

Quote from: prissi on June 13, 2021, 06:02:55 AM
Does the bundle works also on ACs without homebrew installed? This was the reason why it was not used in the current build system on github.

Yes, that's the point of including the libraries into the bundle on the install phase. I don't have a second machine to test, but I tried uninstalling SDL2 from homebrew, then running the game, and everything worked as expected.

prissi

I submitted something that might work for MacOS and Windows, I hope.

Could you give a a script for MacOS similar to the current github script to prepare for MacOS compilation, so I can check the .github  yml for MacOS

Roboron

I don't have much experience with the github build system, but hopefully this works.

Ended up changing the install directory (for mac only) because it's easier to zip if it is in the build directory.

Roboron

Quote from: GitHub MacOS NightlyIn file included from /Users/runner/work/simutrans/simutrans/dataobj/loadsave.cc:28:
/Users/runner/work/simutrans/simutrans/dataobj/../io/rdwr/zstd_file_rdwr_stream.h:12:10: fatal error: 'zstd.h' file not found
#include <zstd.h>
         ^~~~~~~~
1 error generated.

Maybe it will be fixed if we include zstd directories. I've not had such problem myself, so I can't personally test it.

ceeac

One can also use the PkgCofig imported target for this according to the documentation. I have used this solution in r9890 along with some fixes to compile on Linux/SDL2+Freetype. IMO the apporach with the imported target is much cleaner because one does not need to keep track of multiple variables.

Roboron

That's better.  But there's not imported target (or so I see) for static libraries, so for MinGW we still need to use target_include_directories (at least, for Freetype, others doesn't seem to be needed for whatever reason (?)).

The Mac GitHub action is still failing for SDL2. With this new approach we should remove the elseif(APPLE) conditions.

prissi

#63
The problem is the include path for cmake projects on MacOS: This is <SDL.h> not <SDL2/SDL.h>. Can be fixed with ifdefs though. I am working right now on my github fork to test it, so be a little patient. Also the installing was not working.

I hope from github forward and backward I have not messed up the changes to CMakeList.txt concerning libaries. I am too tired tonight, please have a look.

Roboron

I've seen you have included pak64. I wonder if this really makes sense, now that with you can select a pakset at game start if you have no one.

Other than that, very well done. Finally we can move onto another things :-)

prissi

Does the file download work? I heard the program directory is write protected (like in windows) and hence cannot be modified on run time? But any documentation I found is highly useless, so would you please try installation on the fly. (I.e. building without pak and starting).

Roboron

#66
Yes, Yes it works. I tried it before, that's why I suggested it. I had no problem downloading a pakset at start after building without one.

EDIT: I have tried opening the app bundle downloaded from GitHub, and three things happened

1. It warns about the developer being not trusted. You can still open it by trying to open it a second time.
2. The app bundle closes immediately after opening it. I remember this is something that was brought up before by THLeader https://forum.simutrans.com/index.php/topic,19444.msg192083.html#msg192083
3. If I open it from CLI (opening the executable /simutrans.app/Contents/MacOS/simutrans) you were indeed right, it is not able to download any pakset.

But the pakset problem #3 is probably related to problem #2 - since changing the app bundle to my build directory fix both of them. There's something we are overlooking about Apple MacOS and permissions, but I don't know what it is. And I honestly don't have much motivation to fix it, since this is not a cmake-specific problem, nor it is a problem for me for the Steam release (because I call /simutrans.app/Contents/MacOS/simutrans to avoid it). If anyone wants to continue work on this better to move the discussion to another thread.

Roboron

I've added documentation about compiling with CMake to the wiki. Only thing left is how to do it from MSVC GUI (I don't know) https://simutrans-germany.com/wiki/wiki/en_CompilingSimutrans#Compiling_with_CMake

If you want I can work on a new version of the README including this information.

Also a little patch to add an option to generate a Steam build is attached (currently it is only used for unmapping the F12 key, but it will be useful when implementing Steam-related code in the future).

prissi

By the way, does the nightly build now runs on Mac? The last comment was not clear to me.

Roboron

#69
Quote from: prissi on October 09, 2021, 01:36:03 PMBy the way, does the nightly build now runs on Mac? The last comment was not clear to me.

It runs, but you have to call the executable inside the bundle. Some issue (I think related to permissions) is preventing the bundle from being launched. This is only reproducible when I download the nightly, if I compile the bundle myself, I can open it just right. That's why I think it may be related to permissions, but I don't know enough about Mac to fix it.

EDIT: I start to remember, I think it has something to do with every resource Simutrans needs (that is, texts, music, and so on), needs to be INSIDE the bundle.

EDIT2: At my notes I found something about working directories. When launching the app bundle the working directory is randomnized. That would explain why Simutrans does not open. See https://stackoverflow.com/questions/42600770/chdir-to-the-location-of-the-app-bundle-in-c-macos-application