Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 10 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3cb80db
Add derive_compiled feature to force full compilation of derive
sagudev Aug 19, 2023
b12293e
Add cfg opt-in for the precompiled binary
pinkforest Aug 19, 2023
8d54439
Remove redundant features
pinkforest Aug 19, 2023
2e5bba0
Use opt-out instead and provide doc
pinkforest Aug 20, 2023
10a0210
Add simplified gating via build.rs
pinkforest Aug 20, 2023
81e79c1
Provide the opt-out gate without build.rs
pinkforest Aug 20, 2023
6bccee7
Fix the manifest gate for opt-out - thanks @sagudev
pinkforest Aug 20, 2023
5854820
Provide opt-out source compilation by feature `from_source
pinkforest Aug 20, 2023
23059f7
Provide another opt-out alternative w/ nightly with cross-compile
pinkforest Aug 20, 2023
cb23de9
Switch back to opt-out `feature=from_source` override.
pinkforest Aug 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ fn main() {
}
```

## Note about serde_derive Binary
Copy link
Contributor Author

@pinkforest pinkforest Aug 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

[host]
rustflags = [...]

Copy link
Contributor Author

@pinkforest pinkforest Aug 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for trying it out

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@pinkforest pinkforest Aug 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link

@Nemo157 Nemo157 Aug 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) (*)


To force building `serde_derive` from source, an override is provided via
feature `from_source` if the target environment requires it.

## Getting help

Serde is one of the most widely used Rust libraries so any place that Rustaceans
Expand Down
8 changes: 8 additions & 0 deletions precompiled/serde_derive/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,18 @@ rust-version = "1.56"
[features]
default = []
deserialize_in_place = []
from_source = ["proc-macro2", "quote", "syn"]

[lib]
proc-macro = true

# By feature source compilation
[dependencies]
proc-macro2 = { version = "1", optional = true }
quote = { version = "1", optional = true }
syn = { version = "2.0.28", optional = true }

# Compile from source when no precompiled binary available
[target.'cfg(not(all(target_arch = "x86_64", target_os = "linux", target_env = "gnu")))'.dependencies]
proc-macro2 = "1"
quote = "1"
Expand Down
12 changes: 10 additions & 2 deletions precompiled/serde_derive/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,16 @@

#![doc(html_root_url = "https://docs.rs/serde_derive/1.0.183")]

#[cfg(not(all(target_arch = "x86_64", target_os = "linux", target_env = "gnu")))]
#[cfg(any(
feature = "from_source",
not(all(target_arch = "x86_64", target_os = "linux", target_env = "gnu"))
))]
include!("lib_from_source.rs");

#[cfg(all(target_arch = "x86_64", target_os = "linux", target_env = "gnu"))]
#[cfg(all(
not(feature = "from_source"),
target_arch = "x86_64",
target_os = "linux",
target_env = "gnu"
))]
include!("lib_precompiled.rs");
3 changes: 3 additions & 0 deletions serde/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ default = ["std"]
# Provide derive(Serialize, Deserialize) macros.
derive = ["serde_derive"]

# Force compilation from source - currently serde_derive
from_source = ["serde_derive/from_source"]

# Provide impls for common standard library types like Vec<T> and HashMap<K, V>.
# Requires a dependency on the Rust standard library.
std = []
Expand Down
1 change: 1 addition & 0 deletions serde_derive/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ rust-version = "1.56"

[features]
default = []
from_source = []
deserialize_in_place = []

[lib]
Expand Down