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

Add superenv for Linuxbrew#263

Merged
sjackman merged 9 commits into
Linuxbrew:masterfrom
sjackman:superenv
Feb 22, 2017
Merged

Add superenv for Linuxbrew#263
sjackman merged 9 commits into
Linuxbrew:masterfrom
sjackman:superenv

Conversation

@sjackman

Copy link
Copy Markdown
Member

No description provided.

@sjackman sjackman changed the title Add superenv for Linuxbrew Add superenv for Linuxbrew [WIP] Feb 13, 2017
@sjackman sjackman self-assigned this Feb 13, 2017
@sjackman

Copy link
Copy Markdown
Member Author

@Linuxbrew/core As the title says. In this version, I've copied Library/Homebrew/shims/linux/super/cc from the Mac version and modified it just enough to get it up and running. It'll require much more work to bring it up to pace with --env=std in Linuxbrew.

@sjackman

sjackman commented Feb 13, 2017

Copy link
Copy Markdown
Member Author

My last commit makes --env=std the default on Linuxbrew, so that --env=super is opt-in. With that change, we should be able to merge this PR now, and work on improving superenv together.

Comment thread Library/Homebrew/extend/ENV.rb Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why remove inherit?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The behaviour hasn't changed. --env=inherit still works as before.

@sjackman sjackman requested a review from bw-yuhogg February 15, 2017 21:10
@sjackman sjackman changed the title Add superenv for Linuxbrew [WIP] Add superenv for Linuxbrew Feb 15, 2017
@sjackman

Copy link
Copy Markdown
Member Author

Succeeded on Linux. Unrelated (I think) error on Mac:

Error:
Satisfy Dependencies and Requirements::depends_on macos#test_0001_understands depends_on macos: <array>:
Hbc::CaskError: It seems there is already an App at '/tmp/homebrew-tests-20170215-10426-1fzztn2/prefix/appdir-1487198972-634/Caffeine.app'.

https://travis-ci.org/Linuxbrew/brew/jobs/202017165#L469

Comment thread Library/Homebrew/shims/linux/super/cc Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it make sense to skip this if rpath_paths is empty?

@sjackman sjackman Feb 16, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes. It shouldn't happen I think, but better to be paranoid. Thanks for catching that case. Fixed.

@bw-yuhogg bw-yuhogg Feb 16, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be restricted to runtime dependencies?

For shellinabox I got:

patchelf --print-rpath ~/.linuxbrew/bin/shellinaboxd 
/home/linuxbrew/.linuxbrew/lib:/home/linuxbrew/.linuxbrew/opt/libtool/lib:/home/linuxbrew/.linuxbrew/opt/zlib/lib:/home/linuxbrew/.linuxbrew/opt/openssl/lib

zlib and openssl make sense, but I don't think libtool should be there.

Putting some debugging code in revealed autoconf and automake, both build deps, were also considered, but they didn't end up in the RPATH presumably because they don't have lib directories.

@sjackman sjackman Feb 16, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍 Nice catch. Fixed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I haven't yet fixed this for the rpath PR for --env=std. There's shared code, so I think I'll merge this PR first, and then rebase the rpath code on top of this merged PR.

Add the opt_lib directory of each dependency to the RPATH after
$HOMEBREW_PREFIX/lib, which is searched first, allowing dependent
libraries to be found even when the keg is unlinked.
Comment thread Library/Homebrew/build.rb
ENV.keg_only_deps = keg_only_deps
ENV.deps = formula_deps
ENV.run_time_deps = run_time_deps
ENV.x11 = reqs.any? { |rq| rq.is_a?(X11Requirement) }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tangent: Does this need to be changed to also support XorgRequirement?

Note that when we port that to Homebrew/brew, I think we'll probably drop that name and instead use the extension mechanism Mike made, which is going to require some rather substantive retesting on our end come merge time.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's used to point to paths to in the XQuartz SDK, which shouldn't be necessary for xorg because it's in the brew prefix.

Using -rpath=/a -rpath=/a:/b results in /a:/a:/b whereas
using -rpath=/a -rpath=/a -rpath=/b results in /a:/b
exit 1
fi
exec "$HOMEBREW_RUBY_PATH" -x "$0" "$@"
#!/usr/bin/env ruby -W0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting approach... :)

@sjackman sjackman merged commit acd0ff3 into Linuxbrew:master Feb 22, 2017
@sjackman sjackman deleted the superenv branch February 22, 2017 23:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants