Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tickets/DM-5753: XCode 7.3 is broken #14

Merged
merged 2 commits into from Apr 11, 2016
Merged

tickets/DM-5753: XCode 7.3 is broken #14

merged 2 commits into from Apr 11, 2016

Conversation

timj
Copy link
Member

@timj timj commented Apr 8, 2016

Provides a workaround on OSX for the bug linking indirect libraries on OS X. Since I was messing with that code there is a second commit that cleans up the -install_name option and also switches that to use @rpath (which seems to be the recommended thing to do on OS X).

@@ -172,14 +172,20 @@ def _initEnvironment():
if env['PLATFORM'] == 'darwin':
env['LDMODULESUFFIX'] = ".so"
if not re.search(r"-install_name", str(env['SHLINKFLAGS'])):
env.Append(SHLINKFLAGS=["-Wl,-install_name", "-Wl,${TARGET.file}"])
env.Append(SHLINKFLAGS=["-install_name", "@rpath/${TARGET.file}"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not obvious if @rpath/libfoo.dylib is the right solution here. I think this implies we're putting all our libraries into some joint lib (or that we'll be adding a lot of RPATHs into the executables, for each per-product lib)?

It don't think it matters with the current DYLD_... EUPS scheme. But will make it impossible for conda-build to infer the proper install_name from $PREFIX, which may make it more difficult to do an EUPS-less install.

I'd propose to leave it as-is until we have a firmer grasp about what's the right thing to do (probably as a part of developing an EUPS-less install?).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you say, adding @rpath is a no-op when you set DYLD_LIBRARY_PATH as the linker always searches that path first regardless of any path burned into the library. I don't quite understand what you are saying about proper $PREFIX because we don't currently set any prefix at all in the install name and furthermore conda-build explicitly goes in and changes all our library install names to use @rpath. From reading the notes about @rpath I was under the impression that adding @rpath was the only way to allow people to reliably use your libraries if they aren't setting DYLD_LIBRARY_PATH because they can set the search path in the binaries explicitly and indirect dependent libraries will know to also use that search path. Most of the packages outside of EUPS do already do what this patch does (see e.g. boost).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS I'm not going to fight this. I'm okay with adjusting the patch so it just cleans up the -Wl stuff. I'll see what @jdswinbank says when he reviews it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never used conda-build, and I'm not sure I understand the point that @mjuric is raising here -- my beginner-level grasp of how this is supposed to work agrees with @timj.

However: this doesn't seem like it's directly related to the issue at hand, it doesn't solve a problem we're having this week, and if there's a risk it could interfere with the Anaconda build, my suggestion is to drop it.

Always add the trailing slash even in older XCode versions. Can
remove this change when Apple fix XCode.
There is no need to use -Wl, to pass "-install_name" to the linker.
Clang on OS X already knows how to do this so the command line looks
cleaner without it.
@timj timj merged commit c996057 into master Apr 11, 2016
@mjuric
Copy link
Member

mjuric commented Apr 12, 2016

@timj, @jdswinbank: Sorry I wasn't very clear in my comment on @rpaths -- here's what I meant:

With conda build we build our code into $CONDA_ENV/opt/<packagename>/<version>/, with the libraries usually ending up in subdirectory lib or python/lsst/foo/.... Those libraries are frequently linked against libraries in other packages. E.g., daf_persistence uses boost serialization. For daf_persistence to link against libboost_serialization.dylib at run time, it needs to where it will find it. It does that by simply copying the value of the libboost_serialization's install_name attribute (at build time), storing it into its own header.

The issue is that if you set the install_name to @rpath/libfoo.dylib in all libraries, that means that you need now to make sure that the right RPATHs are added to every library that's linking against them. I.e., for daf_persistence, assuming the above directory structure, you'd need to add RPATH of @loader_path/../../../boost/1.60/lib for boost, then something like @loader_path/../../../cfitsio/3233/lib for cfitsio, etc, and do that for every single library it depends on. That's now a logistical pain (and defeats the point of having @rpath in the first place). It's also not automatic -- while the copying of install_name for dependent libraries is done for you by ld when you link against them.

It's better to instead agree upon a fixed point in the filesystem, only store the path to that point as the RPATH in your binaries, and encode install_names relative to that rpath. That's what conda does. It declares that $CONDA_ENV/lib is what each binary should have set as its rpath, and that each library's install_name should be relative to this point in the filesystem. Its packaging tools then scan every generated binary and library to a) add that RPATH (for executable and libraries) and b) make sure their install_name is relative to it (for libraries). So if you inspect (with otool -l) an executable created with conda-build and installed in, say, $CONDA_ENV/opt/foo/bin, you will find that it has an LC_RPATH entry equal to @loader_path/../../../lib/. Similarly, if you inspect (with otool -D) a library named libbar.dylib that's in $CONDA_ENV/opt/bar/lib, you'll find that it's install_name is @rpath/../opt/bar/lib/libbar.dylib.

The key bit is that conda-build will do these path adjustments for you as an afterburner after the normal build process, but only if the install_name it finds in the built libraries is a) absolute and matching the prefix where the build was occurring or b) just the filename. So if we set it to @rpath/filename, we effectively disable this (quite useful) mechanism.

Right now this is all a moot point, as DYLD_LIBRARY_PATH takes precedence over any RPATH that is encoded in the binaries. But if you unset that, you actually should be able to run our code on OS X under Conda (which should actually come in handy with all the DYLD_* mess on El Capitan). And, coincidentally, I think the only reason this doesn't work right now is because boost itself sets the install_name to @rpath/...., thus ruining this clever scheme :).

Anyways, to make a long story short, we will want to change the install_name that sconsUtils sets to something more sensible at some point (and it may depend on what we decide on how to lay things out on the filesystem -- merge the directory trees of all products? keep them apart?), but until we do we could leave it as is and let conda-build do its thing.

@jdswinbank
Copy link
Contributor

Thanks, @mjuric, for taking the time to explain that.

@ktlim ktlim deleted the tickets/DM-5753 branch August 25, 2018 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants