Mac SDK change from i386 to x86_64, compile with visibility=hidden #29

memem359 · 2017-05-25T21:22:36Z

This is intended to switch the Mac SDK build from 32 bit (i386) to 64 bit (x86_64), and to use the compiler flag -fvisibility=hidden (which is already in use for the FO code).

The README.md change is based on what I see with the Travis build.

Besides the x86 and visibility changes to CMakeLists.txt, the BUILD_SHARED_LIBS value was not being modified by cmake, due to it being a CACHE variable. Instead of using FORCE, I figured I would make the change where it is first assigned.

The new patch file is to make the Boost libraries work with the visibility hidden flag.
The Boost code only works with that compile flag when building dynamic shared libraries (plus some other preprocessor macros that are hard to find). Since the FO-SDK is making static libraries, this causes a bunch of variables to remain hidden, resulting in linking errors. I hand-assembled patches on a few of the config files to fix that situation.

After this PR is merged and run to make the Mac 64 bit libraries, the dep folder (with static libraries, frameworks, and includes) would need to be copied to the FO build (inside the Xcode folder), along with the PR to switch that Mac code to 64 bit.
That PR is 1585
freeorion/freeorion#1585

adrianbroher · 2017-05-26T12:18:59Z

The new patch file is to make the Boost libraries work with the visibility hidden flag.
The Boost code only works with that compile flag when building dynamic shared libraries (plus some other preprocessor macros that are hard to find). Since the FO-SDK is making static libraries, this causes a bunch of variables to remain hidden, resulting in linking errors. I hand-assembled patches on a few of the config files to fix that situation.

I'm not interested in carrying garbage outside of boost upstream. Use whatever mechanism boost build provides. If this is not possible then your approach is wrong and upstream didn't intend to let it work this way.

memem359 · 2017-05-26T14:30:35Z

@adrianbroher
I dug into the Boost code quite a bit.
For the Boost version being used, they have something similar to the FO file Export.h.
The "attribute visibility default" is only added when certain preprocessor macros are set.

There is no option where we can use Boost static libraries and still have the right visibility for the code.
Take a look at the new patch for boost/filesystem/config.hpp. We want:
define BOOST_FILESYSTEM_DECL BOOST_SYMBOL_EXPORT
to get the right code visibility, but that is hidden behind requiring the macro BOOST_ALL_DYN_LINK.
I am assuming that means Boost shared libraries, which we are explicitly NOT doing for the Mac build.

I did a lot of searching on the web. I have no idea why the Boost devs chose to fix their visibility=hidden problems this way.
(I don't know if we can assume it was intended, as they may have overlooked this particular choice of settings. I don't know why BOOST_SYMBOL_EXPORT is only for dynamic linking.)

If you don't want to use the "outside garbage", these are your choices for the Mac builds:

Turn off -visibility=hidden everywhere
Use the Boost macro BOOST_ALL_DYN_LINK, which in turn means FO and FO-SDK should switch from static to shared libraries.
Show me know how to get the "attribute visibility default" preprocessor macro to run without using BOOST_ALL_DYN_LINK

I'm fine with any of those.

I was figured the local patch of config.hpp files would be the most painless.
(I going to do more investigating, and if it does look like a bug, submit a message to Boost. But I didn't want to hold up FO development while doing that.)

Edit: I just submitted a ticket to Boost. Hopefully I'll get some feedback, either what I should do differently, or if Boost has an oversight.

Another edit: This PR is outdated. I am working on a very different set of changes, so this is a placeholder only.

memem359 · 2017-06-09T17:04:38Z

Revamped PR.
Should not interfere with the Windows build at all.
This is necessary to get the Logger working correctly on the Mac side.

Mac build switched from i386 to x86_64.
README instructions slightly modified.
Boost libraries for the Mac are shared libraries instead of static now.
There is one new patch for 3 Boost include files, that have preprocessor macros that only work for the Windows build. This patch will make it work on the Mac. One file was already fixed for Boost 1.60. (The SDK uses v1.59.) For the remaining two files, I have submitted PRs to Boost, and made the changes consistent with other config files that have these types of macros.

The SDK build uses CMake.
I did not touch the SDK Xcode project file at all.

The Mac libraries for this SDK will require the FO PR 1585.
That PR has been updated as well.

adrianbroher · 2017-06-13T20:03:43Z

CMakeLists.txt

@@ -6,12 +6,16 @@ endif()

 cmake_minimum_required(VERSION ${REQUIRED_CMAKE_VERSION})

-option(BUILD_SHARED_LIBS "Build external projects as shared libraries." ON)
+if(APPLE)


Revert this section.

The previous version did not work correctly.

"BUILD_SHARED_LIBS" is a Cache variable, which means "set(BUILD_SHARED_LIBS OFF)" on line 56 fails to change the value to OFF. There were two ways to fix this: FORCE the change on line 56, or change it when first defined. I thought it would be clearer to set the value up front, rather than having a change in value buried in the code.

@adrianbroher is memem359's reasoning here and below acceptable to you? If so could you change the status of your review to indicate things are OK with you now, and if not then could you give him a specific solution? It would be great to get this PR resolved.

If so could you change the status of your review to indicate things are OK with you now

They are not otherwise I would have already merged this.

and if not then could you give him a specific solution?

Already given month ago.

Except that specific solution brings back a problem (CACHE variable not being changed).
I don't understand what the objection is.
I'm willing to work with you, but I have to understand what you are looking for.

The second option I listed on June 13th makes fewer changes and does not change the initial part of the file, so I have redone the commits to use that.

adrianbroher · 2017-06-13T20:03:55Z

CMakeLists.txt

 option(CMAKE_VERBOSE_MAKEFILE "Build with verbose Makefile output." OFF)
 set(DIST_VERSION 7 CACHE STRING "FreeOrionSDK release version.")
 set(CMAKE_CXX_STANDARD 11 CACHE STRING "Set the C++ standard the dependencies should be build with.")
 set(CMAKE_OSX_DEPLOYMENT_TARGET 10.9 CACHE STRING "Set the minimum OSX deployment target version")
-set(CMAKE_OSX_ARCHITECTURES i386 CACHE STRING "Set the architecture the universal binaries for OSX should be built for")
+set(CMAKE_OSX_ARCHITECTURES x86_64 CACHE STRING "Set the architecture the binaries for OSX should be built for")


Separate commit.

Made into a separate commit.

adrianbroher · 2017-06-13T20:04:38Z

CMakeLists.txt

@@ -677,6 +694,54 @@ elseif(APPLE)
        ${CMAKE_COMMAND} -E remove_directory
        ${SDK_INSTALL_DIR}
    )
+
+# Need to make Mac-specific changes to the dylib install_name for


That's a b2 job, not an SDK build one.

According to http://boost.2283326.n4.nabble.com/Need-to-add-quot-install-name-quot-to-dylibs-on-OS-X-td2692930.html

http://www.boost.org/build/doc/html/bbv2/overview/builtins/features.html#bbv2.reference.features.dll-path should do the same job in a less messy way.

I noticed these entries last week. Unless I'm doing something wrong, it doesn't work.

I added "dll-path=@executable_path/../SharedSupport" to the "APPEND BOOST_CONFIGURE_FLAGS" (at line 530 of CMakeLists.txt).
The install_names of the Boost shared libraries did not change. They are still in the format of (at)rpath/libboost_(name).dylib, with nothing to indicate that I set "dll_path" to something different.

There are other people who have noted that b2 (for the Mac) cannot handle this job.
Or at least, it was broken in 1.45 and not working until 1.62 (and we are using 1.59).
https://stackoverflow.com/questions/3431619/how-to-force-boost-to-use-rpath

To be clear, I wanted to use b2 to take care of the install_name paths.
I spent several days in June trying to get it to work.
I was unable to get b2 to do what was needed.

I had found the same online links that adrianbroher listed.
I tried it before he suggested it.
I tried it after he suggested it.
I just retried it now.
Still doesn't work (on the Mac).

There is a solution that works right now, which is what I committed:
having cmake use install_name_tool to modify the shared libraries after b2 is finished.
Unless someone can point out a working solution using v1.59 of boost, I don't think it is possible to make this a "b2 job".

This is a regression in boost.
Can we use the working solution and mark it as compensating for the regression in boost that should be reverted when we adopt boost v1.62?

So, that means this issue hasn't been resolved even with boost 1.63? That's unfortunate.

@adrianbroher, as your objection to resort to the hack using install_name_tool is preventing us from using that fix, and the solutions you suggested apparently don't work, how do you suggest to proceed?

I don't want to do anything that you perceive as subverting all the effort you've put into the SDK, but we need to fix this issue. Honestly, I'm out of ideas... 😕

@Vezzra, I get the feeling that there is nothing to "fix", that Boost does not support adjusting the install_path (on OSX).

They felt the need to add this sentence to the documentation: "Note that on Mac OSX, the paths are unconditionally hardcoded by the linker, and it is not possible to disable that behaviour." I suspect this means that the only options are to pass-through a flag (with the path) to the linker, or to modify the install_paths after the libraries are built. The former only works if we are using b2 to make a single library, and the latter is what I submitted (using install_name_tool in the cmake script).

Ok, I see. @adrianbroher, in light of these findings, how do you suggest to proceed?

The former only works if we are using b2 to make a single library

I assume that means building only one of the boost libraries by a single invocation of the b2 build system? If yes, would a possible fix be to change the cmake scripts so that each boost library gets built by separate invocations of the b2 build system?

That said, that solution isn't really better than using install_name_tool, IMO. However, if @adrianbroher prefers it, I can live with that. @adrianbroher?

I was thinking of the linker flag "-install_path", where the absolute path can be defined. Since that will include the name of the library, it would seem to only work 1 library at a time.

I think having b2 make one library at a time is a rather ugly solution. Some of the boost libraries depend on others, so we would have to reverse engineer a lot of stuff that Boost is taking care of automatically. (What order should the libraries be built? Will there be a problem with b2 sharing a library where the install_path has been changed from the default?)

The original request was, "That's a b2 job, not an SDK build one." Everything I can find suggests that b2 cannot do that job (with OSX). Someone would need to point out detailed instructions on how b2 can do it, in order for me to make the requested change.

I was thinking of the linker flag "-install_path", where the absolute path can be defined. Since that will include the name of the library, it would seem to only work 1 library at a time.

Ah, ok, I understand.

Some of the boost libraries depend on others, so we would have to reverse engineer a lot of stuff that Boost is taking care of automatically.

Ugh. That indeed sounds messy. So, not going to happen.

That means the only working solution still on the table is using install_name_tool. Unfortunately, lacking any further feedback from @adrianbroher, we're kind of stuck. Of course, we could just overrule his objections, but I really, really don't want to do this. ☹️

memem359 · 2017-09-02T12:08:35Z

There were 3 requested changes from June 13. One (separate commit) was done. The other 2 requests were not possible, which I noted. (Reverting the early lines in CMakeLists.txt brings back an error, and as far as I can tell Boost/b2 for the Mac cannot do the work that I'm making the SDK do.)

I don't know if that is sufficient in response, or if other actions were preferred.

adrianbroher · 2017-11-05T14:52:04Z

Rebased and merged with b9cfa10

memem359 mentioned this pull request May 26, 2017

Switch Mac build from 32 to 64 bit freeorion/freeorion#1585

Closed

memem359 force-pushed the mac64-visibility branch from dcb7ec3 to 3b85423 Compare June 9, 2017 16:49

adrianbroher suggested changes Jun 13, 2017

View reviewed changes

memem359 added 2 commits June 13, 2017 18:06

Switch Mac from i386 to x86_64

a5e8436

Update README instructions for correct cmake generator

7075ee4

memem359 force-pushed the mac64-visibility branch from 3b85423 to 2a8e021 Compare June 13, 2017 22:10

memem359 mentioned this pull request Jul 8, 2017

Logger system is currently broken freeorion/freeorion#1575

Closed

geoffthemedio added the enhancement label Sep 2, 2017

Changing CACHE variable requires FORCE

2ba6304

memem359 force-pushed the mac64-visibility branch from 2a8e021 to 278744b Compare September 3, 2017 21:49

Switch Mac Boost libs from static to shared

341cfbf

memem359 force-pushed the mac64-visibility branch from 278744b to 341cfbf Compare September 3, 2017 22:28

memem359 mentioned this pull request Oct 8, 2017

Switch Boost library version? #35

Closed

adrianbroher added a commit that referenced this pull request Nov 5, 2017

Merge pull request #29 from memem359/mac64-visibility

b9cfa10

adrianbroher closed this Nov 5, 2017

adrianbroher mentioned this pull request Nov 5, 2017

Add support for 64bit. #21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mac SDK change from i386 to x86_64, compile with visibility=hidden #29

Mac SDK change from i386 to x86_64, compile with visibility=hidden #29

memem359 commented May 25, 2017 •

edited

Loading

adrianbroher commented May 26, 2017

memem359 commented May 26, 2017 •

edited

Loading

memem359 commented Jun 9, 2017 •

edited

Loading

adrianbroher Jun 13, 2017

memem359 Jun 13, 2017

Dilvish-fo Sep 2, 2017

adrianbroher Sep 2, 2017

memem359 Sep 2, 2017

memem359 Sep 3, 2017

adrianbroher Jun 13, 2017

memem359 Jun 13, 2017

adrianbroher Jun 13, 2017

memem359 Jun 13, 2017 •

edited

Loading

memem359 Sep 2, 2017 •

edited

Loading

memem359 Sep 3, 2017

LGM-Doyle Oct 8, 2017

Vezzra Oct 12, 2017

memem359 Oct 29, 2017

Vezzra Oct 29, 2017

memem359 Oct 29, 2017 •

edited

Loading

Vezzra Nov 2, 2017

memem359 commented Sep 2, 2017

adrianbroher commented Nov 5, 2017

Mac SDK change from i386 to x86_64, compile with visibility=hidden #29

Mac SDK change from i386 to x86_64, compile with visibility=hidden #29

Conversation

memem359 commented May 25, 2017 • edited Loading

adrianbroher commented May 26, 2017

memem359 commented May 26, 2017 • edited Loading

memem359 commented Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

memem359 Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

memem359 Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

memem359 Oct 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

memem359 commented Sep 2, 2017

adrianbroher commented Nov 5, 2017

memem359 commented May 25, 2017 •

edited

Loading

memem359 commented May 26, 2017 •

edited

Loading

memem359 commented Jun 9, 2017 •

edited

Loading

memem359 Jun 13, 2017 •

edited

Loading

memem359 Sep 2, 2017 •

edited

Loading

memem359 Oct 29, 2017 •

edited

Loading