Skip to content

superenv: filter out /usr/local on ARM if necessary#10198

Merged
MikeMcQuaid merged 1 commit intoHomebrew:masterfrom
mistydemeo:superenv_filter_out_usr_local_if_necessary
Jan 5, 2021
Merged

superenv: filter out /usr/local on ARM if necessary#10198
MikeMcQuaid merged 1 commit intoHomebrew:masterfrom
mistydemeo:superenv_filter_out_usr_local_if_necessary

Conversation

@mistydemeo
Copy link
Copy Markdown
Contributor

  • 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 typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?
  • Have you successfully run brew man locally and committed any changes?

This improves superenv's build handling on systems which have both an Intel and an Apple Silicon installation of Homebrew side-by-side.

Homebrew has a hardcoded list of paths to reject from build flags like -I and -L, set to various prefixes which are known to interfere with Homebrew builds (MacPorts, Boxen Homebrew, etc.). On Apple Silicon, we currently recommend two different prefixes: /usr/local for Intel, and /opt/homebrew for Apple Silicon. Users who have two Homebrew installations will see interference between them, specifically when building things using their /opt/homebrew installation. Many packages' buildsystems throw -I/usr/local/include and -L/usr/local/lib to the compiler willy-nilly, even if it's not strictly necessary, and this can end up instructing them to try to link against Homebrew-installed x86_64 libraries in /usr/local over the ARM-native versions in /opt/homebrew.

This PR updates the path filtering logic to add a new case after the hardcoded paths. If the system is a) macOS, and b) running in a non-/usr/local, it will filter out /usr/local paths from flags just like with /opt/local and other similar paths. I've left the behaviour on Linux untouched, under the assumption that if this hasn't come up so far, it either doesn't affect Linux users or it's actually seen as desirable.

I've tested one package (qemu) and this fixes a build error on Apple Silicon. I expect this to come up a lot, especially in the period where not all packages have bottles yet.

/cc @sjackman for Linuxbrew opinions

@mistydemeo mistydemeo added the superenv Homebrew's compiler shims label Jan 2, 2021
@BrewTestBot
Copy link
Copy Markdown
Contributor

Review period will end on 2021-01-05 at 00:00:00 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 2, 2021
@fxcoudert
Copy link
Copy Markdown
Member

I don't have a strong opinion, but: this could potentially impact people not on ARM. So another option is to restrict to prefix == /opt/homebrew (instead of prefix != /usr/local).

@mistydemeo
Copy link
Copy Markdown
Contributor Author

I don't have a strong opinion, but: this could potentially impact people not on ARM.

People not on ARM but in the same situation (Homebrew not in /usr/local) would run into the same build errors. I think it's a feature to filter out libraries from that path, not a bug.

@sjackman
Copy link
Copy Markdown
Contributor

sjackman commented Jan 4, 2021

This PR updates the path filtering logic to add a new case after the hardcoded paths. If the system is a) macOS, and b) running in a non-/usr/local, it will filter out /usr/local paths from flags just like with /opt/local and other similar paths. I've left the behaviour on Linux untouched, under the assumption that if this hasn't come up so far, it either doesn't affect Linux users or it's actually seen as desirable.

I want this feature for Linux too, please! Thank you, Misty!
cc @iMichka

@iMichka
Copy link
Copy Markdown
Member

iMichka commented Jan 4, 2021

I think it's desirable for Linux too. There have probably been already some issues with that, but we never had time to investigate further. On CI it's not a big deal but users building from source might be impacted. I am fine to filter this out for Linux.

@BrewTestBot
Copy link
Copy Markdown
Contributor

Review period ended.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 5, 2021
@mistydemeo mistydemeo force-pushed the superenv_filter_out_usr_local_if_necessary branch from de7b1aa to 2a33de7 Compare January 5, 2021 04:57
@mistydemeo
Copy link
Copy Markdown
Contributor Author

I want this feature for Linux too, please! Thank you, Misty!
I think it's desirable for Linux too.

Removed the OS.mac? guard.

@MikeMcQuaid MikeMcQuaid merged commit 3e59c0b into Homebrew:master Jan 5, 2021
@MikeMcQuaid
Copy link
Copy Markdown
Member

Thanks @mistydemeo!

@mistydemeo mistydemeo deleted the superenv_filter_out_usr_local_if_necessary branch January 5, 2021 17:12
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 5, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated PR was locked due to age superenv Homebrew's compiler shims

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants