search

philss / rustler_precompiled

Use precompiled NIFs from trusted sources in your Elixir code
181 stars 25 forks source link

Improve error message when precompiled target isn't available #45

Closed fhunleth closed 1 year ago

fhunleth commented 1 year ago

Here's an example of the current error that you get when trying to build for an unavailable target:

==> tokenizers
Compiling 7 files (.ex)

== Compilation error in file lib/tokenizers/native.ex ==
** (RuntimeError) precompiled NIF is not available for this target: "riscv64gc-unknown-linux-musl".
The available targets are:
 - aarch64-apple-darwin
 - x86_64-apple-darwin
 - x86_64-unknown-linux-gnu
 - x86_64-unknown-linux-musl
 - arm-unknown-linux-gnueabihf
 - aarch64-unknown-linux-gnu
 - x86_64-pc-windows-msvc
 - x86_64-pc-windows-gnu
 - aarch64-unknown-linux-musl
    lib/tokenizers/native.ex:6: (module)
could not compile dependency :tokenizers, "mix compile" failed. Errors may have been logged above. You can recompile this dependency with "mix deps.compile tokenizers", update it with "mix deps.update tokenizers" or clean it with "mix deps.clean tokenizers"

It would be nice for this error message to tell you how to manually build. I'm not sure what the intended way is. For tokenizers, it looks like it's to export TOKENIZERS_BUILD=true. For other apps, I think that it's to add configuration.

It doesn't look possible to automatically build if a target isn't available.

I don't mind making a PR. I just wasn't sure how to direct users.

philss commented 1 year ago

Yeah, makes sense. We could instruct the user to force the build, but the problem is that each project may choose a way to do that. Mostly they will set an env var, like you mentioned for the case of tokenizers.

Maybe we can direct the user to the "hexdocs" page of the package that failed? Or perhaps adding a config to set the env var we should use to "force build"? WDYT?

fhunleth commented 1 year ago

Sorry for not responding earlier. Your suggestions seem great, but I wondering if the change to support so many targets minimizes the need for this. Nerves users certainly are in a good place with the current set of targets. I feel that you addressed the issue in a different way, and this works out even better. Closing seems fine for me, IMHO.

philss commented 1 year ago

Alright, so I'm closing this for now. Thank you!