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

link: don't allow more openssl/libressl linkage. #612

Merged
merged 1 commit into from
Jul 31, 2016
Merged

link: don't allow more openssl/libressl linkage. #612

merged 1 commit into from
Jul 31, 2016

Conversation

MikeMcQuaid
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you successfully run brew tests with your changes locally?

This extends the approach in #597 to further prevent linkage of formulae that conflict with the system OpenSSL and can cause the issues described in that issue. CC @DomT4 for thoughts.

@BrewTestBot BrewTestBot added the in progress Maintainers are working on this label Jul 31, 2016
EOS
next
if HOMEBREW_PREFIX.to_s == "/usr/local"
if keg.name.start_with?("openssl") || keg.name.start_with?("libressl")
Copy link
Contributor

Choose a reason for hiding this comment

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

This nested if will mess up the logic in the following elsif keg.linked? and what follows, i.e. you'll have to keep the condition in one piece. Note you can use multiple arguments to express “starts with any of?”, so this might be short enough to stay on a single line:

if HOMEBREW_PREFIX.to_s == "/usr/local" && keg.name.start_with?("openssl", "libressl")

(My personal belief is that the prefix check could be completely dropped so that users get a more uniform behavior independently of where they chose to install Homebrew, though I do understand that there isn't any issue with non-/usr/local prefixes. Just a thought—I'm happy either way.)

Copy link
Member

Choose a reason for hiding this comment

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

(My personal belief is that the prefix check could be completely dropped so that users get a more uniform behavior independently of where they chose to install Homebrew, though I do understand that there isn't any issue with non-/usr/local prefixes. Just a thought—I'm happy either way.)

I'd prefer we kept exempting non-/usr/local prefixes since there's not actually a problem there. I'd argue openssl shouldn't even really be keg_only outside of /usr/local, but there we go.

👍 on the rest of Martin's comment though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer we kept exempting non-/usr/local prefixes since there's not actually a problem there. I'd argue openssl shouldn't even really be keg_only outside of /usr/local, but there we go.

Maybe to provide a rationale for my thinking: Since those users have to supply custom paths to their compiler and linker invocations anyway, they could as well supply the PREFIX/opt/openssl/{include,lib} paths. The upside would be that their scripts/etc. will continue to work even if they decide to switch from a non-/usr/local to a /usr/local installation or share their work with other Homebrew users.

But as I said, it's just a thought. I'm fine with limiting this to /usr/local installations.

Copy link
Member

Choose a reason for hiding this comment

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

Part of me wants to get Travis building in /opt/brew as well so we know how much breaks in that directory. I'd generally like us to stop vilifying non-/usr/local installations & think it'd be reasonably easy to support, as well as opening up the option of moving in future if Apple clamps down even more or we decide it's worthwhile.

But that's not strictly related to this discussion, so I'll leave that be.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd generally like us to stop vilifying non-/usr/local installations & think it'd be reasonably easy to support, as well as opening up the option of moving in future if Apple clamps down even more or we decide it's worthwhile.

My main issue is bottles. They massively improve the user experience and reduce support burden for us.

Copy link
Member

Choose a reason for hiding this comment

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

We're making progress on that front though.

2356 formulae currently have some form of cellar :any, another 451 are bottle :unneeded, another 5 are bottle :disable. So that leaves... 783 non-portable bottles. Rough numbers, obviously, but we're making solid progress here.

We'll get a lot more wrapped up once we work out how to relocate NLS stuff as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we got everything cellar :any my objection goes away. There's things like git that'll require upstream changes, though. The best middle ground I've heard is leaving the cellar as-is but just moving the default prefix/repo checkout to /usr/local/homebrew, /opt/homebrew or whatever using @chdiza's method.

@MikeMcQuaid
Copy link
Member Author

Addressed feedback.

This extends the approach in #597 to further prevent linkage of formulae
that conflict with the system OpenSSL and can cause the issues
described in that issue.
@UniqMartin
Copy link
Contributor

👍

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

Successfully merging this pull request may close these issues.

None yet

4 participants