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: stop unneeded force linking on Mojave/CLT 10. #4441

Merged
merged 1 commit into from Jul 9, 2018

Conversation

Projects
None yet
5 participants
@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Jul 9, 2018

People are getting in the habit of force-linking things like zlib to fix linking/include issues on Mojave (which doesn't install headers to /usr/include by default). This way lies madness so encourage people to instead pass the correct compiler flags instead.

References Homebrew/homebrew-core#28423 (comment)

  • 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 written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

@wafflebot wafflebot bot added the in progress label Jul 9, 2018

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:fewer-mojave-force-links branch from 20815e6 to 897cd36 Jul 9, 2018

@MikeMcQuaid MikeMcQuaid changed the title ignlink: stop unneeded force linking on Xcode 10/Mojave. ignlink: stop unneeded force linking on Mojave/CLT 10. Jul 9, 2018

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:fewer-mojave-force-links branch from 897cd36 to 8b733a4 Jul 9, 2018

@MikeMcQuaid MikeMcQuaid requested a review from ilovezfs Jul 9, 2018

@MikeMcQuaid MikeMcQuaid changed the title ignlink: stop unneeded force linking on Mojave/CLT 10. link: stop unneeded force linking on Mojave/CLT 10. Jul 9, 2018

link: stop unneeded force linking on Mojave/CLT 10.
People are getting in the habit of force-linking things like `zlib` to
fix linking/include issues on Mojave (which doesn't install headers to
`/usr/include` by default). This way lies madness so encourage people to
instead pass the correct compiler flags instead.

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:fewer-mojave-force-links branch from 8b733a4 to 2f181b3 Jul 9, 2018

elsif (MacOS.version >= :mojave ||
MacOS::Xcode.version >= "10.0" ||
MacOS::CLT.version >= "10.0") &&
dep_f.keg_only_reason.reason == :provided_by_macos

This comment has been minimized.

@ilovezfs

ilovezfs Jul 9, 2018

Contributor

I think we also want to check shadowed_by_macos too since finding the libedit headers may be an issue too.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 9, 2018

Member

👍 but let's wait until we see that in the wild first?

@MikeMcQuaid MikeMcQuaid merged commit d445406 into Homebrew:master Jul 9, 2018

1 of 3 checks passed

codecov/patch 14.28% of diff hit (target 69.67%)
Details
codecov/project 69.66% (-0.02%) compared to b350fe0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Jul 9, 2018

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 9, 2018

Merging as-is for now but plan on adding :shadowed if we see it and perhaps non-Mojave with odeprecated in future 👍

@keyurp1987

This comment has been minimized.

Copy link

keyurp1987 commented on 2f181b3 Jul 10, 2018

Getting the below error which started happening after brew was updated to latest version with the changes above.

undefined local variable or method `dep_f' for Homebrew:Module

This comment has been minimized.

Copy link
Contributor

ilovezfs replied Jul 10, 2018

@keyurp1987 thanks for reporting this. It's now fixed. In the future, please open an issue and fill out the issue template to make sure your observations don't escape our notice.

This comment has been minimized.

Copy link

keyurp1987 replied Jul 10, 2018

@ilovezfs Thanks for the quick fix. Yes I will create the issue to get the attention in future.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jul 10, 2018

Apparently I can now link keg_only stuff without any kind of warning or needing to pass the --force flag? Not sure whether that's intentional but the documentation would still seem to indicate that it isn't.

~> brew link readline
Linking /usr/local/Cellar/readline/7.0.5... 16 symlinks created

~> ls -1 /usr/local/lib | grep readline
libreadline.7.0.dylib
libreadline.7.dylib
libreadline.a
libreadline.dylib
@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jul 10, 2018

That's a bug.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jul 10, 2018

Running brew link <xyz> on something that has already been linked no longer produces the correct behaviour either.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jul 10, 2018

yeah the if HOMEBREW_PREFIX.to_s == "/usr/local" && keg_only is catching more than intended

MikeMcQuaid added a commit to MikeMcQuaid/brew that referenced this pull request Jul 10, 2018

link: refactor, reorder and fix bugs.
The change in Homebrew#4441 broke the handling of the `elsif`s due to the
change in logic. As every block here has a `next` there's no need to do
an `elsif` in here at all. Additionally, reorder the conditions in here
so you get an appropriate message depending on what you're trying to do.
Finally, tweak some of the messaging to remove things that are ignored
and tell people correct commands to run to link things.

@MikeMcQuaid MikeMcQuaid referenced this pull request Jul 10, 2018

Merged

link: refactor, reorder and fix bugs. #4445

5 of 6 tasks complete

MikeMcQuaid added a commit to MikeMcQuaid/brew that referenced this pull request Jul 10, 2018

link: refactor, reorder and fix bugs.
The change in Homebrew#4441 broke the handling of the `elsif`s due to the
change in logic. As every block here has a `next` there's no need to do
an `elsif` in here at all. Additionally, reorder the conditions in here
so you get an appropriate message depending on what you're trying to do.
Finally, tweak some of the messaging to remove things that are ignored
and tell people correct commands to run to link things.
@billinghamj

This comment has been minimized.

Copy link

billinghamj commented Jul 25, 2018

So following this change, how are we meant to use things like curl after installing? Previously the only (simple) way to switch the built-in curl command to the new one was to use the link command. Is there another tool brew provides for this purpose?

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jul 25, 2018

how are we meant to use things like curl after installing?

If you need to have this software first in your PATH run:
  echo 'export PATH="/usr/local/opt/curl/bin:$PATH"' >> ~/.zshrc

For compilers to find this software you may need to set:
    LDFLAGS:  -L/usr/local/opt/curl/lib
    CPPFLAGS: -I/usr/local/opt/curl/include
For pkg-config to find this software you may need to set:
    PKG_CONFIG_PATH: /usr/local/opt/curl/lib/pkgconfig

brew link is a one-line command and echo 'export blah is a one-line command, so I don't see a major increase in difficulty using curl, personally.

@billinghamj

This comment has been minimized.

Copy link

billinghamj commented Jul 25, 2018

Thanks.

I believe a lot of people were using brew link for this purpose. It might be worth adjusting the refusal message to include the same statement as above "If you need to have this software first in your PATH run" etc

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jul 25, 2018

It'd be relatively easy to throw out to Caveats to simply print the caveats again but I don't know how much appetite there is to change this from Mike/ILZ given 1) Anyone running 10.14 or Xcode 10 should theoretically be comfortable enough with software/developer tools/etc to handle this change themselves, 2) This is presumably temporary until 10.14 is stable/GM.

I'll let one of them comment as they wish to before I stick my head into this one too much.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jul 25, 2018

More likely the approach will be applied to <= 10.13 as well, possibly with a deprecation warning first.

@MikeMcQuaid MikeMcQuaid deleted the MikeMcQuaid:fewer-mojave-force-links branch Jul 26, 2018

MikeMcQuaid added a commit to MikeMcQuaid/brew that referenced this pull request Jul 26, 2018

link: when refusing link display keg only caveats
These are a bit easier to follow and have been recently improved.

Inspired by conversation in Homebrew#4441.

@lock lock bot added the outdated label Aug 25, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Aug 25, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.