Make `bindgen` an optional build-time dependency, and use a pre-computed `bindings.rs` file by default.

[jcnelson]

This PR disables bindgen as a build-time dependency by default, because it makes it damn nigh impossible to build on musl-based libc systems like Alpine docker containers. Instead, a pre-generated bindings.rs file will be used. If the user desires, they can invoke bindgen to create the bindings with the with_bindgen feature.

[xoloki]

Hey @jcnelson , something kinda weird is going on here.

When I build p256k1 using glibc on ubuntu-22.04, I get no FFI warnings, and my bindings.rs file get placed in

./target/debug/build/p256k1-f57a9a24c5274c5b/out/bindings.rs

When I build your fork on the same glibc and ubuntu-22.04, I get the warnings you noted, and the bindings.rs file is located in

./p256k1/src/bindings.rs

There's a diff between the two files:

x@xocotl:~/src/p256k1$ diff ./target/debug/build/p256k1-f57a9a24c5274c5b/out/bindings.rs ../jcnelson/p256k1/p256k1/src/bindings.rs 
0a1,4
> #![allow(dead_code)]
> #![allow(non_snake_case)]
> #![allow(non_camel_case_types)]
> #![allow(non_upper_case_globals)]
54c58
< pub const __GLIBC_MINOR__: u32 = 35;
---
> pub const __GLIBC_MINOR__: u32 = 37;
191d194
< pub const __GNUC_VA_LIST: u32 = 1;
2699a2703,2711
>     pub fn arc4random() -> __uint32_t;
> }
> extern "C" {
>     pub fn arc4random_buf(__buf: *mut ::std::os::raw::c_void, __size: usize);
> }
> extern "C" {
>     pub fn arc4random_uniform(__upper_bound: __uint32_t) -> __uint32_t;
> }
> extern "C" {
2998d3009
< pub type va_list = __builtin_va_list;
3522a3534
> pub type va_list = __gnuc_va_list;

I suspect the GLIBC_MINOR part of the diff is the source of the FFI errors. I'll play with that here, can you see what happens if you apply this diff on your end?

I'm also concerned about what happens in the with_bindgen case. cargo publish doesn't like it when generated files go anywhere outside of the output directories.

[jcnelson]

I added the #! macros at the top of the bindings.rs file. The bindings.rs file here was generated from Debian unstable, which might have a different GNU libc than Ubuntu 22.04.

I'm not too worried about the FFI errors anymore because on further inspection, they're in libc functions that aren't used anywhere -- IIRC neither libsecp256k1 nor our code have to do any conversions between 128-bit doubles and strings.

I'm also concerned about what happens in the with_bindgen case. cargo publish doesn't like it when generated files go anywhere outside of the output directories.

I'm curious what cargo publish outputs here? The with_bindgen feature simply preserves the build behavior of this crate without my patch. If this feature is omitted (the default), then src/bindings is used instead (which I'm surprised cargo publish is complaining about, because AFAICT it should be treated as just another source file?).

[xoloki]

I tried patching GLIBC_MINOR, didn't fix the default-features build.

I then tried the -F with_bindgen build and everything worked normally. We end up with two copies of bindings.rs, one in src and one in target.

[jcnelson]

I then tried the -F with_bindgen build and everything worked normally. We end up with two copies of bindings.rs, one in src and one in target.

That's expected. src/bindings.rs is just another source file. The build.rs file creates $OUT_DIR/bindings.rs when with_bindgen is enabled, and the lib.rs file uses that bindings.rs instead of src/bindings.rs with this feature.

[xoloki]

Yeah I think cargo publish won't be a problem, since as noted above we don't replace the src version we just get another in target which is used instead.

If you aren't worried about the FFI errors, then do we need to merge this change? I can't publish a crate if the default build gives FFI warnings.

[jcnelson]

We do need to get this crate to build on Alpine, though, since that's what the Stacks blockchain CI uses as its Docker image base images. IIRC rust-libsecp256k1 gets around this problem using a similar tactic, btw -- it has its own bindings.rs file that is not built by bindgen.

Do the FFI warnings prevent you from building on Alpine?

I'm okay with merging something to fix the Alpine build, just don't want to make the glibc build have the same errors. It's really weird that your bindings.rs gives me the same errors that it fixes for you.

[jcnelson]

Alright, no more FFI warnings when building with src/bindings.rs (1dbb744)

[xoloki]

Ok, your latest commit seems to fix the build warnings on my end. I'll try to build wsts and stacks-signer using your p256k1 on ubuntu and mac, do a test publish, and hopefully that will be it.

[xoloki]

Actually, can we do this in reverse? Can we make it so the default is to use bindgen, but you can turn it off with a feature? I'm kinda leery of making the default bindings.rs specify a GLIBC_MINOR version.

[jcnelson]

Sure, will do

[xoloki]

Sure, will do

Excellent. Maybe put with_bindgen into default-features, so it gets disabled with no-default-features?

[xoloki]

Excellent. Maybe put with_bindgen into default-features, so it gets disabled with no-default-features?

Actually, do whatever will make it easiest for us to have wsts by default build p256k1 with bindgen, but make it so you can disable bindgen on p256k1 via some simple to implement flag on wsts. cargo gets kinda tricky on this sometimes and I've painted myself into a corner before :P

[jcnelson]

How's this? https://github.com/Trust-Machines/wsts/pull/25

[xoloki]

How's this? Trust-Machines/wsts#25

Ok, I figured out how to get what I want, which is

1. p256k1 by default to use bindgen, so everyone gets a bindings.rs tailored to their environment
2. wsts by default to use p256k1 with bindgen for reason 1
3. make it easy to turn off bindgen for both

So in your p256k1 PR, make the following change:

diff --git a/p256k1/Cargo.toml b/p256k1/Cargo.toml
index 9ac8732..40ad497 100644
--- a/p256k1/Cargo.toml
+++ b/p256k1/Cargo.toml
@@ -13,7 +13,7 @@ categories = ["api-bindings", "cryptography", "mathematics"]
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

 [features]
-default = []
+default = ["with_bindgen"]
 with_bindgen = ["dep:bindgen"]

 [dependencies]

In your wsts PR, do this instead of what you currently have:

diff --git a/Cargo.toml b/Cargo.toml
index bb42816..09a2e13 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -12,6 +12,10 @@ categories = ["cryptography"]

 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

+[features]
+default = ["with_p256k1_bindgen"]
+with_p256k1_bindgen = ["p256k1/with_bindgen"]
+
 [dependencies]
 aes-gcm = "0.10"
 bs58 = "0.5"
@@ -21,7 +25,7 @@ num-traits = "0.2"
 polynomial = { version = "0.2.5", features = ["serde"] }
 primitive-types = "0.12"
 rand_core = "0.6"
+p256k1 = { version = "5.5", default-features = false }
 serde = { version = "1.0", features = ["derive"] }
 sha2 = "0.10"
 thiserror = "1.0"

After this, anyone building p256k1 without default features uses your musl compatible bindings.rs, and anyone building wsts without default features gets the same p256k1. So in stacks-signer we just turn off default features for p256k1 and wsts and get something that works with Alpine, without changing the default behavior for people with glibc.

Does that make sense? Sorry to be so obnoxious about this XD

[jcnelson]

Okay, both this PR and the wsts PR are updated as such.

Sorry to be so obnoxious about this XD

Not at all! You're the maintainer of these packages; I'm happy to jump through whatever hoops are needed to make changes that you're willing to maintain :)

[jcnelson]

Seems codecov has decided to barf on wsts