Merge lp:~widelands-dev/widelands/cmakepolicy into lp:widelands

Proposed by Toni Förster on 2018-10-02
Status: Merged
Merged at revision: 8880
Proposed branch: lp:~widelands-dev/widelands/cmakepolicy
Merge into: lp:widelands
Diff against target: 58 lines (+28/-1)
2 files modified
CMakeLists.txt (+24/-1)
src/graphic/graphic.cc (+4/-0)
To merge this branch: bzr merge lp:~widelands-dev/widelands/cmakepolicy
Reviewer Review Type Date Requested Status
kaputtnik 2018-10-02 Approve on 2018-10-13
Review via email: mp+356018@code.launchpad.net

Commit message

set cmake_policy version range to NEW instead of specific policy:

https://cmake.org/cmake/help/v3.12/command/cmake_policy.html

Description of the change

We set the policy CMP0054 to NEW since it was not set. Since I stumbled over bug #1772079 I thought it might be better to set a range instead of every single policy (which is also advised in the Cmake manual)

@kaputtnik can you compile this branch on your system, please. You shouldn't get this warning anymore and Cmake should use the GLVND now.

To post a comment you must log in.
kaputtnik (franku) wrote :

Looks good to me...

The warning does not come up any more.

Thanks :-)

review: Approve
kaputtnik (franku) wrote :

oops, looks i have approved too early:

The warning is gone but i get a compile error later on:

[ 46%] Linking CXX executable test_economy
/usr/bin/ld: ../../graphic/libgraphic_gl_utils.a(initialize.cc.o): undefined reference to symbol 'glDrawBuffer'
/usr/bin/ld: /usr/lib/libGL.so.1: error adding symbols: DSO missing from command line
collect2: Fehler: ld gab 1 als Ende-Status zurück
make[2]: *** [src/economy/test/CMakeFiles/test_economy.dir/build.make:472: src/economy/test/test_economy] Fehler 1
make[1]: *** [CMakeFiles/Makefile2:10016: src/economy/test/CMakeFiles/test_economy.dir/all] Fehler 2
make: *** [Makefile:141: all] Fehler 2

review: Needs Fixing
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4090. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/436365665.
Appveyor build 3886. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_cmakepolicy-3886.

GunChleoc (gunchleoc) wrote :

Trunk also has problems in Ubuntu Trusty builds:

CMake Error at CMakeLists.txt:4 (cmake_policy):
  Policy "CMP0054" is not known to this version of CMake.

https://launchpad.net/~widelands-dev/+archive/ubuntu/widelands-daily/+build/15512527

So, making this fix work is very much desired :)

8868. By Toni Förster on 2018-10-06

merge trunk

8869. By Toni Förster on 2018-10-06

setting version range does not work, use fixed version instead

Toni Förster (stonerl) wrote :

I set a fixed version now: 3.9.2. That's the one that is used to compile the macOS nightlies. The Version range does not seem to work as describe, although it should be backwards-compatible (maybe I just misread something)

This does not fix the warning kaputtnik reported. Apparently Widelands can't use the GLVND-libs ATM. That' at least what I read from the error-message kaputtnik posted. Maybe the bug-report should be updated.

bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4100. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/438094146.
Appveyor build 3896. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_cmakepolicy-3896.

kaputtnik (franku) wrote :

While compiling runs through, the linked bug report isn't fixed. You mentioned CMP0054 while the warning i get mention CMP0072?

-- Boost version: 1.68.0
-- Found the following Boost libraries:
-- unit_test_framework
-- regex
-- system
CMake Warning (dev) at /usr/share/cmake-3.12/Modules/FindOpenGL.cmake:270 (message):
  Policy CMP0072 is not set: FindOpenGL prefers GLVND by default when
  available. Run "cmake --help-policy CMP0072" for policy details. Use the
  cmake_policy command to set the policy and suppress this warning.

  FindOpenGL found both a legacy GL library:

    OPENGL_gl_LIBRARY: /usr/lib/libGL.so

  and GLVND libraries for OpenGL and GLX:

    OPENGL_opengl_LIBRARY: /usr/lib/libOpenGL.so
    OPENGL_glx_LIBRARY: /usr/lib/libGLX.so

  OpenGL_GL_PREFERENCE has not been set to "GLVND" or "LEGACY", so for
  compatibility with CMake 3.10 and below the legacy GL library will be used.
Call Stack (most recent call first):
  CMakeLists.txt:61 (find_package)
This warning is for project developers. Use -Wno-dev to suppress it.

-- SDL2_INCLUDE_DIR is /usr/include/SDL2
-- SDL2MAIN_LIBRARY is /usr/lib/libSDL2main.a
-- Not using AddressSanitizer.
-- Version of Widelands Build is bzr8869[cmakepolicy](Debug)
-- Configuring done

Toni Förster (stonerl) wrote :

@kaputtnik I know.

The warning you get, can't be resolved be simply setting cmake_policy to 3.10 or newer (as I naively thought). Doing so would result in using the new GLVND-libs (which is desired), but apparently widelands currently can't be compiled using these libraries. See the linking errors in your second post.

Getting this fixed means. Get widelands to compile with the GLVND-libs and then set the policy to version 3.10.

The intention for this change was to get the builds for 14.04 running (the ones GunChleoc mentioned later on). By doing so I broke Travis and Appveyor builds, because most cmake-versions there don't support a version range. But just a fixed max version (which is totally fine). A by-product would have been fixing your warning as well, but for reasons I described above this is sadly not as simple as I hoped.

GunChleoc (gunchleoc) wrote :

How about trying to do what the original error message is suggesting?

https://cmake.org/cmake/help/latest/policy/CMP0072.html

Try this line:

OpenGL_GL_PREFERENCE=LEGACY

Toni Förster (stonerl) wrote :

@GunChleoc

But that does not solve the underlying issue, does it?

Setting:

cmake_policy(SET CMP0072 OLD)

is the same as not setting the policy at all. In both will result in:

OpenGL_GL_PREFERENCE=LEGACY

The first one though would silence the warning BUT we would run into issues with Cmake version older than 3.11 since they don't know this policy, as seen here:

https://launchpadlibrarian.net/392627292/buildlog_ubuntu-trusty-i386.widelands_1%3A19-ppa0-bzr8875-201810101704~ubuntu14.04.1_BUILDING.txt.gz

CMake Error at CMakeLists.txt:4 (cmake_policy):
  Policy "CMP0054" is not known to this version of CMake.

Since I don't have a Linux machine I can't real test for the linking errors kaputtnik reported.

The commit as it is now should solve the problem with the buildsystem. Should I open a new commit just for the buildsystem?

8870. By Toni Förster on 2018-10-11

set additional linker flags

8871. By Toni Förster on 2018-10-11

merge trunk

8872. By Toni Förster on 2018-10-11

add description remove unneeded line

Toni Förster (stonerl) wrote :

@kaputtnik can you give this another try?

bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4111. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/440139202.
Appveyor build 3906. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_cmakepolicy-3906.

kaputtnik (franku) wrote :

Compiling is fine here with the latest change: No Warning and no compile errors.

I do not really understand that stuff, but can't we just add a switch like that (instead of setting the linker flags):

if(${CMAKE_VERSION} VERSION_GREATER "3.11")
    cmake_policy(SET CMP0072 OLD)
endif()

I tested this and it works for me (deleted the build directory and compiled again) ... Has to be commented well, though.

8873. By Toni Förster on 2018-10-11

only set CMP0072 if Cmake knows about it

Toni Förster (stonerl) wrote :

@kaputtnik the changes I made set the policy to NEW. The trick that it. did were the additional linker options. But it broke travis builds and will throw errors on system that use old cmake version.

Your proposed switch sets it to the old (deprecated) behavior. Would you be so nice and test the latest change I pushed?

This change only sets the policy to NEW, if it is known to the Cmake version in use. This should work with travis, but we have to wait, though.

Toni Förster (stonerl) wrote :

Btw. AFAIK when using the new behavior, it seems mandatory to set the linker-flags.

See here:

https://stackoverflow.com/questions/23729213/link-error-when-trying-to-build-a-simple-opengl-program

Toni Förster (stonerl) wrote :

@kaputtnik you could try to remove the linker flags bit by bit and see which one is really necessary to compile without linker errors.

8874. By Toni Förster on 2018-10-11

properly set CMP0054 and set message for GLVND linker flags

8875. By Toni Förster on 2018-10-11

add hints to comments for cmake_policy

bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4114. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/440360936.
Appveyor build 3909. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_cmakepolicy-3909.

kaputtnik (franku) wrote :

From my understanding your change will use GLVND on my system. Wouldn't this make bug hunting a bit more complicated, because some bugs may appear on systems using the legacy GL driver but not on systems using GLVND libraries?

Anyway, i have removed the linker flags bit by bit (from right to left), removed the /build directory after each remove and compiled again. The result is:

if("${OpenGL_GL_PREFERENCE}" STREQUAL "GLVND")
   set (CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -lGL")
   message(STATUS "Adding linker flags for GLVND.")
endif()

Removing this last flag (commenting tho whole block) produces the linker errors:

[ 98%] Building CXX object src/economy/test/CMakeFiles/test_economy.dir/test_road.cc.o
/usr/bin/ld: graphic/libgraphic_gl_utils.a(initialize.cc.o): undefined reference to symbol 'glDrawBuffer'
/usr/bin/ld: /usr/lib/libGL.so.1: error adding symbols: DSO missing from command line
collect2: Fehler: ld gab 1 als Ende-Status zurück
make[2]: *** [src/CMakeFiles/widelands.dir/build.make:441: src/widelands] Fehler 1
make[1]: *** [CMakeFiles/Makefile2:1217: src/CMakeFiles/widelands.dir/all] Fehler 2
make[1]: *** Es wird auf noch nicht beendete Prozesse gewartet....
[ 98%] Building CXX object src/economy/test/CMakeFiles/test_economy.dir/test_routing.cc.o
[ 99%] Linking CXX executable test_io_filesystem
/usr/bin/ld: ../../../graphic/libgraphic_gl_utils.a(initialize.cc.o): undefined reference to symbol 'glDrawBuffer'
/usr/bin/ld: /usr/lib/libGL.so.1: error adding symbols: DSO missing from command line
collect2: Fehler: ld gab 1 als Ende-Status zurück
make[2]: *** [src/io/filesystem/test/CMakeFiles/test_io_filesystem.dir/build.make:455: src/io/filesystem/test/test_io_filesystem] Fehler 1
make[1]: *** [CMakeFiles/Makefile2:14128: src/io/filesystem/test/CMakeFiles/test_io_filesystem.dir/all] Fehler 2
[ 99%] Linking CXX executable test_economy
/usr/bin/ld: ../../graphic/libgraphic_gl_utils.a(initialize.cc.o): undefined reference to symbol 'glDrawBuffer'
/usr/bin/ld: /usr/lib/libGL.so.1: error adding symbols: DSO missing from command line
collect2: Fehler: ld gab 1 als Ende-Status zurück
make[2]: *** [src/economy/test/CMakeFiles/test_economy.dir/build.make:472: src/economy/test/test_economy] Fehler 1
make[1]: *** [CMakeFiles/Makefile2:5808: src/economy/test/CMakeFiles/test_economy.dir/all] Fehler 2
make: *** [Makefile:141: all] Fehler 2
mv: der Aufruf von stat für 'src/widelands' ist nicht möglich: Datei oder Verzeichnis nicht gefunden
mv: der Aufruf von stat für '../build/src/website/wl_map_object_info' ist nicht möglich: Datei oder Verzeichnis nicht gefunden
mv: der Aufruf von stat für '../build/src/website/wl_map_info' ist nicht möglich: Datei oder Verzeichnis nicht gefunden

8876. By Toni Förster on 2018-10-12

Add cmake_policy(VERSION) and set CMP0072 for the time beeing.
Removed unnecessary linker flags.

8877. By Toni Förster on 2018-10-12

set CMP0072 to NEW

Toni Förster (stonerl) wrote :

Thanks allot. I changed the the line for the linker flags.

> From my understanding your change will use GLVND on my system. Wouldn't this make bug hunting a
> bit more complicated, because some bugs may appear on systems using the legacy GL driver but not
> on systems using GLVND libraries?

Correct, but if only the new drivers would be installed on a system then only those would be used. And isn't it unavoidable to switch and test with the new drivers as well?

For the sake of bug hunting this could be set to OLD, but this would defeat the purpose of the policy. I think settings this to OLD manually when needed would be sensible.

As for now, this fixes the bug with the buildsystem, silences the warning and compiles when GLVND are used.

@GunChleoc what's your opinion here.

bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4116. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/440557293.
Appveyor build 3913. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_cmakepolicy-3913.

kaputtnik (franku) wrote :

Can we have an output which library is used on the console in the Graphics section? On my system the output, regardless of the used library, is:

Graphics: OpenGL: Version "4.6.0 NVIDIA 410.57"
[..]
**** GRAPHICS REPORT ****
 VIDEO DRIVER x11
 pixel fmt 370546692
 size 1680 1050
**** END GRAPHICS REPORT ****

Toni Förster (stonerl) wrote :

That, I cannot answer.

8878. By Toni Förster on 2018-10-13

add #ifdef for GLVND

Toni Förster (stonerl) wrote :

I guess I found an answer :D

@kaputtnik can you try again, please?

8879. By Toni Förster on 2018-10-13

whitespaces...

kaputtnik (franku) wrote :

Sorry, this do not work, no output here. First i thought thats because the #ifdef is inside the log(), so i pulled it out and give it an own log, like:

#ifdef OpenGL_GL_PREFERENCE
   log(" Using GLVND\n");
#endif

But this didn't work either. Looks to me like the OpenGL_GL_PREFERENCE isn't set at all, because during editing i forgot the ';' and that throughs an error on compiling but after fixing, there is no output either. No idea...

8880. By Toni Förster on 2018-10-13

add compile definition for GLVND

Toni Förster (stonerl) wrote :

Would you be so kind and test again?

kaputtnik (franku) wrote :

You are the master of cmake policies :-D

Original output:

**** GRAPHICS REPORT ****
 VIDEO DRIVER GLVND x11
 pixel fmt 370546692
 size 1680 1050
**** END GRAPHICS REPORT ****

After setting: cmake_policy(SET CMP0072 OLD)

The output is:

**** GRAPHICS REPORT ****
 VIDEO DRIVER x11
 pixel fmt 370546692
 size 1680 1050
**** END GRAPHICS REPORT ****

Lets wait for travis and appveyor and merge this change :-)

review: Approve
kaputtnik (franku) wrote :

I think bunnybot checks the latest latest changes against travis and appveyor automatically, so:

@bunnybot merge

kaputtnik (franku) wrote :

Hm... looks like Ubuntu needs other linkerflags?

I am running archlinux, which is much close to any changes. Each time a new release of a program/library is official it will get soonish into the archlinux repo. Ubuntu is more conservative to this.

Log for revision which has this changes already in:

https://launchpad.net/~widelands-dev/+archive/ubuntu/widelands-daily/+build/15541748/+files/buildlog_ubuntu-cosmic-amd64.widelands_1%3A19-ppa0-bzr8880-201810140801~ubuntu18.10.1_BUILDING.txt.gz

Toni Förster (stonerl) wrote :

It seems so. We would need someone with 18.10 (hasn't been released though) to test.

If no one objects I would append these flag: "-lGLU -lGLEW -lglut" and push directly to trunk.

Toni Förster (stonerl) wrote :

It is using the wrong libraries...

libGL.so: error adding symbols: DSO missing from command line

That's the legacy library. Maybe we need to point to he correct library like so:

-lGL /usr/lib/x86_64-linux-gnu/libOpenGL.so

Toni Förster (stonerl) wrote :

I was wrong about the library. Still no clue as to why it does not compile on 18.10

Toni Förster (stonerl) wrote :

I installed 18.10 in a virtual machine. The problem seems to be cause by GCC-8 which, is standard on 18.10. Compiling with Clang shows no problems at all.

kaputtnik (franku) wrote :

I have both installed, no idea which is used for compiling widelands :-D

$:> pacman -Qs gcc
local/gcc 8.2.1+20180831-1 (base-devel)
    The GNU Compiler Collection - C and C++ frontends

$:> pacman -Qs clang
local/clang 7.0.0-1
    C language family frontend for LLVM

Other packages maybe related on my system:

local/freeglut 3.0.0-2
    Provides functionality for small OpenGL programs
local/glew 2.1.0-1
    The OpenGL Extension Wrangler Library
local/glibmm 2.56.0-1
    C++ bindings for GLib
local/glu 9.0.0-5
    Mesa OpenGL Utility library
local/libglvnd 1.1.0-1
    The GL Vendor-Neutral Dispatch library
local/mesa 18.2.2-1
    An open-source implementation of the OpenGL specification

Toni Förster (stonerl) wrote :

When running the compile.sh script one of the very first line should tell you which compiler is used.

kaputtnik (franku) wrote :

I guess it's gcc 8.x:

-- The C compiler identification is GNU 8.2.1
-- The CXX compiler identification is GNU 8.2.1
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Boost version: 1.68.0
[...]

Why does gcc work here and not on Ubuntu?

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CMakeLists.txt'
2--- CMakeLists.txt 2018-09-26 13:41:40 +0000
3+++ CMakeLists.txt 2018-10-13 18:51:07 +0000
4@@ -1,7 +1,23 @@
5 project (widelands)
6
7 cmake_minimum_required (VERSION 2.8.7)
8-cmake_policy(SET CMP0054 NEW)
9+cmake_policy(VERSION 2.8.7)
10+
11+# This policy is not known to versions prior 3.1 and would result in errors,
12+# if set on such systems. This can be removed when cmake_minimum_required is set
13+# to 3.1 or newer by using:
14+# cmake_policy(VERSION 3.1)
15+if(POLICY CMP0054)
16+ cmake_policy(SET CMP0054 NEW)
17+endif(POLICY CMP0054)
18+
19+# This policy is not known to versions prior 3.11 and would result in errors,
20+# if set on such systems. This can be removed when cmake_minimum_required is set
21+# to 3.11 or newer by using:
22+# cmake_policy(VERSION 3.11)
23+if(POLICY CMP0072)
24+ cmake_policy(SET CMP0072 NEW)
25+endif(POLICY CMP0072)
26
27 include("${CMAKE_SOURCE_DIR}/cmake/WlFunctions.cmake")
28
29@@ -140,6 +156,13 @@
30 message(STATUS "Not using AddressSanitizer.")
31 endif(OPTION_ASAN)
32
33+# This is set to avoid linker errors when using GLVND-libs on Linux
34+if("${OpenGL_GL_PREFERENCE}" STREQUAL "GLVND")
35+ set (CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -lGL")
36+ add_compile_definitions(WL_USE_GLVND)
37+ message(STATUS "Adding linker flags for GLVND.")
38+endif()
39+
40 if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
41 wl_add_flag(WL_COMPILE_DIAGNOSTICS "-Weverything")
42
43
44=== modified file 'src/graphic/graphic.cc'
45--- src/graphic/graphic.cc 2018-08-28 07:53:33 +0000
46+++ src/graphic/graphic.cc 2018-10-13 18:51:07 +0000
47@@ -105,7 +105,11 @@
48 SDL_DisplayMode disp_mode;
49 SDL_GetWindowDisplayMode(sdl_window_, &disp_mode);
50 log("**** GRAPHICS REPORT ****\n"
51+ #ifdef WL_USE_GLVND
52+ " VIDEO DRIVER GLVND %s\n"
53+ #else
54 " VIDEO DRIVER %s\n"
55+ #endif
56 " pixel fmt %u\n"
57 " size %d %d\n"
58 "**** END GRAPHICS REPORT ****\n",