Replace feature serde with use-serde #373

elichai · 2019-12-23T10:28:49Z

This will fix problems like #372
Basically the problem is that cargo doesn't allow colliding package names with feature names, so we had to use the feature use-serde instead of serde (common practice)
But now some users can get confuse and pass serde by mistake which will make it miscompile.

I replaced all usages of feature = "serde" with feature = "use-serde". we could also add to the CI to test the "features" serde and hex such that it will still compile.
another option is to add a build.rs file that will test if serde is set without use-serde and will produce a meaningful error for the user.

Introduced in: #215

codecov-io · 2019-12-23T10:43:02Z

Codecov Report

❗ No coverage uploaded for pull request base (master@efd2168). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #373   +/-   ##
=========================================
  Coverage          ?   81.93%           
=========================================
  Files             ?       39           
  Lines             ?     7185           
  Branches          ?        0           
=========================================
  Hits              ?     5887           
  Misses            ?     1298           
  Partials          ?        0

Impacted Files	Coverage Δ
src/util/address.rs	`86.38% <ø> (ø)`
src/blockdata/opcodes.rs	`97.76% <ø> (ø)`
src/blockdata/transaction.rs	`93.89% <ø> (ø)`
src/util/key.rs	`72.77% <ø> (ø)`
src/util/bip32.rs	`87.14% <ø> (ø)`
src/blockdata/script.rs	`78.59% <ø> (ø)`
src/lib.rs	`100% <ø> (ø)`
src/util/amount.rs	`86.63% <ø> (ø)`
src/internal_macros.rs	`59.47% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efd2168...2df6eb8. Read the comment docs.

stevenroose · 2019-12-23T13:31:22Z

I don't like this. The feature-can't-collide-with-dep issue is clearly a Cargo issue, and we shouldn't be adapting our code for that.

Also this doesn't really solve anything. People accidentally using the serde feature will still get build errors and somehow have to find out that they should use use-serde, just like they do today.

A better mitigation could be a list of existing features in the README.

elichai · 2019-12-23T13:34:45Z

I don't like this. The feature-can't-collide-with-dep issue is clearly a Cargo issue, and we shouldn't be adapting our code for that.

Also this doesn't really solve anything. People accidentally using the serde feature will still get build errors and somehow have to find out that they should use use-serde, just like they do today.

A better mitigation could be a list of existing features in the README.

well the code is still flawed. it checks for a feature serde when we actually don't have such a feature. we have a feature called use-serde.

stevenroose · 2019-12-23T13:49:04Z

Sure, but that is because the only way to check of a dependency is present is through cfg(feature = "dep"). That's a rustc/cargo shortcoming. You are right that it should be cfg(dependency = "serde") or something.

Probably too late to change that..

apoelstra · 2019-12-23T18:00:26Z

We do have a feature serde (by dint of having an optional serde dependency). It just happens that it does not compile without additional features. Though as Steven says, this is a cargo-ism that probably was a historical mistake.

The existing approach tries to require the minimum amount of features for each part of the code.

elichai · 2020-01-08T16:16:47Z

Sure, but that is because the only way to check of a dependency is present is through cfg(feature = "dep"). That's a rustc/cargo shortcoming. You are right that it should be cfg(dependency = "serde") or something.

Probably too late to change that..

So do you think we should replace the current includes to this?

diff --git a/src/lib.rs b/src/lib.rs
index 6ba26f9..0cf16fd 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -56,11 +56,11 @@
 pub extern crate secp256k1;
 pub extern crate bech32;
 
-#[cfg(any(test, feature = "serde"))] extern crate hex;
+#[cfg(any(test, feature = "hex"))] extern crate hex;
 #[cfg(feature = "serde")] extern crate serde;
-#[cfg(all(test, feature = "serde"))] #[macro_use] extern crate serde_derive; // for 1.22.0 compat
-#[cfg(all(test, feature = "serde"))] extern crate serde_json;
-#[cfg(all(test, feature = "serde"))] extern crate serde_test;
+#[cfg(all(test, feature = "serde_derive"))] #[macro_use] extern crate serde_derive; // for 1.22.0 compat
+#[cfg(all(test, feature = "serde_json"))] extern crate serde_json;
+#[cfg(all(test, feature = "serde_test"))] extern crate serde_test;
 #[cfg(all(test, feature = "unstable"))] extern crate test;
 #[cfg(feature="bitcoinconsensus")] extern crate bitcoinconsensus;

dr-orlovsky · 2020-09-08T22:37:36Z

Good explanation on how to avoid this issue: rust-lang/api-guidelines#180

@Kixunil proposed a good solution which we use in LNP/BP library. It works the following way:

In Cargo.toml https://github.com/LNP-BP/rust-lnpbp/blob/master/Cargo.toml#L56

[dependencies]
serde_crate = { package = "serde", version = "~1.0.106", features = ["derive"], optional = true }

[features]
serde = ["serde_crate", "bitcoin/use-serde", "miniscript/serde"]

In lib.rs https://github.com/LNP-BP/rust-lnpbp/blob/master/src/lib.rs#L51-L53

#[cfg(feature = "serde")]
#[macro_use]
extern crate serde_crate as serde;

In crates using underlying one as dependency, just simply use serde as usual: https://github.com/LNP-BP/rgb-node/blob/master/Cargo.toml#L21

serde = { version = "~1.0.111", features = ["derive"] }

apoelstra · 2020-10-08T13:16:39Z

Needs rebase, and probably should also be replaced wholesale with @dr-orlovsky's solution.

elichai · 2020-10-08T13:25:39Z

[dependencies]
serde_crate = { package = "serde", version = "~1.0.106", features = ["derive"], optional = true }

[features]
serde = ["serde_crate", "bitcoin/use-serde", "miniscript/serde"]

Renaming packages is only possible with 1.31+

stevenroose · 2020-10-10T20:38:05Z

What is the actual problem with the current approach? That some users don't read the docs and don't know about the use-serde feature and thus enable the serde feature instead and then get burned? Or just more a matter of principle that it should be possible to get all serde functions by using the feature serde?

Kixunil · 2020-10-11T15:14:27Z

@stevenroose naming features the same as crates is considered idiomatic Rust. It's mostly consistent across ecosystem (although not everyone follows it, people are slowly moving towards that style).

@dtolnay do you have any tip for solving this in older versions of Rust, please?

apoelstra · 2020-10-14T13:18:27Z

I wonder if we should have a Rust 2018 tracking issue and start tagging things that are blocked on it. (We should open issues for code cleanup/architecture stuff as well.) Would be good to have a clear picture of what we're missing by being on 1.29.

apoelstra · 2020-10-26T19:57:20Z

needs rebase

elichai · 2020-10-27T11:32:45Z

needs rebase

Is there a point? it sounded like this is Concept NACK from almost everyone? We could instead just throw this at the top of lib.rs:

#[cfg(all(feature = "serde", not(feature = "use-serde")))]
compile_error!("To use serde you must enable the use-serde feature");

which will throw an error for anyone enabling serde without enabling use-serde, and in the future we could remove this restriction.

I personally still prefer being consistent and if we'll change this in the future then we will go over all the cfg's and change them, on the other hand I don't want someone enabling serde and everything compiles but he doesn't have any serde traits (maybe on top of this refactoring we'll still leave this compile_error)

dr-orlovsky · 2020-10-28T19:42:30Z

I wonder if we should have a Rust 2018 tracking issue and start tagging things that are blocked on it. (We should open issues for code cleanup/architecture stuff as well.) Would be good to have a clear picture of what we're missing by being on 1.29.

@apoelstra: done #510

apoelstra · 2020-10-29T20:54:28Z

Thanks! Closing as I think this is blocked on Rust 2018.

I wouldn't be opposed to a nice complier error as Elichai suggests. But it should probably happen in another PR.

dr-orlovsky · 2020-10-30T10:24:47Z

I also like @elichai compiler error idea

Implement dr-orlovsky and Kixunil's suggestion in GitHub PR rust-bitcoin#373. Use package renaming so that we can have a feature `serde` instead of having `use-serde`.

tcharding · 2022-05-11T00:20:24Z

I tried to get the idea above about package renaming but I cannot get it to build. Build fails for all the serde derives. (Even with edition 2018 enabled.)

Kixunil · 2022-05-24T08:49:19Z

@tcharding the PR looks incomplete. There needs to be a change in Cargo.toml renaming the package to something like real_serde, the feature use-serde to serde, depending on real_serde and then extern crate real_serde as serde; in lib.rs

tcharding · 2022-05-24T21:34:51Z

I read this comment again this morning with fresh eyes but I still don't understand sorry. My comment was just a note for future readers that I had tried to implement the suggestion using package rename but was unable to get it working. (A note to myself as much as anyone else because I tried twice at different times.)

Kixunil · 2022-05-25T06:24:49Z

I guess I'll just do it then. :)

Kixunil · 2022-05-25T06:47:13Z

Done #1006

tcharding · 2022-05-25T07:23:52Z

Legend!

2e7effc Feature `use-serde` renamed to `serde` (Martin Habovstiak) Pull request description: Features activating external crates are supposed to have same name as those crates. However we depend on same feature in other crates so we need a separate feature. After MSRV bump it is possible to rename the crates and features so we can now fix this inconsistency. Sadly, derive can't see that the crate was renamed so all derives must be told to use the other one. Replaces #373 ACKs for top commit: apoelstra: ACK 2e7effc Tree-SHA512: b20364b9e8f30c2269bef915e821b2b2ec929e71dd0e88af2bc3a021821f87011d35e095cb8efe99add77a23dde940a17537eb387fb4582b05c57c8679969eb0

Replace feature serde with use-serde

2df6eb8

apoelstra added the API break This PR requires a version bump for the next release label Oct 8, 2020

thomaseizinger mentioned this pull request Oct 14, 2020

Sigma fun fixes LLFourn/secp256kfun#33

Merged

dr-orlovsky mentioned this pull request Oct 28, 2020

Rust 2018 & MSRV bump 1.41.1 tracking issue #510

Closed

20 tasks

elichai closed this Oct 30, 2020

Kixunil mentioned this pull request May 25, 2022

Feature use-serde renamed to serde #1006

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace feature serde with use-serde #373

Replace feature serde with use-serde #373

elichai commented Dec 23, 2019 •

edited

codecov-io commented Dec 23, 2019

stevenroose commented Dec 23, 2019

elichai commented Dec 23, 2019

stevenroose commented Dec 23, 2019

apoelstra commented Dec 23, 2019 •

edited

elichai commented Jan 8, 2020 •

edited

dr-orlovsky commented Sep 8, 2020

apoelstra commented Oct 8, 2020

elichai commented Oct 8, 2020

stevenroose commented Oct 10, 2020

Kixunil commented Oct 11, 2020

apoelstra commented Oct 14, 2020

apoelstra commented Oct 26, 2020

elichai commented Oct 27, 2020 •

edited

dr-orlovsky commented Oct 28, 2020

apoelstra commented Oct 29, 2020

dr-orlovsky commented Oct 30, 2020

tcharding commented May 11, 2022

Kixunil commented May 24, 2022

tcharding commented May 24, 2022

Kixunil commented May 25, 2022

Kixunil commented May 25, 2022

tcharding commented May 25, 2022

Replace feature serde with use-serde #373

Replace feature serde with use-serde #373

Conversation

elichai commented Dec 23, 2019 • edited

codecov-io commented Dec 23, 2019

Codecov Report

stevenroose commented Dec 23, 2019

elichai commented Dec 23, 2019

stevenroose commented Dec 23, 2019

apoelstra commented Dec 23, 2019 • edited

elichai commented Jan 8, 2020 • edited

dr-orlovsky commented Sep 8, 2020

apoelstra commented Oct 8, 2020

elichai commented Oct 8, 2020

stevenroose commented Oct 10, 2020

Kixunil commented Oct 11, 2020

apoelstra commented Oct 14, 2020

apoelstra commented Oct 26, 2020

elichai commented Oct 27, 2020 • edited

dr-orlovsky commented Oct 28, 2020

apoelstra commented Oct 29, 2020

dr-orlovsky commented Oct 30, 2020

tcharding commented May 11, 2022

Kixunil commented May 24, 2022

tcharding commented May 24, 2022

Kixunil commented May 25, 2022

Kixunil commented May 25, 2022

tcharding commented May 25, 2022

elichai commented Dec 23, 2019 •

edited

apoelstra commented Dec 23, 2019 •

edited

elichai commented Jan 8, 2020 •

edited

elichai commented Oct 27, 2020 •

edited