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

New LLVM 16 release to fix Homebrew installing zstd in nonstandard location #25002

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alan-j-hu
Copy link
Contributor

Should fix the issue at alan-j-hu/llvm-dune#18. @jordydickinson do you think you could test this out and make sure it works for you?

@jordydickinson
Copy link

How do I test this?

@alan-j-hu
Copy link
Contributor Author

alan-j-hu commented Dec 31, 2023

Can you try opam repo add llvm-homebrew https://github.com/alan-j-hu/opam-repository.git#llvm-16-fix-homebrew? Let me know whether the fix works for you or if this command doesn't work.

@jordydickinson
Copy link

It installs with no errors.

@alan-j-hu
Copy link
Contributor Author

alan-j-hu commented Dec 31, 2023

The problem which this release fixes involves the interaction of multiple factors between Apple, Homebrew, and LLVM:

Apple's previous Macs used Intel, but its newest Macs use ARM (the Mac M1 and onward). To ease the transition, Apple has an emulation layer called Rosetta that can execute Intel binaries on ARM. Therefore, Intel and ARM binaries may for now coexist on the new ARM-based Macs. The older versions of Homebrew installed packages in the "standard" location, /usr/local. However, in the interest of having Intel binaries and ARM binaries coexist, Homebrew began installing packages compiled for ARM in /opt/homebrew. This change is discussed at Homebrew/brew#9177. Another reason for the change cited in the discussion is the desire for Homebrew not to install packages in the "standard" location /usr/local, where they may collide with other installations. However, this reason seems to be an ex-post-facto justification, as this change in fact breaks other software that expects to find packages in the standard location.

As a security feature on Apple, child processes ignore the LD_LIBRARY_PATH variable. There is no way to change the default path for searching for libraries.

The llvm-config tool outputs the necessary flags to compile and link programs using the LLVM libraries. The flags for system libraries are hardcoded and llvm-config tool does not perform any search for packages. The linker will see the -lzstd flag and look for the shared object file in the standard locations, but will not find the Homebrew-installed zstd package on ARM-based Macs.

The Homebrew "recipe" for zstd alters the path in the .pc file, allowing pkg-config to find the location of zstd.

So, the causes of the issue are as follows:

  • Apple switching from Intel to ARM for its Macs
  • Homebrew installing packages compiled for ARM to a nonstandard location so that Intel binaries and ARM binaries can coexist
  • Apple not providing a way to change the default path for looking up library files for security reasons
  • llvm-config hardcoding the -lzstd flag and not performing any logic to look for system dependencies
    • pkg-config can find the zstd package because Homebrew changes the prefix in the .pc file

This change is now tripping up multiple people who try to use the zstd package, as seen at https://stackoverflow.com/questions/67840691/ld-library-not-found-for-lzstd-while-bundle-install-for-mysql2-gem-ruby-on-mac and https://discourse.llvm.org/t/kaleidoscope-on-m1-mac-help/69010. It's not clear to me which party is ultimately "responsible" for fixing this issue, but I'm going ahead and fixing the issue in this package by explicitly looking up the zstd package using pkg-config, adding a dependency on conf-pkg-config. This change is an inconvenience that is ultimately beyond the responsibility of the opam ecosystem, affecting people who are trying to use zstd in their C code in projects that don't have anything to do with OCaml.

I feel that opam packages generally expect Homebrew to work out of the box, treating it as an ordinary package manager, as os-distribution = "homebrew" is a supported filter for depexts. The fact that Homebrew is now installing packages in a non-standard location is very annoying, and I wonder if this change breaks other opam packages as well. I'm curious what the opam-repository maintainers think about this issue.

Please let me know if my explanation is unclear or incorrect.

@alan-j-hu
Copy link
Contributor Author

alan-j-hu commented Apr 6, 2024

It's been several months since I opened this PR and there has been no action from the opam-repository maintainers. I want to understand what they think of this PR, and of the changes on Mac and Homebrew that broke stuff. If this PR shouldn't be merged that's fine, but I would like to be told that it shouldn't be merged rather than have a confusing silence.

@samoht
Copy link
Member

samoht commented Apr 10, 2024

Apologies - this is a tricky package, and I suppose none of the maintainers have time to understand the various issues at play. I think that would help if you could review the CI failures (in https://opam.ci.ocaml.org/github/ocaml/opam-repository/commit/4d9fad21a5bc93fd3b1cd19733ce12ee7f259505) and report if these errors are expected or not.

@alan-j-hu
Copy link
Contributor Author

@samoht The compile errors for the binsec package are because of the change from the typed to opaque pointer API. Its LLVM dependency needs to have a version bound of < 16.

The other errors relate to failures to install the system LLVM package in some way or another.

@alan-j-hu
Copy link
Contributor Author

The binsec issue is addressed in #25667. That PR should be merged first, because it's straightforward while this one may involve some consideration.

@shonfeder
Copy link
Collaborator

shonfeder commented Sep 3, 2024

Hello @alan-j-hu! I'm just circling around to see if you think we may be in a position to land this, or at least whether we should put this pack in the queue of PRs needing maintainer attention, now that the PR you noted in #25002 (comment) seems to be merged. Let me know what you think!

PS: I re ran the CI so we could see how things stand now.

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

Successfully merging this pull request may close these issues.

4 participants