Shards' `postinstall` considered harmful

Crystal Contrib
81

Reggarding ameba, for github projects you can use it as a github action, so no need to include it in development_dependencies.

Same can be done in gitlab, etc… i.e. let the CI run it.

I think this is the best suggestion in the whole thread. Be able to disable post install of specific shards in shard.yml. backwards compatible and make people on both sides happy at a price of a minimum YAML addition to trigger the behavior change.

5 Likes
82

I think I previously stated my concerns with postinstall, which is one of the reasons I don’t use.

This is how I use a CLI tool dependency in another of my projects:

targets:
  drift:
    main: lib/drift/src/cli.cr
dependencies:
  drift:
    github: luislavena/drift
    version: "~> 0.3.2"

That way I can install things and only build bin/drift when I need it instead of having to remember to set SHARDS_OPTS=--no-postinstall on all my setups or having to remember to do it every time I shards install.

There are other scenarios that I can think of, as maybe a dependency might provide more than one CLI that I don’t need, so why forcing all the compilation on me.

Shameless plug II: did a few videos (here and here) early last year exploring simplying the cross-compilation aspect so tools like Ameba or crystaline can be delivered as binaries without having them as dependencies and paying the tax compiling it every single time we download it (countless watts have been wasted on that).

I found the postinstall: true proposition interesting and perhaps something worth to be explored.

Cheers.

83

Oooh, good thinking.

And, I assume’ false will explicitly not run it with a “Supressing postinstall” message. Without it, dependencies without any postinstall will keep working like they always have, and those with will… Error out and make the user update the shards file, or interactively ask if it should be run?

Oh, I like that. Make the user take charge. Even if it’s a dependency of a dependency, you’re still stuck with the consequences of running it.

How about a third option to true and false: a command that will be used instead of the one from the dependency? This would allow one to fix up a postinstall (you’re on a platform that needs an obscure option to the C compiler) or tweak it (just build the one cli tool you’re using rather than the whole package).

(off topic, but…) We got PHPStan on our projects, which is pretty much the same deal, but the recommendation is to run it locally before pushing, as the turnaround time for push, check, get errors, fix, push again, is just slowing things down. Of course, in you might opt for a global installation in that case.

2 Likes
84

Hi, I’m not sure what the resolution for this is but I want to mention I use postinstall for Rust wrapper libraries like: bendangelo/tantivy.cr: Provides Crystal bindings for Tantivy. The fast full-text search engine library written in Rust. - Codeberg.org and GitHub - bendangelo/whichlang.cr: Crystal bindings for Whichlang language detection library

These packages when added to an other persons project, will build the rust library as part of the shards install command. This is very convenient for users that just want it built automatically. It also follows how ruby gems build native extensions.

85

It may be convenient if it works. But if it doesn’t, it’s very inconvenient.
Your postinstall actions have quite a lot of requirements for the development environment. It needs a Rust toolchain and has specific expecations about make and shell compatibility. So it’s quite easy that something goes wrong.

86

When I saw this reply, I figured you will say something like this … :grin:

Regardless, postinstall is a solution that tends to work well most of the time, that enough.

1 Like
87

Absolutely but anyone installing the shard would know that. What’s the alternative? Asking the user post install to cd in the shard and run make?

88

How would they now that? If I’d install the shard, I wouldn’t know.

Yes, I believe something like that would be more ergonomical. This could be integrated in the build system of the depending project, so no explicit user interaction is necessary.
Basically, if I add https://codeberg.org/bendangelo/tantivy.cr.git to shard.yml, I’ll add a make recipe with make -C lib/tantativy.cr to Makefile.

1 Like
89

The discussion is really not meaningful, because no one can remove post install feature from shards.

90

Sure we can. Actually, anyone can do this in their realm!

  • Shard authors can stop using it and instead provide documentation on how to get binary dependencies or whatever postinstall is used for.
  • Shard users can run shards with --skip-postinstall (I’d personally love to have this as default configuration).
  • And the maintainers of shards (i.e. the Crystal Core team) could decide to deprecate (and eventually remove) postinstall entirely.
2 Likes
91

Please don’t do it.

From the perspective of the maintainer：

I don’t know how many shards are using the post-install script, but I feel a lot, please stop doing this, because what you are doing makes an already fragile ecosystem even more fragile.

Could you guarantee that every shard scattered around the world would correct themselves? How long would that take? what are the benefits of removing it? doing it manually would only make it worse.

From the perspective of the user：

And, As someone with some experience, I’ve never confusing with post-installation, as a newbie, post install is actually more useful, do you have many examples of newbie who mess up with the post install process?

Even if it’s messed up, that’s not the responsibility of a post install, maintainer could write better tasks to guide the user.

I think we should do some research before give result, both from the perspective of an user and a maintainer, especially the latter. widely.

3 Likes
92

I believe this circles back to an earlier message of going in circles…

One of the pitfalls of languages like C++ is that the tooling is so poor[1] that it makes poses a big challenge for people trying to learn the language. Crystal already has the advantage by providing Shards and postinstall with it, and while it’s definitely possible to deprecate and remove such functionality I guarantee the effect it would have on the community would be very damaging. Crystal struggles enough with a lack of tooling and editor support, there’s no reason to make things more complicated for newer users.

Also to make things clear, I am by no means against giving users full control over the code they use. Often this is the key to progress where simpler/predefined configurations lack. However, that does not mean that both approaches are mutually exclusive. I incorporate such designs into my applications and libraries so that the user always has the option to switch if they want, rather than being forced to use just one approach. I believe this design is what differentiates well-made projects from not so well-made ones, and Shards should strive to be flexible to support the community.

  1. This should not be taken too literally. I’m sure there are lots of good tooling in the C++ ecosystem, but as a prominent outsider I can say that the experience of trying to get into the language was enough to deter me for a very long time. ↩︎

1 Like
93

Recently while reading the docs of bun for a project, found this section about postinstall safety:

Unlike other npm clients, Bun does not execute arbitrary lifecycle scripts like postinstall for installed dependencies. Executing arbitrary scripts represents a potential security risk.

To tell Bun to allow lifecycle scripts for a particular package, add the package to trustedDependencies in your package.json.

It will ignore the postinstall scripts of dependencies unless those are the list of trusted ones.

But it will continue to execute the project’s own scripts part of the NPM package lifecycle, like prepare: scripts | npm Docs

Thought was worth sharing.

Cheers,

4 Likes
94

I’m growing convinced of dropping both the postinstall and executables features in Shards with no replacement.

TLDR: the conveniences are actually inconveniences; they hinder benefits of using Crystal; they fall beyond Shard’s mission: resolve and install dependencies. Let’s drop them.

The postinstall script was meant to quickly build a vendored C library or generate C bindings automatically after install. It was meant to be a convenience for users that install the library, so they can avoid manually typing commands (then forget to automate) and shard authors wouldn’t have to document these additional steps.

In practice arbitrary code is executed on your machine and when it fails for some reason, then shards fails to install dependencies (todo: insert “you had one job meme”). I’m not even talking about Windows, merely assuming Bash or GNU make will break :fearful:

For the executables I’m still searching for an actual use case. I frankly can’t remember.

Since in many cases the executable to install is built by the post install script, using --skip-postinstall will break, so you must also specify --skip-executables :rage:

As much as I am a proponent of the postinstall script, I can only agree that the conveniences are starting to become painful :face_with_head_bandage:

For example Ameba always building itself and installing its executable is a great demonstration of the problem: Ameba is actually an application. Being installable as a library is a special case for when you need Ameba with additional or custom rules for an application (and that’s very nice). In which case it should be documented how to

Micrate also always builds itself and… that requires the mysql, pg, and sqlite3 shards as hard dependencies which in turn requires to install the libsqlite3-dev system package (see this unresolved issue :face_with_head_bandage:

I believe these usages come from Ruby (among others), where applications and tools are distributed as gems. It’s thus natural to install them as development dependencies.

But this is Crystal, and one of the main selling point is that we can compile to a single file that is easy to distribute… and we’re not even benefiting from that in our own ecosystem, worse, we provide the very tools to hinder it :exploding_head:

IMO we shall drop support for the postinstall script, and executables should become merely informational. The few affected shards should either distribute executables (or let users and packagers build them) and properly document when an external library is expected (which one, where to find it, commands to run to build a vendored copy, etc), just like the sqlite3 shard expects the system to have libsqlite3.so and/or libsqlite3.a installed.

5 Likes
95

I agree with removing that functionality from shards. From a documentation generation perspective (crystaldoc.info), I don’t want to give anyone access to run any arbitrary command on my server, so I build docs with --skip-postinstall, but since some shards rely on having extra steps their docs fail to build.

As an alternative, I think we should investigate and build ecosystem-specific task runner and build system(s). There are benefits to having them specific to Crystal, in that tooling such as larimar or ameba or others could make use of them to know how to interface with a crystal shard (think more fleshed out targets: or Makefile alternative). Then in the docs for your shard you could have something like fragment ameba run to build/run ameba locally, or fragment lexbor build_lib to build the C library necessary for it to work (just examples of what it could look like).

96

How do you use shards in your apps that come with a CLI as well as Crystal code to include in the app?

A couple examples:

Any shard that comes with a CLI tool needs to install its CLI, but Protobuf and gRPC have another aspect to this that may be uncommon in that the user doesn’t even invoke them directly. They’re invoked by the protoc compiler.

I’ll open an issue on the shards repo but I just created a proof of concept for this on Interro. The commit is here. Saving you a click:

  • removes the postinstall script
  • installs a very small shell script instead of an eagerly compiled binary
  • the shell script …
    • opens a subshell which …
      • goes into the shard’s directory so that pwd will be exactly what it is in the postinstall script
      • runs what is basically the postinstall script, which overwrites the shell script with the newly compiled binary
    • runs that binary with the same arguments the shell script received.

I updated one of my apps to use that branch and this is what happens when I run my bin/setup script which, among other things, calls bin/interro-migration for both the dev and test DBs (output trimmed for brevity):

$ bin/setup
First run of interro-migration, compiling...
Running CreateUsers
CREATE TABLE users(
  id UUID PRIMARY KEY NOT NULL DEFAULT gen_random_uuid(),
  email TEXT UNIQUE NOT NULL,
  name TEXT NOT NULL,
  password TEXT NOT NULL,
  role INT4 NOT NULL DEFAULT 0,
  created_at TIMESTAMPTZ NOT NULL DEFAULT now(),
  updated_at TIMESTAMPTZ NOT NULL DEFAULT now()
)
CreateUsers: 1.24ms
...
Running CreateUsers
CREATE TABLE users(
  id UUID PRIMARY KEY NOT NULL DEFAULT gen_random_uuid(),
  email TEXT UNIQUE NOT NULL,
  name TEXT NOT NULL,
  password TEXT NOT NULL,
  role INT4 NOT NULL DEFAULT 0,
  created_at TIMESTAMPTZ NOT NULL DEFAULT now(),
  updated_at TIMESTAMPTZ NOT NULL DEFAULT now()
)
CreateUsers: 1.17ms
...

Note that when it ran bin/interro-migration for the dev DB, it showed that it was the first run and was compiling it. The second run, for the test DB, did not show that output because the binary was already compiled over that placeholder script.

And this is repeatable — I’m running rm -rf bin/interro-migration lib && shards update interro && dropdb $DEV_DB && dropdb $TEST_DB && bin/setup and it does the same every time.

97

My problem here is that this is still a problem that needs solving. It doesn’t need to be exactly postinstall that solves it, but I think there needs to be something that allows shard authors to tell users that something needs to be done. The situation may be ok if we only consider direct first-level dependencies, but having to manually look for extensions in various repos for every single shard that has been updated, will be a pain in the ass. Especially when shard authors start to restructure their shards and break them up etc.

There are mitigations that are possible. For example, it would be a lot less pain in the ass if there was a way to instrument shards to output text whenever a specific shard is updated. Preferably separated into a informational text section for random upgrade news - I know rubygems have this, and a ‘please run this’-section that also could be outputted on demand for a shard or the whole app. That would automate the really unfriendly parts while still making it really obvious to the user what is going on.

I feel it is either some help like that or making C linking integration dramatically better - remove the need for using shims when calling into C nonsense like static inline, preprocessor macros etc.

2 Likes
98

With no replacement is probably a bit harsh. Now, Shards is still in v0 so we can break whatever :smiling_imp:

We could:

  • log the postinstall command after installing/updating a shard, instead of running the command;

  • log a short message declared in shard.yml about not forgetting to run some extra steps; this will be abused so we should keep the message short, for example old twitter style at 140 chars max;

  • transform --skip-postinstall and --skip-executables into --postinstall=foo,bar and --executables=foo to specifically allow the features to explicitly named shards.

    This one reverses the burden: it becomes inconvenient to run, so these would be a last resort (granted that the command happens to work).

  • add commands to run postinstall & install executables for explicit shards (meh)

Frankly, reminding to run cd lib/foo && shards build feels enough; the protobuf shard would have its executable at lib/protobuf/bin/protoc-gen-crystal and that’s fine.

Same for db migration like tools: you can document a three lines script to invoke the CLI command, then we could customize it to our hearts content :heart_eyes:

# bin/my_migration.cr
require "pg"
require "my_migration"
MyMigration.call(ARGV, database_url: ENV["DATABASE_URL"])

Heck, we could even embed it into the main binary :exploding_head:

@jgaskins the shell script is nice… but what about a powershell or cmd terminal with no sh in sight? :boom:

1 Like
99

Thinking out loud (maybe not a good idea): protobuf and grpc could distinguish the protoc plugin from the library? It implies that the API between the plugin and the library must be stable (somehow) as not to break if the executable ain’t exactly the same version as the library.

The library could check that it supports the generated code. For example the plugin generates a macro that checks the version, so the it fails at compile time. The message would explain what to do (update/rebuild plugin & regen).

100

If we drop postinstall (and executables), another solution scoped to crystal program could be to declare in the main shard.yml which targets of each shard to shim/install.

This acts as a way to marking some top level shards as trusted by the author. Let’s remember that just compiling a crystal program can run arbitrary commands, so that is also a security threat.

For shards that want to generate code, maybe the generation should happen as an effect of running the installed targets, the output might be put in ./src/generated/... or something alike. In these cases the code of the target might be a simple wrapper on system commands or even delegate to a bash script if cross platform is not a concern.

I think that shards could manage if such shims are eager or lazy compiled.

Some options of syntax to make things concrete

Install all targets

dependencies:
  protobuf:
    github: jeromegn/protobuf.cr
    targets: true

Install only some targets

dependencies:
  protobuf:
    github: shard-with-many-targets/foo.cr
    targets:
      - foo

Install targets with some rename, because naming is hard and we might have clashes between other shards with commands we want

dependencies:
  protobuf:
    github: shard-generic-target/bar.cr
    targets:
      - name: migrate
        as: bar-migrate
1 Like