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.

Sign up for GitHub

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

Jump to bottom

CMake improvements and fixes #26

Closed
wants to merge 1 commit into from
Closed

CMake improvements and fixes #26

wants to merge 1 commit into from

Conversation

susnux
Copy link

@susnux susnux commented Aug 28, 2016

  • Fix third-party libraries, they need to be explicit compiled as static libs.
    Some distributions use shared libraries as default preset in CMake.
  • Fix some CMake checks (missing return in main function).
  • Allow custom install directories (e.g. some distributions do not use share/games or usr/games and use share/ and usr/bin instead, so allow user to set a custom target, but still provide same default value as before).

@Mailaender
Copy link
Contributor

See it in action at https://build.opensuse.org/request/show/423579

@isilkor
Copy link
Contributor

isilkor commented Aug 29, 2016

  • The installation code is currently only used on Linux (and maybe non-Darwin BSDs), so I think the new variable should not show up on Windows so people don't try to set it and have it do nothing.
  • The STATIC change looks fine to me.
  • ISO C++ provides that leaving main() without returning a value implicitly returns 0, and when I wrote the tests I've left the returns out since they aren't relevant to the test itself. But you're right that it's probably still a good idea to make them consistently return or not return.

@susnux
Copy link
Author

susnux commented Aug 29, 2016

So do you would suggest to pack it into an if (UNIX) ?
Would make sense, from my point of view.

@isilkor
Copy link
Contributor

isilkor commented Aug 30, 2016 via email

@susnux
CMake improvements and fixes
61669e0 
* Fix thirdparty libraries, needed to be explicit compiled as static libs.
  Some distributions use shared libraries as default preset in CMake.
* Fix some CMake checks (missing return in main function).
* Allow custom install directories
@ckanibal
Copy link
Contributor

ckanibal commented Aug 30, 2016 •
edited

CMAKE_INSTALL_PREFIX will most likely not work for a end user on macOS due to system integrity protection. I'd rather have the application packaging script-fu working again (thing that creates .app-Files).
Until then I suggest not using the install-method on macOS at all.

isilkor
isilkor requested changes Sep 17, 2016
View reviewed changes
Copy link
Contributor

@isilkor isilkor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OC_SYSTEM_*_DIR needs to be set on all platforms, or alternatively the install() commands need to be protected with if(UNIX).

CMakeLists.txt
@@ -1459,7 +1466,7 @@ foreach(group ${OC_C4GROUPS})
DEPENDS "${native_c4group}"
VERBATIM
)
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${group} DESTINATION share/games/openclonk)
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${group} DESTINATION "${OC_SYSTEM_DATA_DIR}")
Copy link
Contributor

@isilkor isilkor Sep 17, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late response, but this doesn't work on Windows (or rather, all non-UNIX platforms, of which there is only one we support) because OC_SYSTEM_DATA_DIR is unset, which makes CMake unhappy.

CMakeLists.txt
@@ -1474,7 +1481,7 @@ if (NOT APPLE)
install(CODE "execute_process(COMMAND update-desktop-database)")

# Install binaries
install(TARGETS openclonk DESTINATION games)
install(TARGETS openclonk DESTINATION "${OC_SYSTEM_GAMES_DIR}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

RomainNaour added a commit to RomainNaour/openclonk that referenced this pull request Apr 10, 2018
@RomainNaour
CMake: build libmisc and libc4script statically
2d5b097 
As reported by [1], some distributions use shared libraries as
default preset in CMake.

Without explicitely linking statically libmisc and libc4script,
we have the following link issue:

[...]/host/bin/x86_64-linux-g++ --sysroot=[...]sysroot
-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -Os
-std=gnu++14 -Wall -Wextra -Wredundant-decls -Wendif-labels
-Wpointer-arith -Wcast-qual -Wcast-align -Wwrite-strings -Winit-self
-Wsign-promo -Wno-reorder -Wno-unused-parameter -Wnon-virtual-dtor
-Woverloaded-virtual  -DNDEBUG
-rdynamic CMakeFiles/c4group.dir/src/c4group/C4GroupMain.cpp.o
-o c4group
-Wl,-rpath,[...]/build/openclonk-7.0:
liblibmisc.so -lz -lpthread -lrt
liblibmisc.so : référence indéfinie vers « C4LangStringTable::Translate(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const »
liblibmisc.so : référence indéfinie vers « C4LangStringTable::system_string_table »

[1] openclonk#26

Signed-off-by: Romain Naour <romain.naour@gmail.com>
RomainNaour added a commit to RomainNaour/openclonk that referenced this pull request Apr 12, 2018
@RomainNaour
CMake: build libmisc and libc4script statically
923d355 
As reported by [1], some distributions use shared libraries as
default preset in CMake.

Without explicitely linking statically libmisc and libc4script,
we have the following link issue:

[...]/host/bin/x86_64-linux-g++ --sysroot=[...]sysroot
-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -Os
-std=gnu++14 -Wall -Wextra -Wredundant-decls -Wendif-labels
-Wpointer-arith -Wcast-qual -Wcast-align -Wwrite-strings -Winit-self
-Wsign-promo -Wno-reorder -Wno-unused-parameter -Wnon-virtual-dtor
-Woverloaded-virtual  -DNDEBUG
-rdynamic CMakeFiles/c4group.dir/src/c4group/C4GroupMain.cpp.o
-o c4group
-Wl,-rpath,[...]/build/openclonk-7.0:
liblibmisc.so -lz -lpthread -lrt
liblibmisc.so : référence indéfinie vers « C4LangStringTable::Translate(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const »
liblibmisc.so : référence indéfinie vers « C4LangStringTable::system_string_table »

[1] openclonk#26

While at it, build libopenclonk statically since libopenclonk is not
installed by the CMake build system.

Signed-off-by: Romain Naour <romain.naour@gmail.com>
openclonk-mirror pushed a commit that referenced this pull request Apr 21, 2018
@RomainNaour @jcaesar
CMake: build libmisc and libc4script statically
48c38c9 
As reported by [1], some distributions use shared libraries as
default preset in CMake.

Without explicitely linking statically libmisc and libc4script,
we have the following link issue:

[...]/host/bin/x86_64-linux-g++ --sysroot=[...]sysroot
-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -Os
-std=gnu++14 -Wall -Wextra -Wredundant-decls -Wendif-labels
-Wpointer-arith -Wcast-qual -Wcast-align -Wwrite-strings -Winit-self
-Wsign-promo -Wno-reorder -Wno-unused-parameter -Wnon-virtual-dtor
-Woverloaded-virtual  -DNDEBUG
-rdynamic CMakeFiles/c4group.dir/src/c4group/C4GroupMain.cpp.o
-o c4group
-Wl,-rpath,[...]/build/openclonk-7.0:
liblibmisc.so -lz -lpthread -lrt
liblibmisc.so : référence indéfinie vers « C4LangStringTable::Translate(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const »
liblibmisc.so : référence indéfinie vers « C4LangStringTable::system_string_table »

[1] #26

While at it, build libopenclonk statically since libopenclonk is not
installed by the CMake build system.

Signed-off-by: Romain Naour <romain.naour@gmail.com>
@jcaesar
Copy link
Member

jcaesar commented Dec 3, 2018

Since the commits are in master, I believe I can close this?

@jcaesar jcaesar closed this Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isilkor isilkor isilkor requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@susnux @Mailaender @isilkor @ckanibal @jcaesar