Skip to content

Add CRYSTAL_CONFIG_LIBRARY_RPATH compiler config#14319

Closed
straight-shoota wants to merge 1 commit intocrystal-lang:masterfrom
straight-shoota:feat/config-rpath
Closed

Add CRYSTAL_CONFIG_LIBRARY_RPATH compiler config#14319
straight-shoota wants to merge 1 commit intocrystal-lang:masterfrom
straight-shoota:feat/config-rpath

Conversation

@straight-shoota
Copy link
Member

It's already possible to configure the default value for CRYSTAL_LIBRARY_PATH. This PR adds the same for CRYSTAL_LIBRARY_RPATH. I had already assumed this would work by trying to use it in the homebrew formula. https://forum.crystal-lang.org/t/crystal-installation-using-linuxbrew-is-not-working/6559/6?u=straight-shoota
But it doesn't. So time change that!

@crysbot
Copy link
Collaborator

crysbot commented Feb 24, 2024

This pull request has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/crystal-installation-using-linuxbrew-is-not-working/6559/15

# do not call `CrystalPath.expand_paths`, as `$ORIGIN` inside this env
# variable is always expanded at run time
ENV.fetch("CRYSTAL_LIBRARY_RPATH", "")
ENV.fetch("CRYSTAL_LIBRARY_RPATH", {{ env("CRYSTAL_CONFIG_LIBRARY_RPATH") || "" }})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add it into Crystal::Config as in the library_path case?

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 want to merge this with little friction. We can refactor it later.

@straight-shoota
Copy link
Member Author

CRYSTAL_RPATH could be removed in the future because its use case on Windows has vanished (https://forum.crystal-lang.org/t/crystal-installation-using-linuxbrew-is-not-working/6559/7?u=straight-shoota, https://forum.crystal-lang.org/t/crystal-installation-using-linuxbrew-is-not-working/6559/16?u=straight-shoota).

So with the future of the target var questionable, it's not clear if adding this configuration variable really makes sense. I suppose it wouldn't hurt. So we can merge it and if CRYSTAL_RPATH goes away, it will go away as well. But until then, we can use it (as I had expected it to work already). And it's not decided yet and we might even keep CRYSTAL_RPATH around in the end.

#14318 should be enough to solve the original problem this was supposed to fix. So this isn't strictly necessary for the intended use case.

@straight-shoota straight-shoota deleted the feat/config-rpath branch February 12, 2025 22:26
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.

3 participants