DeterminateSystems / riff

Riff automatically provides external dependencies for Rust projects, with support for other languages coming soon.
https://riff.sh
Mozilla Public License 2.0
486 stars 13 forks source link

registry: remove zstd-sys entry #217

Closed figsoda closed 1 year ago

figsoda commented 1 year ago

AFAIW zstd-sys doesn't depend on zlib or clang

lheckemann commented 1 year ago

This turns out to be a bit tricky unfortunately...

The zstd crate will attempt to build against the zstd source, brought in as a submodule, by default. That means that if a user clones zstd-rs and tries to build it using riff run cargo build, it will fail as follows, even with this registry change:

   Compiling zstd-sys v2.0.5+zstd.1.5.2 (/home/linus/scratch/zstd-rs/zstd-safe/zstd-sys)
error: failed to run custom build command for `zstd-sys v2.0.5+zstd.1.5.2 (/home/linus/scratch/zstd-rs/zstd-safe/zstd-sys)`

Caused by:
  process didn't exit successfully: `/home/linus/scratch/zstd-rs/target/debug/build/zstd-sys-4dc637cf181a4aff/build-script-build` (exit status: 101)
  --- stdout
  cargo:rerun-if-env-changed=ZSTD_SYS_USE_PKG_CONFIG

  --- stderr
  thread 'main' panicked at 'Folder 'zstd/lib' does not exists. Maybe you forgot to clone the 'zstd' submodule?', zstd-safe/zstd-sys/build.rs:262:13
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

One way around this would be to add pkg-config as well and set the environment variable ZSTD_SYS_USE_PKG_CONFIG so that the zstd provided by Nix gets used.

On the other hand, the zstd-sys crate as distributed via crates.io includes the zstd source to build against, and works without any registry entry, so maybe that's a preferable approach, and we should remove zstd-sys from the registry completely.

What do you think?

figsoda commented 1 year ago

add pkg-config as well

That should already be the case in since zstd-sys now depends on pkg-config unconditionally

set the environment variable ZSTD_SYS_USE_PKG_CONFIG

also runtime-inputs since dynamic linking seems to need that on nixos

we can also just remove the zstd-sys entry, not sure which one is better

lheckemann commented 1 year ago

Hm, I suspect removing the entry will be simpler and more reliable, as that will link the C code in statically (i.e. the result does not reference the store path and is more likely to be movable to another machine without any further steps). That seems to fit in with riff's "just work" mission :)

Thanks for pointing this out in any case, the previous entry was definitely wrong!

lheckemann commented 1 year ago

It turns out that this was probably a hack to make this crate work with the bindgen feature enabled. The problem here is that Riff doesn't currently have a mechanism that allows respecting features in its invocation of cargo metadata. So:

Either way, I think that the old registry entry isn't the right approach, so let's merge this.