Page MenuHomeWildfire Games

Add a build flag to prefer locally-built libraries.
ClosedPublic

Authored by s0600204 on Jan 14 2019, 2:37 AM.

Details

Reviewers
Itms
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22308: Add a build flag to prefer locally-built libraries (linux/bsd systems)
Summary

As mentioned in https://wildfiregames.com/forum/index.php?/topic/24985-installation-on-centos-7, there are times when a user may have two versions of a library installed - one installed from the OS package repository, and one that was installed manually.

To summarize:

  • A user has CentOS 7, the latest stable release of the CentOS linux distribution (at the time of writing this), and wishes to compile pyrogenesis.
  • One of the requirements of pyrogenesis is boost, with a minimum version of v1.57.
  • Unfortunately, the most recent version of boost available from the CentOS (and EPEL) repos is v1.53.
  • The recommended action is to find and use a more recent version of boost. The easiest way is to compile it oneself.
  • However, there are various other applications of CentOS that also use boost, and are compiled against the repo-supplied version. Thus, replacing the repo-supplied-version with a newer version is not recommended.
  • Thanks to the Filesystem Hierarchy Standard, there is a way round this:
    • repo-provided libraries are installed under /usr/lib
    • locally-generated libraries are installed under /usr/local/lib
  • The user compiles and installs this more-up-to-date version of boost under /usr/local/lib
  • The user then attempts to compile pyrogenesis...

...and this is where we hit a snag.

When it comes to linking in libraries, premake (via the generated {component}.make files) informs the compile-time linker (ld [1]) what libraries are needed, and the linker then attempts to find and link them.

ld (by default) searches for these libraries in a set of "standard directories", and searches them in a certain order. This is to be expected: the compiler (gcc) does something similar when looking for the correct headers to include. The standard directories that both gcc and ld search through are even similar. The problem arises in the order that the locations are searched. The default search order for ld is as follows [2]:

  1. /usr/lib
  2. /usr/local/lib

To put it another way, ld searches amongst the repo-provided libraries before considering any locally-generated ones. Thus, when asked to link in the boost libraries, ld finds the version of boost installed from the repo first and links that, instead of the more up-to-date version generated locally.

The solution is to tell the linker to look in /usr/local/lib first.

As this doesn't just affect boost, but potentially other locally-generated libraries as well, this revision adds a new argument to update-workspaces.sh: --prefer-local-libs that tells premake to add -L/usr/local/lib to the flags passed to the linker.


It should be noted that this only resolves compile-time linking. When pyrogenesis is run after being tied to a library located within /usr/local/lib, then the run-time linker (ld.so) will complain that it can't find the dependency.

There are two ways around this:

  1. Use -rpath instead of -L. This will possibly bring the ire of Fedora packagers down on us[3], and we've already had a ticket (#1421) trying to remove -rpath from our build system. (We use it to link the bundled spidermonkey.)
  2. Tell the user to create a suitable file in /etc/ld.so.conf.d/

Test Plan
  • Have a linux system with two versions of a library installed: one in /usr/lib and the other in /usr/local/lib.
  • Run ./update-workspaces.sh both with and without the new argument. For each case:
    • Compile pyrogenesis.
    • Run ldd ./pyrogenesis on the resultant executable. This will give a list of dependencies, one of the lines of which will indicate the exact library (version and path) used.
  • Verify that with the argument, the library used is the version found in /usr/local/lib; and without the argument, the library used is the version located in /usr/lib.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
local_libs
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7568
Build 12335: Vulcan BuildJenkins
Build 12334: arc lint + arc unit

Event Timeline

s0600204 created this revision.Jan 14 2019, 2:37 AM
Owners added a subscriber: Restricted Owners Package.Jan 14 2019, 2:37 AM
Vulcan added a subscriber: Vulcan.Jan 14 2019, 3:04 AM

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

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

Maybe this could be used for brew @wraitii

This would be a workable approach on OSX but not to actually build a bundle - that's the snag I've hit in the past and I'm trying to fix with D1483.
The problem is OSX always links dynamically if it can, which leads to bundles that either:

  • Have a huge internal Frameworks folder - which increases download times and still requires custom code on our side to support
  • Can't be started unless the target computer has also locally-installed compatible libraries (in the same places).

To link statically on OSX, it needs to see only static libraries. This is what happens when we manually compile the libraries.

If we don't care about that (which we don't for development, arguably), we could use a similar trick and I think it'd work mostly well. However, we still need a compilation script for OSX for the few libs that can't be brought by brew for incompatibility or other reasons. Supporting both is a little annoying, hence the efforts to combine that in D1483 (which is only the start).

I'm not super-qualified to comment on the revision proper here but this looks reasonable.

Itms accepted this revision as: Restricted Owners Package.May 21 2019, 8:34 PM
Itms added a subscriber: Itms.

With three small improvements/cleanups, this looks good to me. You can fix them and then commit. ?

Maybe we could ask the premake devs to add a more specific command that allows one to use local libs on Linux and BSD. Probably difficult to get right, but they might be interested... https://github.com/premake/premake-core/issues

build/premake/premake5.lua
18

Maybe write Linux/BSD since the code checks for that.

19

I agree that we shouldn't use rpath, so maybe we could add in the description that users/maintainers who use the option should also create/distribute a ld.so.conf file?

build/workspaces/update-workspaces.sh
54 ↗(On Diff #7342)

This is unnecessary, the option will be caught by line 56.

Itms accepted this revision.May 21 2019, 8:37 PM
This revision is now accepted and ready to land.May 21 2019, 8:37 PM
s0600204 updated this revision to Diff 8106.May 24 2019, 7:03 PM
s0600204 marked 3 inline comments as done.

Update in response to @Itms's comments.

As this has been accepted, I'll give it a couple of days (to allow for further remarks), then commit.

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1468/display/redirect

This revision was automatically updated to reflect the committed changes.