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

[hessenfarmer]

I need an appveyor build to further debug this

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3881. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/423709137.
Appveyor build 3679. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_appveyor_fix-3679.

[hessenfarmer (stephan-lutz)]

Ok first Appveyor build was succesful now

Probably I found a solution for the GlBinding issue in this Project:

https://github.com/Vita3K/Vita3K/commit/50d5f370ccb31f01931bbad8a61d56415bb26bf2#diff-a3d8e817ab0c2453cab1fd1f9a601e71

I will test this tonight as well.

[GunChleoc (gunchleoc)]

This doesn't work on Ubuntu 18.04:

/home/bratzbert/sources/widelands/appveyor_fix/src/network/crypto.cc:3:10: fatal error: boost/uuid/detail/sha1.hpp: No such file or directory
 #include <boost/uuid/detail/sha1.hpp>
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

I have added another dirty hack, we need proper version detection here when we fix this properly.

[hessenfarmer (stephan-lutz)]

which Version of boost is included in Ubunbtu 18.04?

in Version 67 both files are present while

<boost/uuid/sha1.hpp> is only including

 <boost/uuid/detail/sha1.hpp>

from 68 onwards only the last one is available and until 66 only the first one was used.

Appveyor by the way is on win7 therefore it hasn't the Problem with the crypto API in boost67 or boost68.

[hessenfarmer (stephan-lutz)]

Hi,

I have a working solution for glbinding v3.0.x.x now. However as Ubuntu bionic is still on 2.1.1 this won't work in Ubuntu so we should probably stay with the downgrade of the packages to keep the code as compatible with different environments as possible.
Boost version could be queried and used for a ifdef statement but glbinding can't .
Perhaps we could save some time on appveyor if we just skip the original installation of boost and glbinding in their original version.
I will add the changes to the appveyor.yml to test this

[GunChleoc (gunchleoc)]

I'd be OK with not supporting older versions of glbinding, because we can still build with glew.

[hessenfarmer (stephan-lutz)]

which environments do we need to support then? That is I wasn't sure which version of glbinding is currently used by the travis environment.

For me it seems to be more efficient to stick to the old versions of glbinding and boost and every other possibly upgraded package until we can switch to newer ones for all supported building Environments.

As the MSYS repo will keep this versions for a long time there is no need to hurry. Only thing is we should add the glbinding downgrade to the MSYS build instructions on the homepage as well.

Although I have no idea what could be the implication on the travis environment.

we could do the upgrade for all environments when the environment with the lowest version of the 3rd party packages upgrades.

By the way building with glew only would have been possible with appveyor as well, but I would prefer to have both options available in every environment.

So I would prefer stay with the current libs as long as reasonable and afterwards do one migration task for all environments at once. (good thing is we already know how to do this for glbinding 3.0.x.x and boost 1.68), but in the end it is your decision so if you want me to incorporate the glbinding solution and the boost switch I can do this.

Otherwise we would need to maintain a huge bunch of switches in the code to support different versions of these packages.

[GunChleoc (gunchleoc)]

The Linux builds in Travis could not handle the new Boost header, while the Mac builds could.

I think having a switch for the Boost versions is still a good idea. Document the changes needed for glbinding in the bug, call it "modernize glbinding" or something and retarget it to Build 21?

I have already pushed a fix for the downgrading directly to trunk to get us going again with AppVeyor ASAP.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3896. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/424652169.
Appveyor build 3694. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_appveyor_fix-3694.

[hessenfarmer (stephan-lutz)]

ok I think I found a cool solution for glbinding as well. Let's wait for the CI results and then review please.
Locally it took Glbinding 3 code and boost 1.66 code (had to downgrade this as I am on Win10) so in msys2 environment the switches are working.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3902. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/424998557.
Appveyor build 3700. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_appveyor_fix-3700.

[GunChleoc (gunchleoc)]

We have a new compiler warning on AppVeyor:

../src/graphic/gl/initialize.cc:101:9: warning: format '%s' expects argument of type 'char*', but argument 2 has type 'std::unique_ptr<glbinding::AbstractValue>::pointer' {aka 'glbinding::AbstractValue*'} [-Wformat=]
     log("%s", call.parameters[i].get());
         ^~~~ ~~~~~~~~~~~~~~~~~~~~~~~~
../src/graphic/gl/initialize.cc:107:9: warning: format '%s' expects argument of type 'char*', but argument 2 has type 'std::unique_ptr<glbinding::AbstractValue>::pointer' {aka 'glbinding::AbstractValue*'} [-Wformat=]
     log(" -> %s", call.returnValue.get());
         ^~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~

See if you can get something interesting to log from the objects - do they still have ->asString().c_str() available?

[hessenfarmer (stephan-lutz)]

they deleted asString unfortunately.
I found the solution above that throws the warning in some other project on the internet.
I don't know how to test it, nor how to avoid it, cause I dont't understand the intention behind these code blocks.

[GunChleoc (gunchleoc)]

I have downloaded the source code for glbinding and there is nothing else to use, so I have changed the placeholder to %p. If it now will complain about the pointers not being void* and about nothing else, we can surround it with a macro that I created to shut it up, like this:

    FORMAT_WARNINGS_OFF;
    molog("Some log %p\n", pointer);
    FORMAT_WARNINGS_ON;

I'd like to see the compiler output first though.

[hessenfarmer (stephan-lutz)]

The Solution with .get was used in their examples. But I have created an issue about that in their github repo

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3917. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/426307714.
Appveyor build 3715. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_appveyor_fix-3715.

[GunChleoc (gunchleoc)]

Compiler warnings are gone now. I also tested that it still compiles with older glbinding version and with glew on my machine.

Thanks for figuring this one out!

@bunnybot merge

review: Approve

[GunChleoc (gunchleoc)]

This has now landed with commit message "I need an appveyor build to further debug this". Please take care in the future to set a proper commit message and add such comments in the comment/description section. I don't always catch them all ;)

[hessenfarmer (stephan-lutz)]

Sorry for the commit message.
However it seems that the code style script for the master release has broken the GLbinding switch in Grapic/gl/system_headers.h, cause it introduced some spaces in the detection if glbing/glbinding.h is present

[hessenfarmer (stephan-lutz)]

something has been changed in network/crypto.cc as well to make travis unhappy

=== modified file 'appveyor.yml'
--- appveyor.yml	2018-09-05 05:51:36 +0000
+++ appveyor.yml	2018-09-09 09:55:19 +0000
@@ -20,9 +20,6 @@
   - cmd: "bash --login -c \"pacman -Su --noconfirm\""
   # Installed required libs
   - cmd: "bash --login -c \"pacman --noconfirm -S mingw-w64-%MINGWSUFFIX%-ninja mingw-w64-%MINGWSUFFIX%-boost mingw-w64-%MINGWSUFFIX%-SDL2_ttf mingw-w64-%MINGWSUFFIX%-SDL2_mixer mingw-w64-%MINGWSUFFIX%-SDL2_image mingw-w64-%MINGWSUFFIX%-glbinding\""
-  - cmd: "bash --login -c \"pacman --noconfirm -U http://repo.msys2.org/mingw/%MINGWSUFFIX%/mingw-w64-%MINGWSUFFIX%-glbinding-2.1.4-1-any.pkg.tar.xz\""
-  # Downgrade Boost, because they changed the crypto API include path
-  - cmd: "bash --login -c \"pacman --noconfirm -U http://repo.msys2.org/mingw/%MINGWSUFFIX%/mingw-w64-%MINGWSUFFIX%-boost-1.66.0-2-any.pkg.tar.xz\""
 
 shallow_clone: true
 

=== modified file 'src/graphic/gl/initialize.cc'
--- src/graphic/gl/initialize.cc	2018-08-28 07:53:33 +0000
+++ src/graphic/gl/initialize.cc	2018-09-09 09:55:19 +0000
@@ -48,6 +48,7 @@
 	SDL_GL_MakeCurrent(sdl_window, gl_context);
 
 #ifdef USE_GLBINDING
+#  ifndef GLBINDING3
 	glbinding::Binding::initialize();
 
 	// The undocumented command line argument --debug_gl_trace will set
@@ -56,8 +57,8 @@
 	// is built using -DOPTION_USE_GLBINDING:BOOL=ON. It is a NoOp for GLEW.
 	if (trace == Trace::kYes) {
 		setCallbackMaskExcept(
-		   glbinding::CallbackMask::After | glbinding::CallbackMask::ParametersAndReturnValue,
-		   {"glGetError"});
+			glbinding::CallbackMask::After | glbinding::CallbackMask::ParametersAndReturnValue,
+			{"glGetError"});
 		glbinding::setAfterCallback([](const glbinding::FunctionCall& call) {
 			log("%s(", call.function->name());
 			for (size_t i = 0; i < call.parameters.size(); ++i) {
@@ -80,6 +81,47 @@
 			// }
 		});
 	}
+#  else
+	const glbinding::GetProcAddress get_proc_address = [](const char *name) {
+		return reinterpret_cast<glbinding::ProcAddress>(SDL_GL_GetProcAddress(name));
+		};
+	glbinding::Binding::initialize(get_proc_address, true);
+
+	// The undocumented command line argument --debug_gl_trace will set
+	// Trace::kYes. This will log every OpenGL call that is made, together with
+	// arguments, return values and glError status. This requires that Widelands
+	// is built using -DOPTION_USE_GLBINDING:BOOL=ON. It is a NoOp for GLEW.
+	if (trace == Trace::kYes) {
+		glbinding::setCallbackMaskExcept(
+			glbinding::CallbackMask::After | glbinding::CallbackMask::ParametersAndReturnValue,
+			{"glGetError"});
+		glbinding::setAfterCallback([](const glbinding::FunctionCall& call) {
+			log("%s(", call.function->name());
+			for (size_t i = 0; i < call.parameters.size(); ++i) {
+				FORMAT_WARNINGS_OFF;
+				log("%p", call.parameters[i].get());
+				FORMAT_WARNINGS_ON;
+				if (i < call.parameters.size() - 1)
+					log(", ");
+			}
+			log(")");
+			if (call.returnValue) {
+				FORMAT_WARNINGS_OFF;
+				log(" -> %p", call.returnValue.get());
+				FORMAT_WARNINGS_ON;
+			}
+			const auto error = glGetError();
+			log(" [%s]\n", gl_error_to_string(error));
+			// The next few lines will terminate Widelands if there was any OpenGL
+			// error. This is useful for super aggressive debugging, but probably
+			// not for regular builds. Comment it in if you need to understand
+			// OpenGL problems.
+			// if (error != GL_NO_ERROR) {
+			// std::raise(SIGINT);
+			// }
+		});
+	}
+#  endif
 #else
 	// See graphic/gl/system_headers.h for an explanation of the next line.
 	glewExperimental = GL_TRUE;

=== modified file 'src/graphic/gl/system_headers.h'
--- src/graphic/gl/system_headers.h	2018-04-07 16:59:00 +0000
+++ src/graphic/gl/system_headers.h	2018-09-09 09:55:19 +0000
@@ -37,6 +37,16 @@
 #ifdef USE_GLBINDING
 #include <glbinding/Binding.h>
 #include <glbinding/gl/gl.h>
+
+// testing for the presence of glbinding.h to determine whether we have a glbinding version newer then 2.1.4
+#ifdef __has_include
+#  if __has_include(<glbinding/glbinding.h>)
+#    include <glbinding/glbinding.h>
+#    include <glbinding/ProcAddress.h>
+#    define GLBINDING3
+#  endif
+#endif
+
 // This fakes that most other gl bindings define gl functions in the public namespace.
 CLANG_DIAG_OFF("-Wheader-hygiene")
 using namespace gl;

=== modified file 'src/network/crypto.cc'
--- src/network/crypto.cc	2017-11-26 20:55:56 +0000
+++ src/network/crypto.cc	2018-09-09 09:55:19 +0000
@@ -1,6 +1,10 @@
 #include "network/crypto.h"
 
-#include <boost/uuid/sha1.hpp>
+#if BOOST_VERSION > 106700
+  #include "boost/uuid/detail/sha1.hpp"
+#else
+  #include "boost/uuid/sha1.hpp"
+#endif
 
 namespace crypto {
 

=== modified file 'src/network/crypto.h'
--- src/network/crypto.h	2018-04-07 16:59:00 +0000
+++ src/network/crypto.h	2018-09-09 09:55:19 +0000
@@ -22,6 +22,8 @@
 
 #include <string>
 
+#include "boost/version.hpp"
+
 namespace crypto {
 
 /**