Incorrect backend selection for non-standard targets

[ryankurte]

hi folks,

we're compiling for a non-standard (ie. out-of-tree) target which means target detection doesn't work, and setting '--cfg=curve25519_dalek_bits="32"' per the docs (via the environment or .cargo/config) does not have any effect, resulting in the error:

Error: Unknown Rust target platform.
Please set cfg(curve25519_dalek_bits) explicitly either as 32 or 64.

digging into the build.rs script it appears y'all are using cfg flags to switch features, however, when cross compiling these are not propagated to native components (ie. build scripts) at build time, only when they are executed (so in effect these are always ignored for foreign targets?).

via https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags:

If the --target flag (or build.target) is used, then the flags will only be passed to the compiler for the target. Things being built for the host, such as build scripts or proc macros, will not receive the args. Without --target, the flags will be passed to all compiler invocations (including build scripts and proc macros) because dependencies are shared. If you have args that you do not want to pass to build scripts or proc macros and are building for the host, pass --target with the host triple.

based on the intent here it appears this should instead be switching on environmental variables as these are passed when the build script is executed during compilation for the foreign target, so to have this behave as documented the fix would be something like:

diff --git a/build.rs b/build.rs
index a4c6b3e..f22df58 100644
--- a/build.rs
+++ b/build.rs
@@ -11,14 +11,17 @@ enum DalekBits {
 }

 fn main() {
-    #[cfg(curve25519_dalek_bits = "32")]
-    let curve25519_dalek_bits = DalekBits::Dalek32;
-
-    #[cfg(curve25519_dalek_bits = "64")]
-    let curve25519_dalek_bits = DalekBits::Dalek64;
-
-    #[cfg(all(not(curve25519_dalek_bits = "64"), not(curve25519_dalek_bits = "32")))]
-    let curve25519_dalek_bits = deterministic::determine_curve25519_dalek_bits();
+    let curve25519_dalek_bits = match std::env::var("CARGO_CFG_CURVE25519_DALEK_BITS").as_deref() {
+        Ok("32") => DalekBits::Dalek32,
+        Ok("64") => DalekBits::Dalek64,
+        _ => deterministic::determine_curve25519_dalek_bits(),
+    };

     match curve25519_dalek_bits {
         DalekBits::Dalek64 => println!("cargo:rustc-cfg=curve25519_dalek_bits=\"64\""),

happy to open a PR if y'all would be open to this fix?

[pinkforest]

Not an issue in this crate - it has to be fixed with / in the custom build tooling that does not signal cfg correctly to rustc.

Also cross compiling works with no issue and the current behaviour is correct vs incorrectly assuming custom targets.

This is a duplicate:

• Default curve25519_dalek_bits on non-standard Rust target / build tooling #511

I wonder which build tooling is being used that would not be able to set RUSTFLAGS ?

[pinkforest]

Looking at your repo's I'm assuming you are using a Makefile ?

You would need to add RUSTFLAGS='--cfg curve25519_dalek_bits="32" there given custom target if you wish to use u32.

Or in-line it in before the cargo command wrapped in the Makefile.

Or are you using cross with Cross.toml ?

https://github.com/cross-rs/cross#passing-environment-variables-into-the-build-environment

There is passthrough option which people usually use with RUSTFLAGS to relay it to rustc.

Would like to offer more help but dunno what custom build tooling is used.

[jrose-signal]

So there are two things happening here: @ryankurte is pointing out that build.rs will not see RUSTFLAGS when cross-compiling; this is correct. However, as @pinkforest notes, this has not been a problem for known targets, and that's because the cfg settings in RUSTFLAGS take priority over the ones from build.rs.* (If curve25519-dalek's build.rs did anything with the cfg options other than pass them down to the compiler, this would be a problem even for built-in targets.)

So what is the problem? Please set cfg(curve25519_dalek_bits) explicitly either as 32 or 64. This cannot be detected from build.rs when cross-compiling; it has to be checked in the main source. Or checked via something other than cfg, as @ryankurte suggests.

* Here's a test case; compare RUSTFLAGS='--cfg foo="32"' cargo run with RUSTFLAGS='--cfg foo="32"' cargo run --target $YOUR_HOST_TRIPLE.

[tarcieri]

It seems like we've had enough issues around this we should just pick a default for unknown platforms (and possibly print a warning?)

[ryankurte]

as @pinkforest notes, this has not been a problem for known targets, and that's because the cfg settings in RUSTFLAGS take priority over the ones from build.rs.*

i am pretty sure this is actually because when cross compiling the flag is always ignored but determine_curve25519_dalek_bits() works for targets that're built-in.

i have also confirmed that this occurs whether RUSTFLAGS are set via environmental variable or .cargo/config.toml (where idk.json is from rustc +nightly -Z unstable-options --print target-spec-json --target thumbv7m-none-eabi):

 RUSTFLAGS='--cfg curve25519_dalek_bits="32"' cargo build --target ./idk.json
   Compiling rustc-std-workspace-core v1.99.0 (/home/ryan/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/rustc-std-workspace-core)
   Compiling curve25519-dalek v4.0.0-rc.1
error: failed to run custom build command for `curve25519-dalek v4.0.0-rc.1`

Caused by:
  process didn't exit successfully: `/home/ryan/projects/experiments/something/target/debug/build/curve25519-dalek-f762beaf00f2b7e7/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /home/ryan/.cargo/registry/src/github.com-1ecc6299db9ec823/curve25519-dalek-4.0.0-rc.1/build.rs:17:63
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...

so if you don't get it propagated you panic because of missing backend selection, and if you do manage to set it you panic because of the unwrap on failed detection.

@pinkforest nothing custom in the build, repro here with RUSTFLAGS set via makefile, though i have been trying to use .cargo/config so it just works for cargo commands dalek-repro.zip

It seems like we've had enough issues around this we should just pick a default for unknown platforms (and possibly print a warning?)

a default is a great idea! but even when you get a warning the current mechanisms for backend selection do not work as intended or documented which seems worth a fix?

via https://docs.rs/curve25519-dalek/4.0.0-rc.1/curve25519_dalek/index.html#word-size-for-serial-backends:

Backend word size can be overridden for [default] and fiat by setting the environment variable:
RUSTFLAGS='--cfg curve25519_dalek_bits="SIZE"'
where SIZE is 32 or 64. As in the above section, this can also be placed in ~/.cargo/config.

[pinkforest]

Please use existing issue:

• Default curve25519_dalek_bits on non-standard Rust target / build tooling #511

determine function will not run if rustc would see --cfg to correctly and will not generate any code for it.

Having a default target for custom targets has issues and I've outlined them in the existing issue above.

I'll comment on the existing issue - this may be a rustc bug with custom targets.

[ryankurte]

I suspect this may be a cargo / rustc bug - I have a feeling it doesn't allow passing cfg with custom targets.

could very well be, there seem to be a lot of assumptions around about custom targets and target naming ^_^

[pinkforest]

Problem is if we now make default curve25519_dalek_bits default as 32 because selecting it does not work.

Then people also will have problems selecting backend so really need to get to bottom of this.

Another way to address this could be to register these custom targets in platforms crate if they are widely used.

[pinkforest]

What if we expose an environment variable for building for custom targets to set these ?

This would work/around the rustc issue not setting cfg correctly - but it will be in addition to existing cfg flags.

[ryankurte]

Then people also will have problems selecting backend so really need to get to bottom of this.

yeah if there's a warning / prompt / documentation about setting backends i think you should probably, be able to set backends.

What if we expose an environment variable for building for custom targets to set these ?

i am not sure i understand the opposition to fixing the incorrect behaviour of build.rs as i proposed, which would make the existing configuration mechanisms work as documented?

[pinkforest]

I am trying to get to bottom of this since this also means people are not able to set these configuration settings.

It means nobody is being able to set backend either which is a big issue beyond this.

build.rs works fine otherwise - the issue is with cargo / rustc - we are effectively work/arounding over existing bug there.

[jrose-signal]

This is not the same issue as #511. The problem here is that using Cargo and using RUSTFLAGS cross-compiling for an unsupported platform will fail, because build.rs has an explicit panic in it.

EDIT: (softening) My impression of #511 is a problem with "build systems that don't support build.rs setting cfg flags"

[pinkforest]

because build.rs has an explicit panic in it.

It doesn't have that panic anymore - please use the main branch version.

For cross did you use the Cross.toml to passthrough RUSTFLAGS ?

I have no idea what is used for cross other than either cross or --target (works out of the box with standard targets)
https://github.com/cross-rs/cross#passing-environment-variables-into-the-build-environment

We have two issues - 1) environment issues where people don't passthrough RUSTFLAGS and 2) custom targets #511

[jrose-signal]

It doesn't have that panic anymore

@ryankurte Maybe that's enough to fix your issue? You're just using Cargo, right?

[pinkforest]

@ryankurte Maybe that's enough to fix your issue? You're just using Cargo, right?

This wouldn't since using custom target involves a bug in rustc / cargo where rustc does not set cfg correctly it seems.

[ryankurte]

It doesn't have that panic anymore - please use the main branch version.

no, this is still broken because of the cfg vs env problem, you just get the Please set cfg(curve25519_dalek_bits) explicitly either as 32 or 64. error instead.

It means nobody is being able to set backend either which is a big issue beyond this.

yes, per my first post, manual backend selection for foreign targets is always ignored, which can be fixed by correctly using env instead of cfg in build.rs.

This wouldn't since using custom target involves a bug in rustc / cargo where rustc does not set cfg correctly it seems.

it may be a bug where RUSTFLAGS from the env are not passed to build.rs at build time, only runtime, but this is consistent with the rust documentation and flags specified via .cargo/config.toml, so i think likely the desired behaviour.

[pinkforest]

Using environment variables is not a standard way to configure builds in Rust.

Standard way is to use cfg - there is nothing incorrect about it.

We will allow extra environment variables but cfg will take priority.

[ryankurte]

Standard way is to use cfg - there is nothing incorrect about it.

https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags

If the --target flag (or build.target) is used, then the flags will only be passed to the compiler for the target. Things being built for the host, such as build scripts or proc macros, will not receive the args

cfg is a build time selector, build.rs and proc macros are compiled -before- build and executed -at- build time, if you want build-time flags to be reflected in build.rs they need to be read at execution.

[pinkforest]

Build target cfg flags are not the same as curve25519_dalek cfg flags we've specified.

cfg curve25519_dalek_bits and backend flags work correctly in standard targets with this build.rs just fine.

[ryankurte]

cfg curve25519_dalek_bits and backend flags work correctly in standard targets with this build.rs just fine.

you can test this by patching out determine_curve25519_dalek_bits with a panic in build.rs

diff --git a/build.rs b/build.rs
index a4c6b3e..6944276 100644
--- a/build.rs
+++ b/build.rs
@@ -18,7 +18,7 @@ fn main() {
     let curve25519_dalek_bits = DalekBits::Dalek64;

     #[cfg(all(not(curve25519_dalek_bits = "64"), not(curve25519_dalek_bits = "32")))]
-    let curve25519_dalek_bits = deterministic::determine_curve25519_dalek_bits();
+    let curve25519_dalek_bits = panic!("whoops");

     match curve25519_dalek_bits {
         DalekBits::Dalek64 => println!("cargo:rustc-cfg=curve25519_dalek_bits=\"64\""),

if the overrides were working for foreign in-tree targets this should not be used or executed, and compilation with RUSTFLAGS='--cfg curve25519_dalek_bits="32"' cargo build --target thumbv7m-none-eabi should succeed, this is not the case.

[pinkforest]

The overrides work with my cargo / rustc - by deriving Debug to DalekBits:

    #[cfg(curve25519_dalek_bits = "32")]
    let curve25519_dalek_bits = DalekBits::Dalek32;

    #[cfg(curve25519_dalek_bits = "64")]
    let curve25519_dalek_bits = DalekBits::Dalek64;

    panic!("{}", curve25519_dalek_bits);

$ env RUSTFLAGS='--cfg curve25519_dalek_bits="32"' cargo build

   Compiling curve25519-dalek v4.0.0-rc.1 (/home/foobar/cc/curve25519-dalek)

warning: `curve25519-dalek` (build script) generated 1 warning
error: failed to run custom build command for `curve25519-dalek v4.0.0-rc.1 (/home/foobar/cc/curve25519-dalek)`

Caused by:
  process didn't exit successfully: `/home/foobar/cc/curve25519-dalek/target/debug/build/curve25519-dalek-8f29bc9a97f12ab9/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'Dalek32', build.rs:21:5
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
#

Setting to 64 gives me Dalek64 as expected - it doesn't matter if I put the panic after deterministic.

This is on otherwise 64bit target_pointer_width default target.

What is the cargo / rustc version you are using ? I'm using various and the behaviour is same.

[ryankurte]

env RUSTFLAGS='--cfg curve25519_dalek_bits="32"' cargo build

you seem to be compiling natively there, where RUSTFLAGS behaviour only changes for foreign targets? try with --target thumbv7m-none-eabi so you're cross compiling

[pinkforest]

$ env RUSTFLAGS='--cfg curve25519_dalek_bits="64"' cargo build --target wasm32-wasi

  --- stderr
  Dalek32

This is incorrect indeed - I thought I had this tested with wasm32-u-u sigh.

Now let's see how we address this - as we need to address those configuration failures as well for custom targets.

[ryankurte]

Now let's see how we address this.

the fix i proposed in the first post works for in-tree and custom foreign targets

[pinkforest]

Yeah we'll use environment variable if the cfg is not present but we'll use curve25519_dalek_ env outside cargo_cfg

Need to check if the backend via curve25519_dalek_backend behaves the same way without build script.

EDIT: Oh yeah we can use CARGO_CFG_ indeed 👍

[pinkforest]

@ryankurte See #516 - I've added you as Co-authored-by given your fix