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 CRYSTAL env var to determine compiler #415

Closed

Conversation

bcardiff
Copy link
Member

@bcardiff bcardiff commented Jun 23, 2020

Fixes #406

Shards will read CRYSTAL env var, fallback to crystal, to determine which crystal compiler to use.

This also enables that a compiler+shards package can force to use the compiler of that same packaging if wanted.

Something that is not handled is how the executables + postinstall can deal with this override.

A dependency is forced to use in the postinstall a ${CRYSTAL:-"crystal"} instead of crystal which is not great.
Another alternative is to encourage shards build instead of crystal build that way shards will be invoked again and can choose the appropiate crystal compiler since the environment is preserved.
But we actually have the same problem, the postinstall assumes that shards is on the path. So what? we should have a ${SHARDS:-"shards"}? Not great.

I think that the scenario of having executable helpers in the bin directory that are compiled by dependencies on postinstall should be more declarative. That would be a way to solve it I think.

bcardiff added 3 commits June 23, 2020 15:26
Although it is not used. But that's the pattern followed in config.cr for now.
Avoid requiring crystal to be available in path.
This can allow wrapper scripts on shards and have a cleaner override of the CRYSTAL_VERSION to use to resolve dependencies.
@jhass
Copy link
Member

jhass commented Jun 23, 2020

Can't we just figure out the directory $CRYSTAL lives in and prepend it to the $PATH before invoking postinstall? Or if we want to avoid whatever else might be in that directory to appear in $PATH, we could symlink $CRYSTAL to /tmp/something/crystal and push /tmp/something into $PATH.

@bcardiff
Copy link
Member Author

I think that prepending current shards and crystal location to the path when invoking scripts from shards is acceptable.

@jhass
Copy link
Member

jhass commented Jun 24, 2020

I just realized, the symlink approach could have another advantage: it removes the need that $CRYSTAL points to something called crystal. It is not uncommon to deal with parallel installation of multiple versions of something by renaming some of the executables, so you would have for example /usr/bin/crystal and /usr/bin/crystal-1.2.

@bcardiff
Copy link
Member Author

Good point. I usually have directories like crystal-x.y/bin/crystal. So I didn't consider it.

Should shards create & prepend the directory to the path only when needed? Identifying if the resolution to shards and crystal leads to the same executable that is running and to the Shards.crystal_bin location?

Those /usr/bin/crystal-1.2 that you and @straight-shoota seems to have are those symlinks? Or there are custom made wrapper scripts?

@jhass
Copy link
Member

jhass commented Jun 24, 2020

Should shards create & prepend the directory to the path only when needed? Identifying if the resolution to shards and crystal leads to the same executable that is running and to the Shards.crystal_bin location?

I think it's fine to start simple and make it smart afterwards. That is I'd be totally fine if any shards install invocation sets up the temp directory & symlink early and only cleans it up on process termination. Then iterative optimizations upon that can be to

  • lazily setup only once the first postinstall is hit
  • skip the setup if currently running shards comes first in current $PATH and current $CRYSTAL already comes first in path and both are named shards and crystal respectively.

Those /usr/bin/crystal-1.2 that you and @straight-shoota seems to have are those symlinks? Or there are custom made wrapper scripts?

I don't actually have that anywhere right now. It's just common to see from packages of other things, especially languages. I know for certain that Archlinux, Debian and Fedora/RHEL families of distributions install /usr/bin/ruby-2.6 or /usr/bin/ruby2.6 if you install a ruby2.6 package while the ruby package is say 2.7. Also it's basically ubiquitous to have /usr/bin/python, /usr/bin/python2 and /usr/bin/python3. Whether things are symlinks into /opt/, wrapper scripts that setup some environment variables (hello virtualenv, rbenv) or just executables with different standard paths compiled in shouldn't matter. Crystal would support all three modes of operation right now already, we should only make sure to preserve such here by simply making no assumptions about it. Symlinking a symlink is entirely fine.

@bcardiff
Copy link
Member Author

Ok, will take the route of lazily setup only once the first postinstall is hit for now.

Something that might not work is if crystal-x.y/shards-x.y is an alias I think. They will not be found in path to append. $CRYSTAL will be forwarded but shards will not resolve to shards-x.y. This is out of my comfort zone 🙈 . It's fine if aliases are not supported, that why I wanted to check how the crystal-x.y was created (yet from the /usr/bin prefix it was obvious they are not aliases)

@jhass
Copy link
Member

jhass commented Jun 24, 2020

Well yes, aliases only exist in the current shell anyhow afaik, it won't even properly work to put them into $CRYSTAL.

Copy link
Member

@jhass jhass left a comment

Choose a reason for hiding this comment

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

Two small things, looks good!

src/script.cr Outdated Show resolved Hide resolved
src/script.cr Outdated Show resolved Hide resolved
@straight-shoota
Copy link
Member

Well yes, aliases only exist in the current shell

They do work in subshells when configured via shellrc.

My two cents: I don't like where this is going. It adds even more build tool features to shards. Shards shouldn't be a build tool. It's a dependency manager. There are lots of build tools out there and most of them are better than shards for this job.

@bcardiff
Copy link
Member Author

@straight-shoota how would you solve that the environment used is preserved along the way? Even if we assume that crystal is in the path, how we ensure the same shards is called in the postinstall?

Brian J. Cardiff and others added 2 commits June 24, 2020 20:12
Co-authored-by: Jonne Haß <me@jhass.eu>
Co-authored-by: Jonne Haß <me@jhass.eu>
@bcardiff
Copy link
Member Author

Plus if CRYSTAL env var is used to choose the crystal compiler to grab the version for the resolution, how we can ensure that that binary is used as the compiler all along?

@straight-shoota
Copy link
Member

how we ensure the same shards is called in the postinstall?

I'd let that be the responsibility of a real build tool.

@jhass
Copy link
Member

jhass commented Jun 25, 2020

So you would remove postinstall? Fact is that while people can just crystal build in a postinstall script, they will. I agree we shouldn't overboard, but I view this as the minimum effort we can make to not have that break in hilarious and hard to debug ways.

@waj
Copy link
Member

waj commented Jun 26, 2020

I don't think having a postinstall hook makes Shards a build tool. I don't think it's just a dependency manager either. It's the tool that allows building, managing and (someday) publishing shards. Ideally many projects would just specify postinstall: make.

To me the noise is coming from the relationship with the targets spec. That is totally expendable and it would remove the need of exposing the shards binary in any way to the postinstall step. But I think the idea was always give an option to not require the users to deal with makefiles, bringing a default behaviour to call the Crystal compiler.

With the risk of turning this conversation away from the topic of this PR, maybe we could extend the postinstall syntax to integrate it better with targets and thus not requiring to specify a command that invokes shards. For example this could indicate that the target foo must be built after the shard is installed:

postinstall:
  target: foo

I may even suggest that the postinstall could be (optionally) an array, so more than one target or command can be specified. Some might be thinking "Oh no... MORE features", but this would provide more cohesion within the configuration.

If this message receive at least one thumbs up, I'll open a RFC so we could discuss the feature there 🙂

@bcardiff
Copy link
Member Author

I will give the 👍 because I mention something like that in #415 (comment)

I think that the scenario of having executable helpers in the bin directory that are compiled by dependencies on postinstall should be more declarative. That would be a way to solve it I think.

Yet, that will not solve the issue that if a makefile is used either shards or crystal might be called there.

@bcardiff
Copy link
Member Author

It seems that whatever solution we try to achieve here will lead to half-way implementation. Although #406 seems legit at first, shards is already bound to crystal binary in reality since post_install steps might call crystal compiler.

It was a nice excercise though :-)

For now it seems that the best things is to close this PR and maybe #406.

@bcardiff bcardiff closed this Jul 29, 2020
@jhass
Copy link
Member

jhass commented Jul 29, 2020

I didn't get what the outstanding issue with this approach is. Also let's not get the aim for a perfect solution to be in the way of useful iteration (that theme seems to expose in a couple of other places as well...). Once we find it, we still can move towards the perfect solution, improving status quo is not useless in the meantime.

@bcardiff
Copy link
Member Author

Part of the line of thought was

#406 proposed another way to determine which crystal version is wanted. Choosing the binary push the story of how to honor that choice within the postinstall steps.

If the postinstall where more declarative, not requiring to explicitly call crystal in a shell script there will be no need to set the PATH since shards could do that per target directly.

But users might still prefer to use make or other build tools and call crystal.

Maybe in the future we can have a standardize an env var like CC to choose the crystal compiler, that would remove the need for changing the PATH with temporal directories. It will also allow compilers named crystal-x.y to be used. Shards could set a fallback to that env var.

Part of my motivations here was to pave the road for a self-containted .tar.gz where the crystal and shards binary there will call each other. This will be better handle by wrapper scripts of crystal and shards that will prepend the ./bin location relative to the wrapper scripts to PATH.

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

Successfully merging this pull request may close these issues.

Resolution of crystal dependency is hard tied to crystal executable
4 participants