crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.33k stars 1.61k forks source link

Add `make install` command when building from source #10702

Closed ryanstout closed 3 years ago

ryanstout commented 3 years ago

When building from source, there is some complexity around installing crystal in /usr/bin since it also requires .build/crystal to be copied to /usr/lib/crystal/bin/. Adding a make install would be very helpful, though there's some complexity around overwriting the existing crystal install.

Thanks!

jhass commented 3 years ago

I don't think we should recommend end users to make install anything, but it could be a good thing to do to standardize packaging.

Some inspiration on what the standard locations could be: https://github.com/jhass/PKGBUILDs/blob/b62760f9a47d31a117f942362c8e6b885a91acf7/crystal-git/PKGBUILD#L64-L87 (also used in main Archlinux package).

ryanstout commented 3 years ago

Just curious, whats the concern with users doing make install. For example, I've was hitting a bug that was fixed after 1.0, so building from source is my only option currently. I would imagine that wouldn't be that uncommon. Not the first way to install crystal, but helpful when you need it imho.

jhass commented 3 years ago

Because software installed outside a package manager is hard to cleanly update and remove from a system again :)

I keep alias dcrystal="$HOME/projects/crystal/bin/crystal" around to work with a locally built version, there's no real need to install it. If something references crystal implicitly in a script and I want it to use the local build I do export PATH="$HOME/projects/crystal/bin:$PATH" in the respective shell.

straight-shoota commented 3 years ago

I think installing could actually work really well, if you want to use that.

But yeah, standardizing this for packaging might be the real value.

The package building scripts could make use of make install:

straight-shoota commented 3 years ago

The only significant precondition is that we need to include the wrapper script in this repository.

jhass commented 3 years ago

The Archlinux package shows for many years now that proper for distribution packaging needs no wrapper script. I would not like to see for our make install to include any workarounds just to make another project (cross-distribution packaging) easier.

straight-shoota commented 3 years ago

Ok, then the precondition would be to remove the wrapper script.

But doesn't that require to bake the CRYSTAL_PATH config into the binary, essentially mixing build and install? Or, alternatively, place the logic for relative path resolution in the compiler binary. I suppose if we have a install structure defined by make install, this shouldn't be much of a problem. And package maintainers could obviously still use the wrapper script to override the default behaviour.

jhass commented 3 years ago

A package always builds and installs in a coordinated process. Thus baking packaging specific defaults into the binary is perfectly normal. This is what we introduced all the CRYSTAL_CONFIG_ settings for in the first place. The most traditional solution for these kinds of things are ./configure flags which you find in thousands of projects. So think of our CRYSTAL_CONFIG_ variables as our ./configure. Projects in the suckless family actually go as far as making changing any configuration require a recompile. I encourage you again to have a look at the Archlinux package script, it's just a simple shell script that shows this setup.

Now that we got a 1.0 we should actually try to find distribution package maintainers for official distribution packages. They will actually expect behavior like this and be irritated by requiring configuration through a wrapper script.

Such an approach of a wrapper script and what not is still fine for things like our repository where we want to find a packaging style which works across as many distributions as possible with as least effort as possible. But it's the responsibility of the project creating those packages to make that work, it has no place in the main project and the main project should make no decisions or set any defaults just to make that job easier.

straight-shoota commented 3 years ago

Yeah well, I see a problem with baking in a fixed path like CRYSTAL_CONFIG_PATH="/usr/lib/crystal" because it forces a specific install location. With normal package installs, this might be fine because it would likely be the canonical location anyways. But you can't just download a tarball and run that to use a different Crystal build, because it would pick up the stdlib at the canonical location /usr/lib/crystal, not the one that was shipped with the specific compiler version in the tarball. The same goes for the libraries path.

A generic solution to that could be to resolve the stdlib path relative to Program.executable_path with a default value of ../share/crystal/src for example. So the only fixed thing would be the path relation between the compiler binary and the associated stdlib sources. And I think that should be fine, even as a cross-platform solution (although the relative paths may differ based on the operating system). There's still enough flexibility through configuring specific paths at compile or run time if necessary.

jhass commented 3 years ago

A pre-compiled tarball is essentially a variant on a cross-distribution package. Shipping a wrapper script with such a package to make it work seems entirely reasonable. I don't understand why we would need to change the compiler's behavior to accomodate that special usecase.

straight-shoota commented 3 years ago

I assumed if we want to get rid of the wrapper script, we could get rid of it entirely. That makes more sense to me, than keeping it around only for some distribution channels. It would only require a tiny compiler change to make this possible. This seems very reasonable to me.

The pre-compiled tarball is not just a single use case, it also serves as a basis for other channels, such as our deb and rpm packages. It would be great if we can get rid of the wrapper script for those, too (without having to re-build binaries).

jhass commented 3 years ago

Is it a tiny change though? Code-wise maybe, but semantic wise? You wouldn't want to apply this logic to CRYSTAL_PATH obviously but only to CRYSTAL_CONFIG_PATH. But now the semantic of their value is substantially different and thus harder to understand. It'll also make it impossible to include additional paths relative to the working directory (like we do for lib) via CRYSTAL_CONFIG_PATH.

straight-shoota commented 3 years ago

I was thinking of using a special notation to designate a path relative to the executable. Or we could introduce an entirely new config variable for this. Perhaps it could even be a fixed default in the compiler source, where we currently have {{env("CRYSTAL_CONFIG_PATH") || ""}} the empty string could be replaced by relative path resultion at runtime.