Page MenuHomeWildfire Games

Remove boost "system" from Mac OS build system.
AbandonedPublic

Authored by Stan on Dec 9 2018, 3:21 PM.

Details

Summary

As noticed by fabio, system is not needed anymore to build the game on OSX.

Tested on OSX 10.9.5 Mavericks

Test Plan

Test it on other Mac versions, and verify everything still works fine.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 6767
Build 11130: Vulcan BuildJenkins

Event Timeline

Stan created this revision.Dec 9 2018, 3:21 PM
Vulcan added a subscriber: Vulcan.Dec 9 2018, 3:25 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/821/

fabio accepted this revision.Dec 9 2018, 4:58 PM
This revision is now accepted and ready to land.Dec 9 2018, 4:58 PM

If you have some time can you review it wraitii ?

wraitii requested changes to this revision.Jan 5 2019, 10:05 AM

This does seem to compile, but the change isn't complete, you need to remove the line in extern_libs5.lua in build/premake.

This revision now requires changes to proceed.Jan 5 2019, 10:05 AM
Stan updated this revision to Diff 7263.Jan 5 2019, 3:29 PM
  • Remove boost-system-mt extern_libs5.lua

@fabio, seems that boost-system is still linked in the extern_libs5.lua for Unix. Maybe another patch ?

Vulcan added a comment.Jan 5 2019, 3:46 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/918/

fabio marked an inline comment as done.Jan 7 2019, 4:02 PM
fabio added inline comments.
build/premake/extern_libs5.lua
205

Unrelated to osx, but I think

, os.findlib("boost_system-mt") and "boost_system-mt" or "boost_system"

can also be removed here.

Stan updated this revision to Diff 7305.Jan 7 2019, 10:55 PM
Stan retitled this revision from Remove boost "system" from build-osx-libs.sh to Remove boost "system" from build systems..
Stan added a subscriber: elexis.

@fabio @elexis @vladislavbelov can you try on linux ? I removed boost-system from there as well ?

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/942/

fabio accepted this revision.Jan 8 2019, 1:25 PM

Untested, but LGTM.

fabio added a comment.Jan 8 2019, 2:10 PM

Looks Good To Me
:)

Itms added a comment.Jan 12 2019, 1:04 AM

This doesn't work for me. Linking of pyrogenesis fails with undefined references to boost::system stuff. I will retry tomorrow with a clean checkout.

Doesn't work for me either (Arch Linux, Boost 1.68).

Looks like Linux systems need the boost_system library added explicitly.

Itms requested changes to this revision.Jan 12 2019, 10:17 AM

I think we do not need to include boost::system because we don't use anything from it, but we need to link it because boost::filesystem depends on it. I think only the build-osx change should be kept, as bootstrap is clever enough to build the boost::system lib while now excluding it from distributed includes.

This revision now requires changes to proceed.Jan 12 2019, 10:17 AM
Stan added a comment.Jan 12 2019, 12:11 PM

Thanks for testing @Itms and @s0600204 ! I will restore the patch to only touch Macos then. Would be nice if @trompetin17 and @vladislavbelov could test it.

Stan planned changes to this revision.Jan 12 2019, 12:11 PM
Stan updated this revision to Diff 7314.
Stan retitled this revision from Remove boost "system" from build systems. to Remove boost "system" from Mac OS build system..

Only change MacOS build system.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/947/

fabio accepted this revision.Jan 28 2019, 1:23 PM
wraitii accepted this revision.Apr 13 2019, 10:46 AM

I'm not sure how useful this is. We're linking statically anyways and this is OSX only so...

Stan added a comment.Apr 20 2019, 5:38 PM

I though Mac OS had a tendency to link dynamically when it could ? It isn't a big change, it more of a cleanup. If you have a good argument against committing it, I can just abandon the revision :)

I feel extremely "meh" about this :p
Commit it if you want, don't if you don't. It's unlikely I'll do it myself is my point.

fabio added a comment.Apr 20 2019, 5:57 PM

Why not? system was already removed from Linux since it is no longer used, OS X should follow...

Stan added a comment.Apr 20 2019, 6:06 PM

Actually it was not removed, else I would have kept the previous diff :)

fabio added a comment.Apr 21 2019, 7:49 AM

I said system, which got removed from Linux times ago, as I wrote here:
https://trac.wildfiregames.com/ticket/4362#comment:26
And which is removed in the second hunk of the patch.

Stan added a comment.Apr 21 2019, 8:53 AM
In D1691#75466, @fabio wrote:

I said system, which got removed from Linux times ago, as I wrote here:
https://trac.wildfiregames.com/ticket/4362#comment:26
And which is removed in the second hunk of the patch.

But when we removed system_mt from premake5.lua it failed to build on @Itms' computer, didn't it, It's in the comment above

fabio added a comment.EditedApr 21 2019, 10:17 AM

Ok, so just commit the second hunk?

Stan added a comment.Apr 21 2019, 3:45 PM

Both hunks are Mac OS :) I was just stating that since we didn't remove it for Linux it wasn't an argument for doing it on Mac ;) I don't see a problem with committing this but since @wraitii seems to think this is not worth it I wanted to know why.

I tried to test this on macOS 10.14 Mojave, but couldn't get it to work.

  • clean checkout trunk at r22391 with this patch applied, (and another patch applied to skip Nettle/GnuTLS, in favour of Homebrew symlinks, per #5453 and D1483, which works without this patch).
  • building libs. Completes without errors.
  • building workspaces. Completes without errors.
  • building the game (build/workspaces/gcc$ make -j5). This fails.
==== Building pyrogenesis (release) ====
Linking pyrogenesis
ld: warning: directory not found for option '-L/lib'
Undefined symbols for architecture x86_64:
  "boost::system::system_category()", referenced from:
      boost::filesystem::detail::canonical(boost::filesystem::path const&, boost::filesystem::path const&, boost::system::error_code*) in libboost_filesystem-mt.a(operations.o)
      boost::filesystem::detail::symlink_status(boost::filesystem::path const&, boost::system::error_code*) in libboost_filesystem-mt.a(operations.o)
      boost::filesystem::detail::read_symlink(boost::filesystem::path const&, boost::system::error_code*) in libboost_filesystem-mt.a(operations.o)
      boost::filesystem::detail::copy(boost::filesystem::path const&, boost::filesystem::path const&, boost::system::error_code*) in libboost_filesystem-mt.a(operations.o)
      (anonymous namespace)::error(int, boost::filesystem::path const&, boost::filesystem::path const&, boost::system::error_code*, char const*) in libboost_filesystem-mt.a(operations.o)
      boost::filesystem::detail::create_directories(boost::filesystem::path const&, boost::system::error_code*) in libboost_filesystem-mt.a(operations.o)
      boost::filesystem::detail::create_directory(boost::filesystem::path const&, boost::system::error_code*) in libboost_filesystem-mt.a(operations.o)
      ...
  "boost::system::generic_category()", referenced from:
      boost::filesystem::detail::canonical(boost::filesystem::path const&, boost::filesystem::path const&, boost::system::error_code*) in libboost_filesystem-mt.a(operations.o)
      boost::filesystem::detail::create_directories(boost::filesystem::path const&, boost::system::error_code*) in libboost_filesystem-mt.a(operations.o)
      boost::filesystem::detail::permissions(boost::filesystem::path const&, boost::filesystem::perms, boost::system::error_code*) in libboost_filesystem-mt.a(operations.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[1]: *** [../../../binaries/system/pyrogenesis] Error 1
make: *** [pyrogenesis] Error 2

There are also two compiler warnings that I don't recall seeing when building the game without this patch:

<snip>
glooxwrapper.cpp
precompiled.cpp
In file included from ../../../source/lobby/glooxwrapper/glooxwrapper.cpp:20:
../../../source/lobby/glooxwrapper/glooxwrapper.h:642:10: warning: private field 'm_Owned' is not used [-Wunused-private-field]
                                bool m_Owned;
                                     ^
<snip>
CCmpUnitMotion.cpp
CCmpUnitRenderer.cpp
../../../source/simulation2/components/CCmpUnitMotion.cpp:1365:129: warning: unused parameter 'target' [-Wunused-parameter]
bool CCmpUnitMotion::MoveToPointRange(entity_pos_t x, entity_pos_t z, entity_pos_t minRange, entity_pos_t maxRange, entity_id_t target)
                                                                                                                                ^
<snip>
Stan added a comment.Thu, Jun 20, 8:27 AM

Yeah I don't think that will work ever. I'm not sure where @fabio saw that it could be removed.

The first warning m_owned is old and could use a patch the second I guess is a regression introduced by @wraitii.

wraitii resigned from this revision.Thu, Jun 20, 8:40 AM
In D1691#83383, @Stan wrote:

Yeah I don't think that will work ever. I'm not sure where @fabio saw that it could be removed.

Weird that it compiled when I tried above. Guessing my compilation setup is a bit too customised for these kind of patches.

The first warning m_owned is old and could use a patch the second I guess is a regression introduced by @wraitii.

Yup, it's there since rP22352. The parameter only ever was INVALID_ENTITY. I did notice the warning at some point, just need to make sure I don't forget about it. Concern can be raised so I don't (but I can't raise it myself).

Stan abandoned this revision.Thu, Jun 20, 8:52 AM

I'm abandoning this :) It probably can be reopened when we actually remove these references.