Skip to content

Add compiler path to $PATH and $CRYSTAL_EXEC_PATH for subcommands #15186

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

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Nov 12, 2024

Resolves #15145

This patch contains a breaking change of dropping the environment variable $CRYSTAL (introduced in #14953 1.14.0) in favour of the newly added ones.

Quoting from #15145:

I think it should be acceptable to replace the $CRYSTAL variable. We could consider keeping it along, but if we do that, we'll need to do that for a long time. I think it's better to drop it in order to avoid new subcommands ever depend on it.

@straight-shoota straight-shoota force-pushed the feat/compiler-subcommand-path branch from 13afe7e to c8d8a61 Compare November 13, 2024 21:00
@straight-shoota
Copy link
Member Author

I dropped the test code for exit_code because it was failing on Windows. It was unrelated to this change anyway. I'll investigate further and add it in a follow-up.

@bcardiff
Copy link
Member

An upside of the $CRYSTAL env variable is that if the compiler is not crystal but something else crystal-1.0, crystal-1.14, crystal-2 then the external commands still works.

I don't mind prepending to the $PATH and having a $CRYSTAL_EXEC_PATH, but I don't see how we enable binaries named differently.

$CRYSTAL is also used in shards as a convention, so I thought it was convenient to keep the same pattern. (which happens to be the same as in cabal with $CABAL)

@bcardiff
Copy link
Member

If no one thinks that supporting different names of the compiler binary is needed then 👍

@RX14
Copy link
Member

RX14 commented Nov 16, 2024

The interface between the compiler and extension scripts is likely to be a long-lived interface, so I think it's worth spending the time to get it working when the compiler isn't named just crystal. This PR is an improvement already, but I think using $CRYSTAL is a good thing to encourage.

@straight-shoota straight-shoota changed the title Add compiler path to $PATH for subcommands Add compiler path to $PATH and $CRYSTAL_EXEC_PATH for subcommands Feb 24, 2025
@straight-shoota
Copy link
Member Author

CI on mingw-w64 is failing due to #15516.
This is the first time we're adding a new test for a new behaviour of the compiler executable, so it's the first time we notice that the CI workflow doesn't work correctly.

@straight-shoota
Copy link
Member Author

straight-shoota commented Feb 26, 2025

The MinGW-W64 CI workflow appears to have even more issues: #15516 (comment)

I might try to skip this spec then so we can move forward here.

@straight-shoota straight-shoota added this to the 1.16.0 milestone Feb 28, 2025
@straight-shoota straight-shoota merged commit c5598e8 into crystal-lang:master Mar 25, 2025
40 of 41 checks passed
@straight-shoota straight-shoota deleted the feat/compiler-subcommand-path branch March 25, 2025 16:43
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.

Propagating the target compiler to subcommands
4 participants