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

Add CRYSTAL_LIBRARY_PATH for lookup static libraries #7562

Merged
merged 2 commits into from Mar 29, 2019

Conversation

@bcardiff
Copy link
Member

commented Mar 19, 2019

The compiler will use the CRYSTAL_LIBRARY_PATH environment variable as a first lookup destination for the static libraries that are wanted to be linked.

It follows the other configuration options conventions:

  • Upon building the compiler the CRYSTAL_CONFIG_LIBRARY_PATH variable can be used to define the default embedded.
  • The Crystal::LIBRARY_PATH constant is defined on compiled programs.

Ref: https://forum.crystal-lang.org/t/rfc-link-configuration/546

@asterite

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

Fixes #251 :-)

@bcardiff bcardiff requested review from RX14 and asterite Mar 20, 2019

@asterite
Copy link
Member

left a comment

I'm a bit confused because there are two ENV vars. The ones exposed by the compiler seem to also take into account the runtime ENV vars, not only the ones that were used at compile time. But I guess it's fine (I'm not super familiar with this). Feel free to merge.

@ysbaddaden

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

If the goal is to fix #251 then this is fine, otherwise I fail to see to point. We can already set LIBRARY_PATH (for static libraries) and LD_LIBRARY_PATH (for shared libraries) when building, to help the linker find libraries.

@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2019

The goal is to have a priority lookup of static libraries. Independent of having pkg-config or not.
That will also allow

  • linking against embedded shipped libraries easier.
  • end user to link against other builds of the library without learning to write a .pc and setup pkg-config.

I'm following what it was more or less discussed in the forum.
After this I would like to implement some version of link annotation override from compiler options.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

If this only applies to static libraries, should that maybe be reflected in the env var's name? Like CRYSTAL_STATIC_LIBRARY_PATH?

@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2019

@straight-shoota maybe, but it was too long for my eyes.

We could also append it to -L so it can be used for shared library also if it is prefered. (👍/👎 in comment if this is/isn't wanted)

@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

With the upcoming changes of a custom GC and embedded compiler-rt, I think we need to push this.

If the user has a bdwgc package installed, or another llvm version that is not the one the compiler is used, the pkg-config, if configured, will default to those. Ignoring the ones shipped with the compiler.

We need some setting for libraries that should be prioritized more than pkg-config.

@bcardiff bcardiff force-pushed the bcardiff:feature/crystal_library_path branch from 477123e to 3db6e70 Mar 25, 2019

@bcardiff bcardiff requested a review from ysbaddaden Mar 25, 2019

@bcardiff bcardiff requested a review from asterite Mar 26, 2019

@bcardiff bcardiff added this to the 0.28.0 milestone Mar 27, 2019

"CRYSTAL_VERSION" => Config.version || "",
"CRYSTAL_CACHE_DIR" => CacheDir.instance.dir,
"CRYSTAL_PATH" => CrystalPath.default_path,
"CRYSTAL_VERSION" => Config.version || "",

This comment has been minimized.

Copy link
@wontruefree

wontruefree Mar 28, 2019

Contributor

Could this ever be nil?

This comment has been minimized.

Copy link
@bcardiff

bcardiff Mar 28, 2019

Author Member

Crystal.version probably not since now there is a file with it. This line is from time where git describe was used. It can be handled in another PR if wanted

This comment has been minimized.

Copy link
@r00ster91

r00ster91 Mar 28, 2019

Contributor

I agree. I think that's better than just CRYSTAL_VERSION= and Crystal developers will probably understand CRYSTAL_VERSION=nil.

Suggested change
"CRYSTAL_VERSION" => Config.version || "",
"CRYSTAL_VERSION" => Config.version || "nil",

This comment has been minimized.

Copy link
@j8r

j8r Mar 28, 2019

Contributor

nil isn't a proper semantic version...

This comment has been minimized.

Copy link
@r00ster91

r00ster91 Mar 28, 2019

Contributor

It should just indicate that there is no semantic version.

This comment has been minimized.

Copy link
@bew

bew Mar 28, 2019

Contributor

Actually the version can be nil, see #7602

This comment has been minimized.

Copy link
@wontruefree

wontruefree Mar 29, 2019

Contributor

I didn't expect that. seems like config should not allow nil to be returned.
https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/config.cr#L7

Also. This might want to be its own PR.

@bcardiff bcardiff merged commit 5330f5b into crystal-lang:master Mar 29, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bcardiff bcardiff referenced this pull request Apr 3, 2019

Merged

Bump distribution-scripts #7622

@bcardiff bcardiff deleted the bcardiff:feature/crystal_library_path branch Apr 11, 2019

@RX14

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

I think I would have preferred a CRYSTAL_LIBRARY_OVERRIDES="foo=/path/too/foo.a bar=..." variable, the name seems clearer and easier to use. It'd just override the entire linking lookup logic making it easier to understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.