supranational / blst

Multilingual BLS12-381 signature library
Apache License 2.0
454 stars 171 forks source link

Rust bindings reference non-existent "std" feature #186

Closed DaniPopes closed 8 months ago

DaniPopes commented 11 months ago

lib.rs:

https://github.com/supranational/blst/blob/78fee18b25e16975e27b2d0314f6a323a23e6e83/bindings/rust/src/lib.rs#L5

But no std feature is declared in Cargo.toml.

dot-asm commented 11 months ago

And the problem is...

But on a serious note, Cargo.toml is not the only way to engage a feature, and the referred line is a conscious choice in the context. If you want to complain that you can't control it, then you have to recognize that you couldn't do so in the previous version either. In other words, there are no observable changes for current users. That's the point. But there is new possibility to compile blst for bare-metal platforms for new users.

DaniPopes commented 11 months ago

Sorry, I don't quite understand? It's not possible to activate this feature when importing the crate as a dependency under other projects using Cargo, such as c-kzg-4844, because it is not declared in Cargo.toml, and for Cargo this means the feature does not exist. That cfg line can never be enabled from other Cargo projects.

dot-asm commented 11 months ago

Again, there are no observable changes for current users. In other words, if you had a reference to blst 0.3.10, then no modifications shall be needed upon update, new version shall keep working exactly as it was before. If you're disputing this assertion, then tell what is it that stopped working or changed behaviour.

DaniPopes commented 1 month ago

I don't know what you're trying to say to be honest.

If you run cargo check (currently only on a beta/nightly build), which enables --check-cfg by default, it will display several warnings stating that the feature std is not declared, which is what my issue is about:

warning: unexpected `cfg` condition value: `std`
 --> src/lib.rs:5:17
  |
5 | #![cfg_attr(not(feature = "std"), no_std)]
  |                 ^^^^^^^^^^^^^^^
  |
  = note: expected values for `feature` are: `default`, `force-adx`, `no-threads`, `portable`, `serde`, and `serde-secret`
  = help: consider adding `std` as a feature in `Cargo.toml`
  = note: see <https://doc.rust-lang.org/nightly/rustc/check-cfg/cargo-specifics.html> for more information about checking conditional configuration
  = note: `#[warn(unexpected_cfgs)]` on by default

warning: unexpected `cfg` condition value: `std`
  --> src/lib.rs:20:7
   |
20 | #[cfg(feature = "std")]
   |       ^^^^^^^^^^^^^^^
   |
   = note: expected values for `feature` are: `default`, `force-adx`, `no-threads`, `portable`, `serde`, and `serde-secret`
   = help: consider adding `std` as a feature in `Cargo.toml`
   = note: see <https://doc.rust-lang.org/nightly/rustc/check-cfg/cargo-specifics.html> for more information about checking conditional configuration

...

This is simply fixed by adding [features] std = [] in Cargo.toml, as by convention cfg(feature = "xyz") is specified in Cargo.toml just like the other features the bindings crate provides.

I was just making this known to you, as this feature is completely unavailable for Cargo users if this is not done.

dot-asm commented 1 month ago

I don't know what you're trying to say to be honest.

The goal was to arrange it so that "before" and "after" looks exactly the same on before-supported platforms. There was no std option before and you still can't specify it after. And no matter what you do on the cargo command line, the outcome of the compilation on before-supported platform would be the same in respect to the feature in question even after.

this feature is completely unavailable for Cargo users if this is not done.

And this is intentional. It's not an option we provide, but a way to support ~for~ new, primarily "bare-metal," platforms.