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

Allow overriding CRYSTAL_PATH in the wrapper script #9632

Merged

Conversation

bcardiff
Copy link
Member

This will allow setting the CRYSTAL_PATH in the same fashion as in official packages.

CRYSTAL_PATH=/path/to/prepend:$(bin/crystal env CRYSTAL_PATH)

It is mainly convenient for test-ecosystem to reduce the noise on specific deprecation warnings by overriding some files of the std-lib.

@jhass
Copy link
Member

jhass commented Jul 23, 2020

Mmh, feels like a potential footgun. bin/crystal is intended to run the compiler in the local repo and thus heavily relies on $CRYSTAL_ROOT/src being inside the path. I can easily imagine setups, say version switchers akin to RVM or dependency managers competing with shards, that make use of setting CRYSTAL_PATH globally and thus break working on the compiler repo in subtle ways.

Maybe this should either ensure $CRYSTAL_ROOT/src to be in CRYSTAL_PATH (but where to append it? Hard problem.) or just error out or at least warn if its not.

@bcardiff
Copy link
Member Author

I like the proposal of warning if $CRYSTAL_ROOT/src is not mentiones in CRYSTAL_PATH

@bcardiff bcardiff requested a review from jhass July 23, 2020 14:44
@bcardiff
Copy link
Member Author

Something I forgot to mention is that this allows shards specs to work with the wrapper script since in their spec the CRYSTAL_PATH is set.

@bcardiff bcardiff merged commit 7d13f68 into crystal-lang:master Jul 24, 2020
@bcardiff bcardiff added this to the 1.0.0 milestone Jul 24, 2020
@bcardiff bcardiff deleted the feature/wrapper-script-override-path branch July 24, 2020 18:38
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