Provide opt-out `feature=from_source` to use `serde_derive` source #2580

pinkforest · 2023-08-19T13:27:54Z

Closes #2538
Supercedes #2579

Follows my recommendation: #2579 (comment)

Current choice: Opt-Out by feature=from_source (current HEAD commit)

Triggers build from source if used with feature from_source via either serde or serde_derive

Alternative choice: Opt-Out by cfg(serde_derive_build = "source") (commit #6bccee)

Override via cfg(serde_derive_build = "source") to build from source

Has flaw re: cross compile as per @bjorn3 at #2580 (comment) due to Cargo limitation - but can be addressed via adocumentation as has been done via commit #23059f7

Alterative: Opt-In - My recommendation (commit #8d544)

Would make opt-in choice to use the precompiled binary with cfg(serde_derive_build = "precompiled") and leaving it to the top-level binary to use the precompiled blob if it explicitly opts in to - by default people will compile from source always.

cfg Approach

This also demonstrates the cfg based approach that frees dependency chain from messing up with the features in the middle leaving it to the top level binary as informed choice.

This is used elsewhere e.g.: https://github.com/dalek-cryptography/curve25519-dalek/tree/main/curve25519-dalek#manual-backend-override - that was result of this discussion: dalek-cryptography/curve25519-dalek#414 providing background why / how this approach has been used elsewhere.

Cc/ @sagudev I cherry picked

simonsan · 2023-08-19T13:42:35Z

How would I specify that I don't want the precompiled binary when I only pull in serde with the serde_derive feature like this?

serde = { version = "=1.0.171", features = ["serde_derive"] }

pinkforest · 2023-08-19T13:48:50Z

@simonsan

Please note that this PR proposes to make it opt-in only where the top-level binary would need to want the precompiled

By default you would get it built from the source if this PR is merged and leaves it to the top level bin to opt-in the binary.

If the maintainer wishes this to be opt-out instead then could always add a feature that signals the cfg override

EDIT: I further clarified at top - thanks for the q.

kayabaNerve · 2023-08-19T15:32:11Z

I will repeat my comment from #2579.

I don't believe this adequately resolves #2538. This would still cause unknown, unsigned, and unverifiable executables to placed on any systems which use serde as a dependency. That's still a notable security risk.

While I'm obviously in favor of this, as it's a massive improvement, this still leaves an unverifiable executable sitting on the system.

I'd state the binary should only be downloaded on opt-in.

EDIT: What about a serde-derive-bin which has the binary, yet is optional and only downloaded under this cfg? That wouldn't reduce security from the current status quo, and would prevent anyone who doesn't opt-in from needing to download the bin.

kayabaNerve · 2023-08-19T16:19:53Z

@pinkforest While I agree anyone being able to include any file is a distinct issue, it being in a separate crate which isn't actually downloaded would solve that. I do see a comment claiming optional dependencies, even if unused, will appear in the crate graph and be downloaded though, which may void my comment's validity.

pinkforest · 2023-08-19T16:32:48Z

@oli-obk ok - I will add another commit to opt-out - can always re-base to previous commit if opt-in is chosen instead so choice will be with maintainers - I will also add doc how to use it - thanks

pinkforest · 2023-08-19T16:52:58Z

New behaviour with cfg(serde_derive_build = "source")

Default - host x86_64-linux-gnu

$ cargo build
   Compiling serde_derive v1.0.183 (/home/foobar/code/serde/precompiled/serde_derive)
    Finished dev [unoptimized + debuginfo] target(s) in 0.16s

Override - host x86_64-linux-gnu

$ env RUSTFLAGS='--cfg serde_derive_build="source"' cargo build
   Compiling proc-macro2 v1.0.66
   Compiling unicode-ident v1.0.11
   Compiling quote v1.0.33
   Compiling syn v2.0.29
   Compiling serde_derive v1.0.183 (/home/foobar/code/serde/precompiled/serde_derive)
    Finished dev [unoptimized + debuginfo] target(s) in 3.17s

newpavlov · 2023-08-19T16:55:13Z

@simonsan

How would I specify that I don't want the precompiled binary when I only pull in serde with the serde_derive feature like this?

The most flexible option is to use:

serde = { version = "1.0.x, <1.0.172", features = ["derive"] }

where x is minimum version required by your project.

@oli-obk

But it doesn't actually change too much, as afaik it will still be downloaded, just never built. Or at least show up in a Cargo.lock

IIRC unused crates do not get downloaded during compilation, but, yes, they get mentioned in Cargo.lock.

From my experience, having a separate crate for pre-compiled version of the crate will be quite useful when dependencies get vendored. Even though it will be mentioned in Cargo.lock, we will not have a semi-random blob as part of our vendored source code.

bjorn3 · 2023-08-19T17:47:36Z

It is really easy to accidentally override the rustflags and thus opt back into the precompiled version. RUSTFLAGS will override ~/.cargo/config.toml and a per-project .cargo/config.toml will override the global one. As such putting the opt-out once in your ~/.cargo/config.toml may accidentally use the precompiled binary again if the project you build uses .cargo/config.toml or you or the project needs RUSTFLAGS (eg in rust's build system)

Edit: Github was borked so I thought my comment was lost so I wrote it again. After I noticed that it wasn't gone I deleted my second comment.

README.md

precompiled/serde_derive/src/lib.rs

precompiled/serde_derive/build.rs

precompiled/serde_derive/Cargo.toml

bjorn3 · 2023-08-20T11:23:09Z

If serde_derive is used for a build dependency afaik with the v2 resolver it wouldn't get feature unified with serde_derive used by a non-build dependency or the root crate.

pinkforest · 2023-08-20T11:48:01Z

README.md

@@ -73,6 +73,11 @@ fn main() {
 }
 ```

+## Note about serde_derive Binary


@oli-obk Sadly with either CARGO_ENCODED_RUSTFLAGS or given these .cargo/config neither work 😭

build.target

[build] target = "wasm32-unknown-unknown" [target.wasm32-unknown-unknown] rustflags = ['--cfg=serde_derive_build="source"']

build.rustflags

[build] rustflags = ['--cfg=serde_derive_build="source"']

Both set the options but it doesn't get seen at serde_derive procmacro level still

To get rustflags into build script builds you need to use -Zhost-config and

[host] rustflags = [...]

Oh we have a winner! Thanks @Nemo157 ❤️

$ cargo -Zhost-config build error: the -Zhost-config flag requires the -Ztarget-applies-to-host flag to be set [foobar@localhost whatever]$ cargo -Ztarget-applies-to-host -Zhost-config build Compiling proc-macro2 v1.0.66 Compiling unicode-ident v1.0.11 Compiling whatever v0.1.0 (/home/foobar/code/whatever) warning: "x86_64-unknown-linux-gnu" Compiling quote v1.0.33 Compiling syn v2.0.29 Compiling serde_derive v1.0.183 (/home/foobar/code/pushpush/serde/precompiled/serde_derive) Finished dev [unoptimized + debuginfo] target(s) in 2.84s [foobar@localhost whatever]$ cargo -Ztarget-applies-to-host -Zhost-config build --target wasm32-unknown-unknown Compiling proc-macro2 v1.0.66 Compiling unicode-ident v1.0.11 Compiling whatever v0.1.0 (/home/foobar/code/whatever) warning: "wasm32-unknown-unknown" Compiling quote v1.0.33 Compiling syn v2.0.29 Compiling serde_derive v1.0.183 (/home/foobar/code/pushpush/serde/precompiled/serde_derive) Finished dev [unoptimized + debuginfo] target(s) in 3.43s [foobar@localhost whatever]$ cat .cargo/config [host] rustflags = ['--cfg=serde_derive_build="source"']

When were these -Z introduced ? 🤔 so we can document it as required version if using this opt-out ?

I think using an unstable option for the opt-out is wrong. Also it still has the same issue of getting overriden by other ways to specify rustflags rather than appending to it.

Thanks for trying it out

It's fragile (or rather: easy to get wrong). Is there some solution to the resolver=2 problem?

Good question need to build a test-case for it and prove it works I'll do that shortly.

EDIT: Yeah so I've replicated the resolver problem. Maybe we can just document this ?
This was the repro for it: https://github.com/pinkforest/serde_optout_unification_proof

Alright with me. Is there something that can be done to ensure build dependencies also have the feature enabled?

It seems I didn't clear the target properly between my repro previously

and it seems I can't replicate the problem with feature unification

e.g. I don't see anything wrong with the feature approach ?

That's because it's a proc-macro crate, which is its own kind of dependency similar to a build-dependency, which is built for a single target: the host; so both the build-dep and normal-dep refer to the same proc-macro-dependency and their features get unified in it. A more accurate cargo tree would look like

top-bin v0.1.0 (/tmp/tmp.mNq6xeO1Ms/serde_optout_unification_proof/top-bin) └── some-lib v0.1.0 (/tmp/tmp.mNq6xeO1Ms/serde_optout_unification_proof/some-lib) [proc-macro-dependencies] └── serde_derive v1.0.183 (proc-macro) (https://github.com/serde-rs/serde?rev=cb23de92e7f479139add329e8629eb3b179109f6#cb23de92) ├── proc-macro2 v1.0.66 │ └── unicode-ident v1.0.11 ├── quote v1.0.33 │ └── proc-macro2 v1.0.66 (*) └── syn v2.0.29 ├── proc-macro2 v1.0.66 (*) ├── quote v1.0.33 (*) └── unicode-ident v1.0.11 [build-dependencies] └── some-build-lib v0.1.0 (/tmp/tmp.mNq6xeO1Ms/serde_optout_unification_proof/some-build-lib) [proc-macro-dependencies] └── serde_derive v1.0.183 (proc-macro) (https://github.com/serde-rs/serde?rev=cb23de92e7f479139add329e8629eb3b179109f6#cb23de92) (*)

EDIT: And by switching both dependencies to serde = { features = ["derive"] } instead of being direct you can see that there are two versions of serde built (because no (*)), one for host one for target, that both link against the same serde_derive (because (*)):

> cargo tree --features=from_source warning: skipping duplicate package `serde_derive` found at `/home/nemo157/.cargo/git/checkouts/serde-1cbd136c2e06ab68/cb23de9/precompiled/serde_derive` top-bin v0.1.0 (/tmp/tmp.mNq6xeO1Ms/serde_optout_unification_proof/top-bin) └── some-lib v0.1.0 (/tmp/tmp.mNq6xeO1Ms/serde_optout_unification_proof/some-lib) └── serde v1.0.183 (https://github.com/serde-rs/serde?rev=cb23de92e7f479139add329e8629eb3b179109f6#cb23de92) [proc-macro-dependencies] └── serde_derive v1.0.183 (proc-macro) (https://github.com/serde-rs/serde?rev=cb23de92e7f479139add329e8629eb3b179109f6#cb23de92) ├── proc-macro2 v1.0.66 │ └── unicode-ident v1.0.11 ├── quote v1.0.33 │ └── proc-macro2 v1.0.66 (*) └── syn v2.0.29 ├── proc-macro2 v1.0.66 (*) ├── quote v1.0.33 (*) └── unicode-ident v1.0.11 [build-dependencies] └── some-build-lib v0.1.0 (/tmp/tmp.mNq6xeO1Ms/serde_optout_unification_proof/some-build-lib) └── serde v1.0.183 (https://github.com/serde-rs/serde?rev=cb23de92e7f479139add329e8629eb3b179109f6#cb23de92) [proc-macro-dependencies] └── serde_derive v1.0.183 (proc-macro) (https://github.com/serde-rs/serde?rev=cb23de92e7f479139add329e8629eb3b179109f6#cb23de92) (*)

Co-authored-by: sagudev <16504129+sagudev@users.noreply.github.com> Co-authored-by: Oliver Schneider <oli-obk@users.noreply.github.com>

With stable in non-cross compile context is still possible: RUSTFLAGS='--cfg serde_derive_build="source"' Co-authored-by: Nemo157 <github@nemo157.com>

LuckyTurtleDev · 2023-08-20T14:12:16Z

Hey, initially an opt-in version was pitched. Is opt-in off the table now?

I personally would also prefer an opt-in solution. But if any crate you used has opt-in to use the binary, you can not disable it anymore. So opt-out would probably the better solution.

bjorn3 · 2023-08-20T14:21:03Z

Opt-in through RUSTFLAGS doesn't have that problem.

pinkforest · 2023-08-20T14:42:38Z

@bjorn3 fwiw - I just tested parked at commit #585482 the resolver="2" and it seems to work regardless ?

Repro here: https://github.com/pinkforest/serde_optout_unification_proof

Source by opt-out

top-bin]$ rm -rf ../target ; cargo build --features from_source
   Compiling proc-macro2 v1.0.66
   Compiling unicode-ident v1.0.11
   Compiling quote v1.0.33
   Compiling syn v2.0.29
   Compiling serde_derive v1.0.183 (/home/foobar/code/serde/precompiled/serde_derive)
   Compiling some-lib v0.1.0 (/home/foobar/code/serde_optout_unification_proof/some-lib)
   Compiling top-bin v0.1.0 (/home/foobar/code/serde_optout_unification_proof/top-bin)
    Finished dev [unoptimized + debuginfo] target(s) in 2.97s

Default precompiled

top-bin]$ rm -rf ../target ; cargo build
   Compiling serde_derive v1.0.183 (/home/foobar/code/serde/precompiled/serde_derive)
   Compiling some-lib v0.1.0 (/home/foobar/code/serde_optout_unification_proof/some-lib)
   Compiling top-bin v0.1.0 (/home/foobar/code/serde_optout_unification_proof/top-bin)
    Finished dev [unoptimized + debuginfo] target(s) in 0.33s

Anything I did wrong here to not reproduce the given problem with resolver = "2" ?

My preference certainly would be opt-in as well but will see what the decision here is given options provided.

This can be useful in target environments which require building from the source. The feature flag is provided both via serde_derive and serde. Co-authored-by: sagudev <16504129+sagudev@users.noreply.github.com> Co-authored-by: Oliver Schneider <oli-obk@users.noreply.github.com>

AlexTMjugador · 2023-08-20T16:46:21Z

I'd like to mention that the decision of using precompiled binaries for Linux x64 platforms is already causing significant ecosystem fragmentation. That might be useful to keep in mind for reviewing this PR: time-rs/time#611

jnicholls · 2023-08-20T18:18:39Z

IMHO Opt-in for the precompiled binary should be the desired outcome. It is backwards compatible with the ecosystem's current serde dependency specifications, and its expectations before learning about the precompiled binary.

GoWeasel · 2023-08-20T18:30:30Z

As followed this issue, just mention to care about upcoming cyber resillience act,
only/or by default just binary would drop out serde of projects.

bjorn3 · 2023-08-20T18:47:14Z

@bjorn3 fwiw - I just tested parked at commit #585482 the resolver="2" and it seems to work regardless ?

Maybe it only applies to non-proc-macro deps? In any case great that it works.

joshka · 2023-08-20T18:47:15Z

@AlexTMjugador wrote:

I'd like to mention that the decision of using precompiled binaries for Linux x64 platforms is already causing significant ecosystem fragmentation. That might be useful to keep in mind for reviewing this PR: time-rs/time#611

Regarding this - do you think it would be worth putting forward a PR that fully reverts the precompiled PR until this or #2579 land (or whatever is the real fix)? To me that approach seems like it will give some breathing room to work out the correct fix.

AlexTMjugador · 2023-08-20T18:59:33Z

Regarding this - do you think it would be worth putting forward a PR that fully reverts the precompiled PR until this or #2579 land (or whatever is the real fix)? To me that approach seems like it will give some breathing room to work out the correct fix.

I'd support that, but I'm not a serde repository maintainer to decide.

joshka · 2023-08-20T19:02:20Z

I'd support that, but I'm not a serde repository maintainer to decide.

Yep - I understand that. I read your reply over in the time issue and figured you might have a reasonable perspective on this one way or another.

faulesocke · 2023-08-20T19:14:30Z

from_source should really be on by default, making it not include the binary in the default feature set.

dtolnay

This code is no longer used since serde_derive 1.0.184 — would you be interested in sending a different PR to delete it? The precompiled directory and anything having to do with cfg(precompiled) in the rest of the code.

Thanks, I appreciate your diligence on this PR, and the dozens of commenters who helped brainstorm the possible approaches for an opt-out and opt-in implementation.

pinkforest · 2023-08-21T03:07:50Z

Thanks will do - appreciate looking over - I can just override this existing branch in this PR no problem

dtolnay · 2023-08-21T03:09:02Z

Nice. I'd prefer a new PR to preserve the final working approach that was settled on in this one for opt-out.

pinkforest · 2023-08-21T03:09:30Z

Ok great valid point I agree ❤️

This PR phases out the precompiled per final consensus made in serde-rs#2580 i# Fix a cfg

* Release 1.0.184 * Following consensus on: serde-rs#2580 (review) This PR phases out the precompiled per final consensus made in serde-rs#2580 i# Fix a cfg * No need for slow macOS CI if there is no platform-specific code * Add repro of issue 2591 error[E0507]: cannot move out of `*__self` which is behind a shared reference --> test_suite/tests/test_remote.rs:210:10 | 210 | #[derive(Serialize, Deserialize)] | ^^^^^^^^^ | | | data moved here | move occurs because `unrecognized` has type `ErrorKind`, which does not implement the `Copy` trait | = note: this error originates in the derive macro `Serialize` (in Nightly builds, run with -Z macro-backtrace for more info) help: consider borrowing here | 210 | #[derive(&Serialize, Deserialize)] | + * Fix "cannot move out of `*self` which is behind a shared reference" * Release 1.0.185 --------- Co-authored-by: David Tolnay <dtolnay@gmail.com> Co-authored-by: pinkforest <36498018+pinkforest@users.noreply.github.com>

Add derive_compiled feature to force full compilation of derive

3cb80db

pinkforest changed the title ~~Cfg override~~ Provide opt-in cfg to use precompiled blob Aug 19, 2023

This was referenced Aug 19, 2023

Add derive_compiled feature to force compilation of serde_derive #2579

Closed

using serde_derive without precompiled binary #2538

Closed

This comment was marked as resolved.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as resolved.

Sign in to view

This comment was marked as off-topic.

Sign in to view

pinkforest changed the title ~~Provide opt-in cfg to use precompiled blob~~ Provide opt-out cfg to use serde_derive source Aug 19, 2023

alerque reviewed Aug 19, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

pinkforest added 3 commits August 20, 2023 09:21

Add cfg opt-in for the precompiled binary

b12293e

Remove redundant features

8d54439

Use opt-out instead and provide doc

2e5bba0

schoenenberg reviewed Aug 20, 2023

View reviewed changes

precompiled/serde_derive/src/lib.rs Outdated Show resolved Hide resolved

Add simplified gating via build.rs

10a0210

oli-obk reviewed Aug 20, 2023

View reviewed changes

precompiled/serde_derive/build.rs Outdated Show resolved Hide resolved

Provide the opt-out gate without build.rs

81e79c1

sagudev reviewed Aug 20, 2023

View reviewed changes

precompiled/serde_derive/Cargo.toml Outdated Show resolved Hide resolved

pinkforest commented Aug 20, 2023

View reviewed changes

Provide opt-out source compilation by feature `from_source

5854820

Co-authored-by: sagudev <16504129+sagudev@users.noreply.github.com> Co-authored-by: Oliver Schneider <oli-obk@users.noreply.github.com>

pinkforest force-pushed the cfg-override branch from ddfa22b to 5854820 Compare August 20, 2023 11:55

This comment was marked as resolved.

Sign in to view

Provide another opt-out alternative w/ nightly with cross-compile

23059f7

With stable in non-cross compile context is still possible: RUSTFLAGS='--cfg serde_derive_build="source"' Co-authored-by: Nemo157 <github@nemo157.com>

Stebalien mentioned this pull request Aug 20, 2023

Force serde-derive to build from source filecoin-project/ref-fvm#1851

Closed

andreisilviudragnea mentioned this pull request Aug 20, 2023

Blocked from serde org? React here andreisilviudragnea/serde-blocked#2

Open

mo8it mentioned this pull request Aug 20, 2023

Blocked from serde org? React here andreisilviudragnea/serde-blocked#3

Open

dtolnay requested changes Aug 21, 2023

View reviewed changes

dtolnay closed this Aug 21, 2023

pinkforest mentioned this pull request Aug 21, 2023

Phase out precompiled #2590

Merged

pinkforest added a commit to pinkforest/serde that referenced this pull request Aug 21, 2023

Following consensus on: serde-rs#2580 (review)

360606b

This PR phases out the precompiled per final consensus made in serde-rs#2580 i# Fix a cfg

Provide opt-out feature=from_source to use serde_derive source #2580

Provide opt-out feature=from_source to use serde_derive source #2580

Conversation

pinkforest commented Aug 19, 2023 • edited

simonsan commented Aug 19, 2023

pinkforest commented Aug 19, 2023 • edited

kayabaNerve commented Aug 19, 2023 • edited

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as off-topic.

This comment was marked as resolved.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

kayabaNerve commented Aug 19, 2023

This comment was marked as off-topic.

pinkforest commented Aug 19, 2023

pinkforest commented Aug 19, 2023

newpavlov commented Aug 19, 2023 • edited

bjorn3 commented Aug 19, 2023 • edited

bjorn3 commented Aug 20, 2023

pinkforest Aug 20, 2023 • edited

Choose a reason for hiding this comment

Nemo157 Aug 20, 2023

Choose a reason for hiding this comment

pinkforest Aug 20, 2023 • edited

Choose a reason for hiding this comment

bjorn3 Aug 20, 2023

Choose a reason for hiding this comment

oli-obk Aug 20, 2023

Choose a reason for hiding this comment

oli-obk Aug 20, 2023

Choose a reason for hiding this comment

pinkforest Aug 20, 2023 • edited

Choose a reason for hiding this comment

oli-obk Aug 20, 2023

Choose a reason for hiding this comment

pinkforest Aug 20, 2023

Choose a reason for hiding this comment

Nemo157 Aug 20, 2023 • edited

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

LuckyTurtleDev commented Aug 20, 2023

bjorn3 commented Aug 20, 2023

pinkforest commented Aug 20, 2023 • edited

AlexTMjugador commented Aug 20, 2023

jnicholls commented Aug 20, 2023

GoWeasel commented Aug 20, 2023

bjorn3 commented Aug 20, 2023

joshka commented Aug 20, 2023

AlexTMjugador commented Aug 20, 2023

joshka commented Aug 20, 2023

faulesocke commented Aug 20, 2023

dtolnay left a comment

Choose a reason for hiding this comment

pinkforest commented Aug 21, 2023

dtolnay commented Aug 21, 2023

pinkforest commented Aug 21, 2023 • edited

Provide opt-out `feature=from_source` to use `serde_derive` source #2580

Provide opt-out `feature=from_source` to use `serde_derive` source #2580

pinkforest commented Aug 19, 2023 •

edited

pinkforest commented Aug 19, 2023 •

edited

kayabaNerve commented Aug 19, 2023 •

edited

newpavlov commented Aug 19, 2023 •

edited

bjorn3 commented Aug 19, 2023 •

edited

pinkforest Aug 20, 2023 •

edited

pinkforest Aug 20, 2023 •

edited

pinkforest Aug 20, 2023 •

edited

Nemo157 Aug 20, 2023 •

edited

pinkforest commented Aug 20, 2023 •

edited

pinkforest commented Aug 21, 2023 •

edited