The idea behind this looks fine to me, but I'll give it a more thorough review late today. Thank you!

It kinda sucks libwebp-sys2 doesn't even bother trying pkg-config when on cross before deciding to just statically build their vendored libwebp... (since it would theoretically just work fine for us)

Edit: Wait wouldn't this fail for cross build/host pairs where stdenv.buildPlatform.canExecute stdenv.hostPlatform (e.g. x86_64->i686)?

Edit: Wait wouldn't this fail for cross build/host pairs where [...]

Indeed it would, but intentionally because it would have the insecure libwebp version. currently there is no easy way to build a secure version on those systems.
One intermediate solution would be to add a withSystemLibwebp which is !cross && !musl, but then also set knownVulnerabilities if the the flag is false. So cross-compile would then be allowed, but only with NIXPKGS_ALLOW_INSECURE=1

Yeah I'd rather set meta.knownVulnerabilities than fail the build. I think we can just set both meta.knownVulnerabilities and disable the installCheck based on the same stdenv.hostPlatform.isStatic || stdenv.buildPlatform != stdenv.hostPlatform conditional

So, curiously enough, I cross-compiled gst-plugins-rs (x86_64-linux -> aarch64-linux) and it does still link against system libwebpdemux.so, despite what the upstream libwebp-sys2 crate seems like it should be doing based on the build.rs 🤔

[lily@build01:~/nixpkgs]$ readelf -a /nix/store/23prbi00d4mqy2328mnn2jnmrgnxf0nb-gst-plugins-rs-aarch64-unknown-linux-gnu-0.11.0+fixup/lib/gstreamer-1.0/libgstrswebp.so | grep -i libwebpdemux.so
 0x0000000000000001 (NEEDED)             Shared library: [libwebpdemux.so.2]

And indeed the symbols are dynamically loaded (i.e. it's not just unnecessarily marked "needed" for some reason in the ELF header)

[lily@build01:~/nixpkgs]$ objdump -T /nix/store/23prbi00d4mqy2328mnn2jnmrgnxf0nb-gst-plugins-rs-aarch64-unknown-linux-gnu-0.11.0+fixup/lib/gstreamer-1.0/libgstrswebp.so | grep -i webp
/nix/store/23prbi00d4mqy2328mnn2jnmrgnxf0nb-gst-plugins-rs-aarch64-unknown-linux-gnu-0.11.0+fixup/lib/gstreamer-1.0/libgstrswebp.so:     file format elf64-little
0000000000000000      DF *UND*  0000000000000000  Base        WebPAnimDecoderDelete
0000000000000000      DF *UND*  0000000000000000  Base        WebPAnimDecoderNewInternal
0000000000000000      DF *UND*  0000000000000000  Base        WebPAnimDecoderGetInfo
0000000000000000      DF *UND*  0000000000000000  Base        WebPAnimDecoderOptionsInitInternal
0000000000000000      DF *UND*  0000000000000000  Base        WebPAnimDecoderHasMoreFrames
0000000000000000      DF *UND*  0000000000000000  Base        WebPAnimDecoderGetNext
00000000000117a0 g    DF .text  000000000000000c  Base        gst_plugin_rswebp_get_desc
0000000000011748 g    DF .text  0000000000000058  Base        gst_plugin_rswebp_register

Ah it actually checks pkg-config first in all but the case where the Cargo static feature is enabled on the crate, so I misread the build.rs initially. We do not I think enable the static crate feature for stdenv.hostPlatform.isStatic anyway for Rust, just add -C target-feature=+crt-static to RUSTFLAGS. And gst-plugins-rs does not add the static feature either afaict (also this is probably never gonna successfully statically build in nixpkgs anyway, without a lot of effort, because too much gobject stuff to untangle...)

So I think it should never be using the vendored libwebp in gst-plugins-rs in nixpkgs and we can just unconditionally run this installCheck (since it also does not depend on native code execution, although cross forces it off anyway in stdenv.mkDerivation for legacy reasons...)

Ah it actually checks pkg-config first in all but the case where the Cargo static feature is enabled on the crate

Great, even better!

I would like to keep this permanently. One thing less to worry about when the next libwebp vulnerability comes around.
Other dependencies can be added to the check as needed.

Other dependencies can be added to the check as needed.

I meant the conditional would need to be adjusted to not bother with the webp plugin check and instead the tests in the installCheckPhase would need to be dynamically added based on selected plugins

But again that can be done if/when others are added, since I really don't care enough

Just realized, I forgot to say thank you for checking on this package!

The libwebp thing seems a bit of an unfortunate mess of random vendoring and I appreciate your (and everyone's) work on it!

Could we rm -r the vendored libwebp instead to have if fail early?

Could we rm -r the vendored libwebp instead to have if fail early?

I'm not sure how we'd do that for a vendored libwebp copy in a dependent crate. If you have something more specific, I suppose we could add it though (although it shouldn't be using the vendored libwebp in our gst-plugins-rs anyway on major platforms or for cross to major platforms afaict)

Ah, looks like importCargoLock does not have mechanism for that. Nevermind then.