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

Propagating the target compiler to subcommands #15145

Open
straight-shoota opened this issue Nov 1, 2024 · 8 comments · May be fixed by #15186
Open

Propagating the target compiler to subcommands #15145

straight-shoota opened this issue Nov 1, 2024 · 8 comments · May be fixed by #15186

Comments

@straight-shoota
Copy link
Member

straight-shoota commented Nov 1, 2024

Since #14953 the compiler can run external executables as subcommands as an alternative to builtin commands.

This kind of subcommand delegation needs to ensure consistency in the compiler version being used.
For example, let's run path/to/crystal subcommand which calls crystal-subcommand from somewhere in $PATH. The subcommand needs to be aware that the target compiler is path/to/crystal, not whatever crystal is in $PATH.

The current solution from #14953 passes the environment variable $CRYSTAL to the subcommand which has the full path to the compiler.

I don't think it works very well. It requires the subcommand to recognize the environment variable. But it must also expect it to be unset when the command is executed directly (as crystal-subcommand). And this trickles down to other processes that the subcommand might spawn. They typically should also be aware of the target crystal compiler. This is practically impossible to realize with the $CRYSTAL environment variable.

git uses a similar mechanism to run subcommands and handles propagating the target instance in a different way: It prepends the path to the parent directory of the executable to $PATH. That means if the subcommand process calls git, it'll resolve to the original one. Path lookup just works when the subcommand is executed directly.

Additionally, git also puts this directory path in the environment variable $GIT_EXEC_PATH. I'm not familiar with the specific details when one is used over the other, but it looks like the prepended $PATH and $GIT_EXEC_PATH are identical.

I think we should add the compiler's parent directory to $PATH. This feature is still brand new and AFAIK not actively used, so 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.

Introducing a separate $CRYSTAL_EXEC_PATH could also be valuable. I believe it could be a more generic replacement to $CRYSTAL_SPEC_COMPILER_BIN which we use in the spec suite.

@straight-shoota
Copy link
Member Author

I think we should also drop the current mechanism with the CRYSTAL environment variable.

That'll break existing command integrations relying on this veriable. Considering that the entire subcommand feature is new in 1.14 I don't expect much use of it though.

The trouble is that there's really no mechanism to deprecate an environment variable. If we keep supporting it, it'll inevitably be used. And then we effectively can never drop it, or have a breaking change later on when there's potentially more integrations depending on it.
Breaking it this early is acceptable and the best cause of action in my opinion.

@straight-shoota straight-shoota linked a pull request Nov 12, 2024 that will close this issue
@straight-shoota
Copy link
Member Author

I figure we also need a variable $CRYSTAL_EXEC_PATH to keep a reference to the original compiler path, which may not be identical to the process' executable path. For example the latter would be .build/crystal but the command that the user runs is actually bin/crystal. And I suppose we want to keep using bin/crystal instead of the executable path directly.

@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)

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

@straight-shoota
Copy link
Member Author

That's an interesting point 🤔
I'm not sure if there are actually good use cases, though. Usually the compiler entrypoint is called crystal. It's either bin/crystal or the build artifact from make crystal. Technically the artifact name is configurable with CRYSTAL_BIN=crystal-1.0, or the file can be renamed afterwards. Does anybody do that though?

I've sure used names with qualifiers like crystal-1.14 to distinguish different variants of the compiler. But they've always been symlinks to either bin/crystal or the binary from an install location. So the actual entrypoint is still called crystal.

With regards to targeting subcommands, this also raises the question whether crystal-1.14 ext should delegate to crystal-1.14-ext or crystal-ext or both? 🤔

My gut feeling is that this qualifier thing might be more complexity than needed for practical use cases. But I'm really not sure about it.

@bcardiff
Copy link
Member

With regards to targeting subcommands, this also raises the question whether crystal-1.14 ext should delegate to crystal-1.14-ext or crystal-ext or both?

I would say the later. crystal-ext could target multiple crystal versions at the same time.

--

I'm not sure if restricting the packaging to have symlinks to a crystal binary is good.

I don't know if any packaging does that.

Using the same convention in shards seems reasonable to me. It could be that $CRYSTAL has the full path or just the name of we are updating the $PATH. Yet others tweaking the $PATH could lead to mismatch binaries.

I like that using just crystal will work in most cases by tweaking the $PATH. But I would not like to loose the ability to have a solution that always work.

@straight-shoota
Copy link
Member Author

straight-shoota commented Nov 15, 2024

With regards to targeting subcommands, this also raises the question whether crystal-1.14 ext should delegate to crystal-1.14-ext or crystal-ext or both?

I would say the later. crystal-ext could target multiple crystal versions at the same time.

Yeah, for custom local extensions in your path, that seems right.
But for core parts of the compiler that are implemented as subcommands, it would make a lot of sense to build them with the same naming scheme as the compiler itself: If the compiler itself is named crystal-1.14, the playground would be called as crystal-1.14 play and delegate to the subcommand crystal-1.14-play.
In this context it seems odd if the subcommand would be crystal-play because if it's in path, it would lead to the same naming conflict as crystal would if multiple versions are installed. And I believe that's the only reason why you would want to call the compiler anything else than crystal.

@bcardiff
Copy link
Member

Ok. Then both. Current executable name and then crystal-.

For internal commands we could rely on wrapper scripts setting the path thou. But is not different from relying on symlinks I guess.

So... both?

@RX14
Copy link
Contributor

RX14 commented Nov 16, 2024

Priority to executable-name-ext then running crystal-ext makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants