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

crystal: install wrapper script to set LD_RUN_PATH pointing to homebrew lib #165986

Conversation

straight-shoota
Copy link
Contributor

@straight-shoota straight-shoota commented Mar 13, 2024

The Crystal compiler requires a linker and by default it just picks up whatever gcc it finds in PATH.
This causes issues with discovering libraries installed via homebrew. We previously tried to solve this with #162182, but that doesn't work.
Now the Crystal compiler has gained support for a configuration value CRYSTAL_CONFIG_CC which allows changing the baked-in default linker path (crystal-lang/crystal#14318). So we can point it directly to the gcc from homebrew. And this resolves all issues with dependency discovery.
The compiler change is not yet released, so it only works with --HEAD.

It's still possible to specify a non-default linker via the environment variable CC at runtime, of course.

This effectively reverts #162182 and replaces it with an alternative which actually works.

@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Mar 13, 2024
@straight-shoota straight-shoota changed the title [crystal] Configure CRYSTAL_CONFIG_CC to bake reference to homebrew's gcc crystal: Configure CRYSTAL_CONFIG_CC to bake reference to homebrew's gcc Mar 13, 2024
@Bo98
Copy link
Member

Bo98 commented Mar 13, 2024

Do we need this on macOS? Original issue sounds like we only need to add this dependency on_linux. crystal run seems to work fine on macOS

Also feels slightly weird installing both LLVM and GCC here, but maybe it's necessary.

@straight-shoota
Copy link
Contributor Author

straight-shoota commented Mar 13, 2024

Do we need this on macOS?

I'm not sure and I'm not familiar with the specifics of macOS. However, I would expect the same problem could apply there as well. If cc resolves to a linker that is not installed via homebrew, it probably might not find the libraries installed only in homebrew?

crystal run seems to work fine on macOS

It used to work on Linux as well, but now it doesn't. And it's not entirely clear what has changed. We should probably be suprised that it worked before 🤷
Note that this is not an issue if the required libraries are available in the system configuration. Then the linker will just pick those and you'll never notice they aren't from homebrew (unless you look into it).

Also feels slightly weird installing both LLVM and GCC here, but maybe it's necessary.

LLVM is only a lib dependency of the compiler. We could trim the dependency down to just libllvm.
GCC is used solely as a linker. Technically, GCC shouldn't be necessary and we should only need a pure linker (such as ld). But currently the Crystal compiler relies on some features that GCC sprinkles on top. This might change in the future. (crystal-lang/crystal#14332).
It should also be possible to use clang as linker, but gcc is typically the default and I'm sure changing that would lead to many suprises.

@Bo98
Copy link
Member

Bo98 commented Mar 13, 2024

On macOS, it links correctly with the default system compiler:

$ crystal build test.cr
$ otool -L test
test:
	/opt/homebrew/opt/pcre2/lib/libpcre2-8.0.dylib (compatibility version 12.0.0, current version 12.2.0)
	/opt/homebrew/opt/bdw-gc/lib/libgc.1.dylib (compatibility version 7.0.0, current version 7.3.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1345.100.2)
	/opt/homebrew/opt/libevent/lib/libevent-2.1.7.dylib (compatibility version 8.0.0, current version 8.1.0)
	/usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0)

this is because on macOS it links with the absolute paths linked in:

$ otool -l test
[...]
Load command 15
          cmd LC_LOAD_DYLIB
      cmdsize 72
         name /opt/homebrew/opt/bdw-gc/lib/libgc.1.dylib (offset 24)
   time stamp 2 Thu Jan  1 01:00:02 1970
      current version 7.3.0
compatibility version 7.0.0
[...]

so it doesn't use an rpath system like Linux does.

This might change in the future. (crystal-lang/crystal#14332).

It's worth noting that if the future direction is linker-level support then our GCC hack won't kick in and you'll need to implement an rpath system for that.

@straight-shoota
Copy link
Contributor Author

I'm happy to opt-out on macOS. IIUC gcc itself is also only modified on linux.

Not sure if it makes sense to keep the gcc dependency though. Crystal needs a C compiler to use for linking. By default it uses cc (or $CC if set). I suppose the homebrew formula doesn't fill this alias, so the dependency is probably mute if it's not hard-coded?
I suppose that could be a reason to keep CRYSTAL_CONFIG_CC on macOS, though. 🤷

Even with gcc dependency, out-of-the-box experience with the crystal formula seems pretty poor, though. I tried brew install crystal on a blank install of Debian linux, with a blank homebrew; it fails miserably.

Formula/c/crystal.rb Outdated Show resolved Hide resolved
Formula/c/crystal.rb Outdated Show resolved Hide resolved
@beta-ziliani
Copy link
Contributor

It might make sense to add the dependency in the formula without restrictions (and be sure to use it, also in macOS). In any case, we can first try this for the Linux version and then see if we really want to bother the macOS version with it.

@straight-shoota
Copy link
Contributor Author

straight-shoota commented Apr 9, 2024

Putting the gcc dependency in on_linux causes a commit style error:

Formulae in homebrew/core should not have a Linux-only dependency on GCC.

@Bo98 Any advice on what I should do? I could revert to f44c79f47dd388f3be5205c082fc1ac53f6897d9 which seemed fine. That adds gcc as a dependency on all platforms, which isn't wrong (you need a C compiler to use Crystal).

@SMillerDev
Copy link
Member

GCC is the default on Linux so no need to specify a dependency unless you need a specific version

@straight-shoota
Copy link
Contributor Author

@SMillerDev I tried to remove the gcc dependency entirely, but then Formula["gcc"] is nil and we can't get the path to the C compiler from there. How could this work?

@straight-shoota straight-shoota marked this pull request as draft April 9, 2024 18:06
@straight-shoota
Copy link
Contributor Author

I'm temporarily closing this PR to let autobump update the formula.

@straight-shoota
Copy link
Contributor Author

Audit seems to be unhappy with having gcc as a dependency only on Linux, so I guess it should really just be a general dependency.

@straight-shoota straight-shoota marked this pull request as ready for review May 2, 2024 21:01
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label May 17, 2024
@straight-shoota
Copy link
Contributor Author

straight-shoota commented Jun 5, 2024

I'm temporarily closing this PR to let autobump update the formula. (again)

Copy link
Contributor

github-actions bot commented Jul 7, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Jul 7, 2024
@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Aug 21, 2024
@straight-shoota straight-shoota changed the title crystal: Configure CRYSTAL_CONFIG_CC to bake reference to homebrew's gcc Install wrapper script to set LD_RUN_PATH pointing to homebrew lib Aug 21, 2024
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Sep 11, 2024
@carlocab carlocab changed the title Install wrapper script to set LD_RUN_PATH pointing to homebrew lib crystal: install wrapper script to set LD_RUN_PATH pointing to homebrew lib Sep 11, 2024
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Sep 11, 2024
Formula/c/crystal.rb Outdated Show resolved Hide resolved
@github-actions github-actions bot added autosquash Automatically squash pull request commits according to Homebrew style. and removed stale No recent activity labels Sep 12, 2024
@carlocab carlocab added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Sep 17, 2024
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Sep 17, 2024
@carlocab carlocab force-pushed the feat/crystal-config-cc branch 2 times, most recently from 91a736c to 66d428b Compare September 17, 2024 05:16
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

Copy link
Contributor

⚠️ @carlocab bottle publish failed.

@carlocab
Copy link
Member

Blocked by #181351.

@carlocab carlocab mentioned this pull request Sep 22, 2024
6 tasks
carlocab pushed a commit to lukeshingles/homebrew-core that referenced this pull request Sep 22, 2024
…brew lib

- revision bump (use llvm@18)

Closes Homebrew#165986.

Co-authored-by: Luke Shingles <luke.shingles@gmail.com>
@cho-m cho-m added the rollup proposed It has been proposed that this PR's commits are cherry-picked and merged as a part of another PR label Sep 23, 2024
@carlocab
Copy link
Member

Merged in 085c886. Thanks @straight-shoota!

@carlocab carlocab closed this Sep 23, 2024
@straight-shoota straight-shoota deleted the feat/crystal-config-cc branch September 23, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. rollup proposed It has been proposed that this PR's commits are cherry-picked and merged as a part of another PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants