Page MenuHomeWildfire Games

Remove boost "system" from Mac OS build system.
Needs ReviewPublic

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.