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

Fix the build on OpenBSD #1333

Merged
merged 2 commits into from Apr 16, 2017

Conversation

Projects
None yet
2 participants
@oldlaptop
Contributor

oldlaptop commented Apr 13, 2017

FSO doesn't build from source on OpenBSD anymore. This patch corrects the only significant problem: the iterator in LuaTable.h/cpp known as _L clashes with a #define in OpenBSD's standard headers (ctypes.h to be specific). The new name proposed here is simply taken from the type.

One other problem still prevents building on OpenBSD at this time: an assert in code/scripting/ade.h that calls std::is_trivially_copyable, available in libstdc++ >= 5 (where OpenBSD has 4.9 in ports/packages). OpenBSD on i386/amd64 will probably use clang/libc++ in the future, so this issue will go away anyhow. For the record, the following diff on top of this PR fixes the build on OpenBSD 6.1-current at this time:

diff --git a/code/scripting/ade.h b/code/scripting/ade.h
index 8478ea3ea..04cf920ef 100644
--- a/code/scripting/ade.h
+++ b/code/scripting/ade.h
@@ -226,7 +226,7 @@ template<class StoreType>
 class ade_obj: public ade_lib_handle {
  public:
        // Make sure that the stored type is compatible with our requirements
-       static_assert(std::is_trivially_copyable<StoreType>::value, "ADE object types must be trivially copyable!");
+       //static_assert(std::is_trivially_copyable<StoreType>::value, "ADE object types must be trivially copyable!");
 
        ade_obj(const char* in_name, const char* in_desc, const ade_lib_handle* in_deriv = NULL) {
                ade_table_entry ate;
Show outdated Hide outdated code/scripting/lua/LuaTable.h
@@ -19,7 +19,7 @@ class LuaTable;
* be in the exact same state when you call the next method of this class.
*/
class LuaTableIterator {
lua_State* _L = nullptr;
lua_State* L_State = nullptr;

This comment has been minimized.

@asarium

asarium Apr 13, 2017

Member

The rest of the class uses the format _varname so this field should do the same. _luaState would be an acceptable name.

@asarium

asarium Apr 13, 2017

Member

The rest of the class uses the format _varname so this field should do the same. _luaState would be an acceptable name.

This comment has been minimized.

@oldlaptop

oldlaptop Apr 13, 2017

Contributor

Ah. I was reluctant to do that originally because leading underscores are theoretically reserved for the standard headers...

@oldlaptop

oldlaptop Apr 13, 2017

Contributor

Ah. I was reluctant to do that originally because leading underscores are theoretically reserved for the standard headers...

This comment has been minimized.

@asarium

asarium Apr 13, 2017

Member

Only global names beginning with an underscore are reserved. For names in non-global namespaces (e.g. a class) the name may begin with an underscore but the next letter must be lower case which also explains why _L is an invalid name.

@asarium

asarium Apr 13, 2017

Member

Only global names beginning with an underscore are reserved. For names in non-global namespaces (e.g. a class) the name may begin with an underscore but the next letter must be lower case which also explains why _L is an invalid name.

@asarium

This comment has been minimized.

Show comment
Hide comment
@asarium

asarium Apr 13, 2017

Member

The static assert is needed to ensure that no runtime errors occur but it could be conditionally enabled only if support for is_trivially_copyable is detected. This could be done using a CMake platform check. See platformChecks.cmake for examples on how that is done.

Member

asarium commented Apr 13, 2017

The static assert is needed to ensure that no runtime errors occur but it could be conditionally enabled only if support for is_trivially_copyable is detected. This could be done using a CMake platform check. See platformChecks.cmake for examples on how that is done.

@asarium asarium added this to the Release 3.8 milestone Apr 13, 2017

@asarium asarium added the bugfix label Apr 13, 2017

s/_L/L_State
 The iterator known as _L, besides having an absurdly poor name,
 clashes with standard headers on OpenBSD -current at the time
 of writing. This commit is the minimum needed to build on
 OpenBSD with an updated C++ standard library.

@oldlaptop oldlaptop changed the title from (Mostly) fix the build on OpenBSD to Fix the build on OpenBSD Apr 14, 2017

@oldlaptop

This comment has been minimized.

Show comment
Hide comment
@oldlaptop

oldlaptop Apr 14, 2017

Contributor

I've just added a platform check for is_trivially_copyable, can split off into another PR if desired.

Contributor

oldlaptop commented Apr 14, 2017

I've just added a platform check for is_trivially_copyable, can split off into another PR if desired.

Show outdated Hide outdated cmake/platformChecks.cmake
@@ -16,6 +17,9 @@ CHECK_TYPE_SIZE("max_align_t" MAX_ALIGN_T)
set(CMAKE_EXTRA_INCLUDE_FILES "cstddef")
CHECK_TYPE_SIZE("std::max_align_t" STD_MAX_ALIGN_T LANGUAGE CXX)
set(CMAKE_EXTRA_INCLUDE_FILES "type_traits")
CHECK_TYPE_SIZE("std::is_trivially_copyable<int>" STD_IS_TRIVIALLY_COPYABLE LANGUAGE CXX)
MESSAGE(${HAVE_STD_IS_TRIVIALLY_COPYABLE})

This comment has been minimized.

@asarium

asarium Apr 14, 2017

Member

This seems to be working properly, although our Travis GCC config fails this test for some reason although it compiled the previous version properly. That doesn't matter though since Clang works properly.

Once you remove this test message, this is ready to be merged.

@asarium

asarium Apr 14, 2017

Member

This seems to be working properly, although our Travis GCC config fails this test for some reason although it compiled the previous version properly. That doesn't matter though since Clang works properly.

Once you remove this test message, this is ready to be merged.

This comment has been minimized.

@oldlaptop

oldlaptop Apr 15, 2017

Contributor

Ah, dammit. Thanks for catching... :)

@oldlaptop

oldlaptop Apr 15, 2017

Contributor

Ah, dammit. Thanks for catching... :)

This comment has been minimized.

@oldlaptop

oldlaptop Apr 15, 2017

Contributor

My local setup passes the test, by the way (6.3.0 in Debian testing). Unfortunately my machines all have either 'old' gcc (4.9) or new gcc (>6) at the moment; I'll try to investigate behavior on gcc 5 (Debian packages 5.4 at the moment, in testing).

@oldlaptop

oldlaptop Apr 15, 2017

Contributor

My local setup passes the test, by the way (6.3.0 in Debian testing). Unfortunately my machines all have either 'old' gcc (4.9) or new gcc (>6) at the moment; I'll try to investigate behavior on gcc 5 (Debian packages 5.4 at the moment, in testing).

This comment has been minimized.

@oldlaptop

oldlaptop Apr 15, 2017

Contributor

The reason gcc5 fails the test appears to be quite simple and stupid - from CMakeError.txt:

/usr/include/c++/5/bits/c++0x_warning.h:32:2: error: #error This file requires compiler and library support for the ISO C++ 2011 standard. This support must be enabled with the -std=c++11 or -std=gnu++11 compiler options.

I should be able to push a fix shortly.

@oldlaptop

oldlaptop Apr 15, 2017

Contributor

The reason gcc5 fails the test appears to be quite simple and stupid - from CMakeError.txt:

/usr/include/c++/5/bits/c++0x_warning.h:32:2: error: #error This file requires compiler and library support for the ISO C++ 2011 standard. This support must be enabled with the -std=c++11 or -std=gnu++11 compiler options.

I should be able to push a fix shortly.

Show outdated Hide outdated cmake/platformChecks.cmake
@@ -16,6 +17,10 @@ CHECK_TYPE_SIZE("max_align_t" MAX_ALIGN_T)
set(CMAKE_EXTRA_INCLUDE_FILES "cstddef")
CHECK_TYPE_SIZE("std::max_align_t" STD_MAX_ALIGN_T LANGUAGE CXX)
set(CMAKE_EXTRA_INCLUDE_FILES "type_traits")
set(CMAKE_REQUIRED_FLAGS "-std=c++11") # required for g++ <= 5

This comment has been minimized.

@asarium

asarium Apr 15, 2017

Member

Since this is only relevant for GCC it should be surrounded by a check for that compiler. if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU") should work since that is the same string our toolchain detection uses.

Since this applies to all the C++ checks, could you insert that code above the other CHECK_TYPE_SIZE? That will make the results more consistent.

@asarium

asarium Apr 15, 2017

Member

Since this is only relevant for GCC it should be surrounded by a check for that compiler. if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU") should work since that is the same string our toolchain detection uses.

Since this applies to all the C++ checks, could you insert that code above the other CHECK_TYPE_SIZE? That will make the results more consistent.

This comment has been minimized.

@oldlaptop

oldlaptop Apr 15, 2017

Contributor

Done.

@oldlaptop

oldlaptop Apr 15, 2017

Contributor

Done.

Show outdated Hide outdated cmake/platformChecks.cmake
CHECK_TYPE_SIZE("std::max_align_t" STD_MAX_ALIGN_T LANGUAGE CXX)
set(CMAKE_EXTRA_INCLUDE_FILES)
if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
set(CMAKE_REQUIRED_FLAGS "-std=c++11") # required for g++ <= 5

This comment has been minimized.

@asarium

asarium Apr 15, 2017

Member

That's not what I meant. Only this line should be in the if body. The other lines are required on all platforms but -std=c++11 is only relevant for GCC.

This is how it should look like:

if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
    set(CMAKE_REQUIRED_FLAGS "-std=c++11") # required for g++ <= 5
endif()

set(CMAKE_EXTRA_INCLUDE_FILES "cstddef")
CHECK_TYPE_SIZE("std::max_align_t" STD_MAX_ALIGN_T LANGUAGE CXX)
set(CMAKE_EXTRA_INCLUDE_FILES "type_traits")
CHECK_TYPE_SIZE("std::is_trivially_copyable<int>" STD_IS_TRIVIALLY_COPYABLE LANGUAGE CXX)
set(CMAKE_EXTRA_INCLUDE_FILES)

set(CMAKE_REQUIRED_FLAGS)
@asarium

asarium Apr 15, 2017

Member

That's not what I meant. Only this line should be in the if body. The other lines are required on all platforms but -std=c++11 is only relevant for GCC.

This is how it should look like:

if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
    set(CMAKE_REQUIRED_FLAGS "-std=c++11") # required for g++ <= 5
endif()

set(CMAKE_EXTRA_INCLUDE_FILES "cstddef")
CHECK_TYPE_SIZE("std::max_align_t" STD_MAX_ALIGN_T LANGUAGE CXX)
set(CMAKE_EXTRA_INCLUDE_FILES "type_traits")
CHECK_TYPE_SIZE("std::is_trivially_copyable<int>" STD_IS_TRIVIALLY_COPYABLE LANGUAGE CXX)
set(CMAKE_EXTRA_INCLUDE_FILES)

set(CMAKE_REQUIRED_FLAGS)

This comment has been minimized.

@oldlaptop

oldlaptop Apr 16, 2017

Contributor

Ah, sorry.

@oldlaptop

oldlaptop Apr 16, 2017

Contributor

Ah, sorry.

add platform check for std::is_trivially_copyable
 Yes, the template instantiation appears necessary for cmake to
 detect the type. If there's a better type for it than int, I'm
 open to suggestions.
@asarium

This comment has been minimized.

Show comment
Hide comment
@asarium

asarium Apr 16, 2017

Member

Yes, that looks good now.

Member

asarium commented Apr 16, 2017

Yes, that looks good now.

@asarium asarium merged commit 8850efc into scp-fs2open:master Apr 16, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment