FreeOrion

Forums for the FreeOrion project
It is currently Sun Nov 19, 2017 3:21 am

All times are UTC




Post new topic Reply to topic  [ 14 posts ] 
Author Message
PostPosted: Thu Jun 01, 2017 11:56 am 
Offline
Dyson Forest
User avatar

Joined: Sun Jun 08, 2014 1:18 am
Posts: 214
I've reached an impasse for the Mac build. I need to hear what steps the FO group want to take next.
The FO code is compiled with "-fvisibility=hidden", which really means that the SDK libraries should also be built that way too. (In for a penny, in for a pound.) I've been working on that.

I've come across a case that I think can only be resolved by either turning off the "-fvisibility=hidden" flag when compiling the code, or by hacking one of the Boost files with a patch. adrianbroher has stated strongly that user patches are not an option (no "local garbage"), but I don't see another solution that will retain hidden visibility.

I need feedback.
Do we drop visibility=hidden, am I allowed an exception to use a home-grown SDK patch for this one specific issue, or is there another solution that I'm not seeing?

--The Details--
Looking through the online discussions, Boost has visibility issues which are being resolved, but most of that work is on the Windows side. The "iostreams" library does not have a solution for non-Windows builds.

For the Mac, with the libraries and FO code using visibility=hidden, the code cannot find "boost::iostreams::zlib::default_compression". This is defined in iostreams/filter/zlib.hpp as
Code:
BOOST_IOSTREAMS_DECL extern const int default_compression;

The only place that BOOST_IOSTREAMS_DECL is defined is iostreams/detail/config/dyn_link.hpp
That file sets the macro to either Windows-specific settings (declspec(dllexport)), or leaves it blank.
(That file is the same in both versions 1.59 and the recent 1.64.)
So the Boost iostreams library will have the wrong visibility settings.

This is unlike other places in the Boost code, where it is possible to get the right visibility settings by defining the library DECL macro as BOOST_SYMBOL_EXPORT. (Take a look at filesystem/config.hpp as an example of how it is done for most of the other libraries.)


Last edited by mem359 on Thu Jun 01, 2017 2:05 pm, edited 1 time in total.

Top
 Profile  
 
PostPosted: Thu Jun 01, 2017 12:34 pm 
Offline
Programming, Design, Admin
User avatar

Joined: Wed Oct 08, 2003 1:33 am
Posts: 12013
Location: Munich
mem359 wrote:
Do we drop visibility=hidden ...
What consequence would that have?


Top
 Profile  
 
PostPosted: Thu Jun 01, 2017 2:02 pm 
Offline
Dyson Forest
User avatar

Joined: Sun Jun 08, 2014 1:18 am
Posts: 214
Geoff the Medio wrote:
mem359 wrote:
Do we drop visibility=hidden ...
What consequence would that have?

The use of visibility=hidden in the main FO code was introduced before my time. :mrgreen:

We have FO_COMMON_API and FO_PARSE_API sprinkled throughout the code specifically to handle "hidden".
I didn't want to make a fairly big change in compiling the code without having a discussion first.

I understand that it is good practice (using hidden), to make sure variables aren't getting confused by accident between (unrelated) libraries/code, but maybe that is overkill for FO ? :? That is the risk in turning it off: variables/functions unintentionally being called or changed.

If you mean, leave things as they are (SDK libraries with default visibility, FO with hidden), then we have hundreds of warnings in the Travis check, due to code being built with different visibility settings. Not lethal, but easy to miss a real problem when it is buried in dross. And it weakens the reason to use visibility=hidden in the first place.

That's how I see it.
But I've only been working on the code for a few months, so I might be missing a lot.


Edit: I should have mentioned that this started out as an investigation for why the Logger system doesn't work correctly for the Mac build. Clues appear to point to issues with Boost Log visibility, static vs. shared libraries, etc.


Top
 Profile  
 
PostPosted: Thu Jun 01, 2017 2:43 pm 
Offline
Programmer

Joined: Mon Feb 29, 2016 8:37 pm
Posts: 198
visibilty=hidden is the same as the Windows equivalent __declspec(dllexport). It means that all of the symbols in a shared library that are not marked as for export are not included in the link table. This makes the shared library smaller and hence linking and loading are faster.

Removing visibility=hidden on OSX will make the boost and freeorion libraries larger, and compile, link and load times take longer. This would only be a change that affects OSX.

mem359,

Does linking dynamically (instead of statically) without visibility=hidden fix all of the linker errors?

Have you tried the change to the iostreams library? Does it also result in a clean build?

What is the difference in shared library size, boost and fo?

What is the difference in build times?

If we have answers to those questions, then we will know if we care or not, about whether the OSX SDK is compiled with visibility=hidden.


Top
 Profile  
 
PostPosted: Thu Jun 01, 2017 3:30 pm 
Offline
Dyson Forest
User avatar

Joined: Sun Jun 08, 2014 1:18 am
Posts: 214
LGM-Doyle wrote:
Does linking dynamically (instead of statically) without visibility=hidden fix all of the linker errors?

Have you tried the change to the iostreams library? Does it also result in a clean build?

What is the difference in shared library size, boost and fo?

What is the difference in build times?

If we have answers to those questions, then we will know if we care or not, about whether the OSX SDK is compiled with visibility=hidden.

I also wanted a discussion about the SDK patch policy.
I understand the reason why the answer is "no" by default, but that assumes that Boost is intended to operate the way it is. Looking at dyn_link.hpp, that is clearly a Windows-only solution (for symbol visibility) that was never extended to other platforms. If there was ever a reason to have "local garbage", this would be it.

----
For your other questions, I'm still weeding out the (missing symbol) link errors.

I haven't been able to find documentation about building Boost libraries with hidden visibility, so it has been digging into the code, trial-and-error (plus SDK-compile plus FO-compile). And CMake (for the SDK) was brand-new to me, so that was another learning curve.

I think I'll have an error-free Mac compile by the weekend (with shared libraries and hidden visibility and 1 SDK patch).
After that, some test games to see if anything broke.
If that works, then I will do app-size and compile-time comparisons.

This solution will be wasted effort, if the group decides to go on a different path.
This discussion might take several days, so I wanted to get it started, while working on my solution in parallel.
I figured more experienced contributors would know which FO choices are critical (versus just being convenient).


Top
 Profile  
 
PostPosted: Thu Jun 01, 2017 4:41 pm 
Offline
Programmer

Joined: Mon Feb 29, 2016 8:37 pm
Posts: 198
Don't feel any pressure. There is no rush.

When you have a fix, you could provide a patch to boost iostreams. That would make a good argument for including cruft with a clear note explaining when the cruft can be removed.

The problem with "local garbage" is that it is rarely clearly documented as to why it was done, when it should stop being done and what changes upstream will break it. Like the decision to statically link on OSX. Was it required, a bug fix, a performance decision? And now statically linking is quietly, almost silently causing linker errors.

I don't think your efforts have been wasted. The OSX SDK is already improved.

If you want to isolate the dynamic vs static linking problems from the hidden vs default visibility problems, you could use FREEORION_MACOSX to define FO_COMMON_API and FO_PARSE_API as blank for OSX.


Top
 Profile  
 
PostPosted: Thu Jun 01, 2017 8:48 pm 
Offline
Dyson Forest
User avatar

Joined: Sun Jun 08, 2014 1:18 am
Posts: 214
I'm down to 5 linker errors, so I'm no longer drowning in red.

Turns out that the Boost Locale library also has a Windows-specific setting, which wasn't obvious at first glance.

In locale/definitions.hpp, it is required that BOOST_HAS_DECLSPEC is defined before doing the visibility settings.
This is only true for win32 (platform) and gcc (compiler).
Since I'm using macosx/clang, this is another file that would need modification (and another Boost bug ticket).


Top
 Profile  
 
PostPosted: Fri Jun 02, 2017 9:01 pm 
Offline
Programmer

Joined: Mon Feb 29, 2016 8:37 pm
Posts: 198
That is 224 fewer warnings, from the last time I checked. Awesome. 224 X :D


Top
 Profile  
 
PostPosted: Sat Jun 03, 2017 1:07 pm 
Offline
Dyson Forest
User avatar

Joined: Sun Jun 08, 2014 1:18 am
Posts: 214
LGM-Doyle wrote:
That is 224 fewer warnings, from the last time I checked. Awesome. 224 X :D

Well, there are still the "semantic issue" warnings (mostly suggesting to use more parentheses to make the order of operations clearer).

The next roadblock are the Boost shared libraries. (The Boost guide says that Log will need be shared instead of static.) The FO build succeeds (no errors!), but the runtime linking is no where close to working. I was hoping to have a magic Boost compile setting that would use the dylib install_name that the Mac system wants, but Boost support is focused on Windows builds. I'll keep plugging at it.


Top
 Profile  
 
PostPosted: Mon Jun 05, 2017 3:24 am 
Offline
Dyson Forest
User avatar

Joined: Sun Jun 08, 2014 1:18 am
Posts: 214
There was some useful information on the 3-year old thread CMake and the Windows FO SDK. For the Xcode build, I want to have the Common dylib target link to the Parse dylib, and to remove the flag "undefined dynamic_lookup". Turns out that was in the plans way back then.

That flag was hiding an undefined symbol that was causing my run-time link failure. After beating my head against the wall the entire weekend trying to resolve it, one last web search finally showed what I needed. The Boost log_setup library had a Mac/Clang bug in 1.59 (which we are using) which was fixed in 1.60.
https://github.com/boostorg/log/pull/10
I will apply that as a patch to the SDK (on top of my fixes for two other files that have settings that only work for MSVC).


Top
 Profile  
 
PostPosted: Mon Jun 05, 2017 3:27 am 
Offline
AI Lead, Programmer
User avatar

Joined: Sat Sep 22, 2012 6:25 pm
Posts: 4374
Bravo!

_________________
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0


Top
 Profile  
 
PostPosted: Mon Jun 05, 2017 9:47 am 
Offline
Programmer
User avatar

Joined: Fri Mar 01, 2013 9:52 am
Posts: 1040
Location: Germany
mem359

just to make this clear: You're wasting your time here. None of your proposed changes solves an urgent problem or brings a significant improvement to the project or its workflow, they rather do the opposite or something completely orthogonal. Anything that prolongs the lifetime of the xcode or msvc project, everything that doesn't directly support the outright removal of those components in favour of cmake (or any other cross platform build system; IDGAF anymore and if it would make people happy: do a red bike shed instead of green one) I consider sabotage on the build system and my years long work and I will block it.

I was sick of waiting for changes happen to the SDK to make a unified building system possible, hence the FO-SDK build system in it's current form exists. Before that there was some manual undocumented something happening somewhere.

I was sick of the fact that every platform uses a completely different build setup (shared libs here, static libs there, but only sometimes under special circumstances, rarely documented), hence the current setup with all it's quirks und unintitive settings exist. Nothing of this is permanent or as I consider it should be done but before that it was way worse and made any build setup changes almost impossible.

I was sick of the fact that people changed the build setup and broke the build on antother platform, just leaving it to the users to find this out, hence the CI setup in it's current meager form exists and actually it should take over many more mundane tasks in the future to improve project quality.

I'm still sick of the fact that there are multiple files to change for simple project changes like adding a new source file, sometimes requiring the involvement of multiple developers due to the lack of platform specific tools availability (I mean WTF?). I don't want to pester Vezzra for mundane tasks like adding files to the project because I lack the ability to edit Xcode files.

I'm still sick of the fact that there is no way to add additional targets to allow supplementary tasks like style enforcement or automated tests to be executed in a unified way. to prevent issues before they hit the repository. I won't support multiple build systems just for somebody else convenience of not using CMake and hampering the overall project quality.

I'm still sick of the unnecessary complex build and release setup. Downloading code here and some SDK over there, but the mathing version please? Also keep the right in mind that your need toolchain X because otherwise screw you, we can't support this. Checking the capabilities of the compiler and platform setup? HA, neither Xcode nor MSVC are able to handle this.


Quote:
adrianbroher has stated strongly that user patches are not an option (no "local garbage")


My point was (and still is): "When I am the upstream developer, would I accept this patch to my code?". For the patch you suggested the obvious answer is "No" as it would break any other platform for boost. So I as FreeOrion-SDK maintainer would be forced to track this patch within the SDK repository and never get rid of it (local garbage). This is not true for the already existing patches as some of them are already accepted to the corresponding upstream project and part of the next version (for example GLEW or openal-soft), are just plain crutches to fix FreeOrion wrong usage of the corresponding project (for example the boost header install location) or backports to bring back fixes of future relases of a dependency (for example clang compiler burbs within boost.gil).

Problem being now that I won't unfreeze the current SDK setup until FreeOrion has a unified build setup because I won't do work three times (reconfigure and test msvc, xcode and cmake with perstering multiple devs) when I can go away with doing it once (cmake only) (PR #1455)(PR #1416).

_________________
Resident code gremlin
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz


Top
 Profile  
 
PostPosted: Mon Jun 05, 2017 12:46 pm 
Offline
Dyson Forest
User avatar

Joined: Sun Jun 08, 2014 1:18 am
Posts: 214
Marcel, please reconsider.
I think you jumped to some wrong assumptions. (Some are my fault, so understandable.)

adrian_broher wrote:
I was sick of...

When I am complaining about things, that was mostly griping about the learning curve for CMake, and not knowing for FO what was intended and what was temporary. The former is necessary, and the latter is not your fault. (As you say, you've been making it better than when you found it.)

If I sound unappreciative, let me say I am glad the CMake-SDK scripts exist.
Getting all that to work, just for Xcode, would be a nightmare.
Now that I'm farther down the learning curve, and finding more of the comments on this forum, I have a better (but not complete) idea of what to do for the SDK.

adrian_broher wrote:
None of your proposed changes solves an urgent problem or brings a significant improvement to the project or its workflow

Disagree.
It may only affect the Mac users, but there are still problems (that would not be obvious to Windows users).

First, the Logger system does not work for the Mac.
Tracking down bugs is a lot easier when it is working as intended.

Second, we want the visibility=hidden to reduce chances of code mistakes.
That should be done in the SDK also. If we don't, we have hundreds of warnings in the FO Travis check, which can hide real problems.

According to the Boost documentation, the Boost Log libraries should be shared, not static. I'm hoping that will fix the Logger situation for the Mac build. Maybe, maybe not, but I won't know until I get that working.

adrian_broher wrote:
Anything that prolongs the lifetime of the xcode or msvc project, everything that doesn't directly support the outright removal of those components in favour of cmake...

For the SDK, I am only using CMake.
I've mentioned that a few times already.
(Not even touching Xcode for the SDK build. That will be hopelessly left behind.)

Now that I've learned the basics of CMake, any proposed changes to the FO build will be for both Xcode and CMakeLists.txt. (I should have stated that earlier.)
I'm using XCode to get something that works. (Easier for me to work in that environment.)
Once I have an Xcode solution, I won't submit a PR until that is duplicated for CMake.

adrian_broher wrote:
I won't support multiple build systems just for somebody else convenience of not using CMake and hampering the overall project quality.

See last point.

adrian_broher wrote:
For the patch you suggested the obvious answer is "No" as it would break any other platform for boost.

Thank you for clarifying that.

Completely valid.
I was looking for a quick fix, since I didn't have enough Boost and CMake knowledge at the time.
Since I am proposing switching Boost from static to shared libs for the Mac (for Boost Log), those particular patches are no longer necessary.

I'm farther on the learning curve.
The "new" patches will be in a format that could be absorbed by the Boost repository. (I intend to submit them to Boost when I get caught up, or maybe I'll find an existing commit like I did for log/detail/setup_config.hpp.) The changes will only be for the (setup) files that have Windows-specific solutions only, to extend the same behavior for the Mac build.

adrian_broher wrote:
Problem being now that I won't unfreeze the current SDK setup until FreeOrion has a unified build setup because I won't do work three times (reconfigure and test msvc, xcode and cmake with perstering multiple devs) when I can go away with doing it once (cmake only) (PR #1455)(PR #1416).

Your comments are very useful.
I'm glad to hear all of your comments now, so I will make fewer mistakes for future PR changes.

My current SDK PR is dead in the water.
It is a placeholder, until I get everything working from beginning to end. (I will edit my comment to reflect that.)
Any changes I am working on now will only affect the Mac build.

(There is one thing I'd like to do for both Mac and Windows builds, but I will start a new forum topic and make a different PR.)


Top
 Profile  
 
PostPosted: Fri Jun 09, 2017 1:47 pm 
Offline
Dyson Forest
User avatar

Joined: Sun Jun 08, 2014 1:18 am
Posts: 214
I have a local build where the Logger is working correctly on my Mac.
I will be updated the SDK and FO PRs to have the least amount of modifications needed to get things working.

This is what I'm planning on doing for the Mac builds:
- Build Boost libraries as shared instead of static.
Necessary to get the Log code working.
http://boost-log.sourceforge.net/libs/l ... onfig.html
- Add patches to the SDK to modify 3 Boost include files (preprocessor macros)
All three are needed to handle visibility settings that were written only for Windows compilation. One was already merged into Boost 1.60 (FO uses 1.59). I have made PRs on the Boost repository for the other two, but it may be months before they are even looked at.
- SDK: Do not turn on visibility=hidden (leave it as it is)
The Boost set-up isn't designed to do that for the Mac/Clang build. To get the right visibility settings requires building for shared libraries, but a few of the libraries (like boost-exception) can only be built as static.
- SDK: Change the install_name path of the dylibs to be @executable_path/../SharedSupport
Needed for the Mac-specific way uses the install_name. This path is the same that is used for freeorioncommon.dylib and freeorionparse.dylib.
- Thirteen Boost preprocessor macros to be used for the main FO build
This will get the visibility=hidden settings right for the Boost include file definitions.


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 14 posts ] 

All times are UTC


Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
Powered by phpBB® Forum Software © phpBB Group