Skip to content
This repository has been archived by the owner on Mar 12, 2019. It is now read-only.

ENV: Add formula.lib to RPATH for unlinked kegs #449

Merged
merged 1 commit into from
Aug 6, 2017
Merged

ENV: Add formula.lib to RPATH for unlinked kegs #449

merged 1 commit into from
Aug 6, 2017

Conversation

sjackman
Copy link
Member

@sjackman sjackman commented Aug 4, 2017

Unlinked kegs, such as versioned kegs, need their lib directory in their RPATH.

See openssl@1.1 as an example.

Copy link
Contributor

@rwhogg rwhogg left a comment

Choose a reason for hiding this comment

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

Seems like it definitely fixes the problem, both for superenv and stdenv (tested that with mozjpeg). Thanks a bunch Shaun 👍

Left some points for clarification.

end

# Set the search path for header files.
prepend_path "CPATH", HOMEBREW_PREFIX/"include"
# Set the dynamic linker and library search path.
append "LDFLAGS", "-Wl,--dynamic-linker=#{HOMEBREW_PREFIX}/lib/ld.so -Wl,-rpath,#{HOMEBREW_PREFIX}/lib"
Copy link
Contributor

Choose a reason for hiding this comment

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

According to https://github.com/numpy/numpy/pull/8110/files the use of a comma after -rpath is a Mac-specific thing, but this is inside an if OS.linux? block.

I'm confused; it doesn't look like every stdenv-built executable had a bad RPATH as a result, but I find it hard to believe that we didn't notice this until now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Commented below.

@@ -311,6 +311,10 @@ class Cmd
paths.map! { |path| prefix + path }
end

def rpath_flags(prefix, paths)
paths.uniq.map { |path| prefix + path }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not remove non-directories from the list like path_flags does?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question! A comment is justified here. The problem is that the formula.lib directory does not yet exist when the formula is being compiled and this function is called, so formula.lib gets pruned from the list of directories.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that non-exsting lib directories of dependencies are already pruned from this list earlier: https://github.com/Linuxbrew/brew/blob/master/Library/Homebrew/extend/ENV/super.rb#L193

Pruning again here was a bit redundant.

prepend_path "LD_RUN_PATH", HOMEBREW_PREFIX/"lib"
unless formula.nil?
prepend "LDFLAGS", "-Wl,-rpath=#{formula.lib}"
Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, the issue affects only keg-only formulae (because their libraries aren't symlinked into $HOMEBREW_PREFIX/lib), but we're adding the lib directory for all formulae, right?

I can think of one possible consequence of that, which is that if you have two conflicting formulae that both install a shared library and you force one of them with brew link --overwrite, the library from this formula will still be preferred even if it's overwritten. I'm not sure if that's a good thing or a bad thing; I'm leaning towards good thing, but some users might find it surprising.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, and yes that's a change in behaviour, but I agree a good thing.

@sjackman
Copy link
Member Author

sjackman commented Aug 6, 2017

Comma after -rpath in cc -Wl,-rpath,FOO is valid on both macOS and Linux. The C compiler front-end translates the commas to spaces before calling the linker, and it becomes -rpath FOO. With an equal sign cc -Wl,-rpath=FOO becomes ld -rpath=FOO. Both forms are valid. I have an aesthetic preference for the equal sign, though I think the comma is probably more common.

No bottles until now had formula.lib in the RPATH, but they did all have HOMEBREW_PREFIX/lib in their RPATH, so as long as the keg was linked, it worked just fine.

This fixes one other small issue. If you had multiple versions of curl kegs installed, and you run an specific older version bin/curl that wasn't linked, it would use the old executable, but the libcurl.so of the newest linked keg.

Unlinked kegs, such as versioned kegs, need their lib directory in their RPATH.
Copy link
Contributor

@rwhogg rwhogg left a comment

Choose a reason for hiding this comment

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

Thanks for the explanations Shaun. :shipit:

@sjackman sjackman merged commit f99078a into Linuxbrew:master Aug 6, 2017
@sjackman sjackman deleted the rpath branch August 6, 2017 20:26
@sjackman
Copy link
Member Author

sjackman commented Aug 6, 2017

Merged!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants