Disable Serde's default-features #905

ass3rt · 2022-03-24T05:32:29Z

With this patch, existing users of the use-serde feature will no longer be
compiling with serde/std enabled, but this allows dependent projects
to import serde and enable serde/alloc as required by some no-std targets.

ass3rt · 2022-03-24T14:39:05Z

Seems I have some test failures to look into. I'll try to get these worked out tonight.

tcharding · 2022-03-24T23:51:04Z

Changing features makes this a breaking change, right? We are in the process of a big breaking release anyways but thought I'd flag it.

Cargo.toml

ass3rt · 2022-03-25T01:36:41Z

Changing features makes this a breaking change, right?

That is my understanding 👍

apoelstra

ACK f18c42c8e51821d1e0d0c0f8fd7358f8e02d0002

Yes, this is a breaking change, but it's fine to bring into the RC.

dr-orlovsky

NACK: build is broken due to test dependency issues + possible undesirable side-effect of always having non-std serde even for std builds

Cargo.toml

ass3rt · 2022-03-28T13:59:18Z

Sorry for the repeated test failures. I promise I am running ./contrib/test.sh before submitting patches, but the tests are passing on my system. Perhaps I'm missing a step. If anyone has a recommended test workflow before submitting PRs, I will happily accept any advice to improve my testing :)

apoelstra · 2022-03-28T14:05:30Z

There are probably some environment variables you need to set to get test.sh to do anything. In particular DO_FEATURE_MATRIX=true.

Kixunil · 2022-03-29T14:02:56Z

The test failures are actually caused by the lack of serde-std feature. We really have to use it since I don't believe anyone here wants to bump MSRV to 1.60.

ass3rt · 2022-04-06T04:10:44Z

I've reworked this PR to address comments, fix tests, and rebase onto master. Sorry for the delay (its been a busy week on other projects).

Specifically, I added the serde-std feature for those that need it, and enabled it for tests.

Cargo.toml

Kixunil

This doesn't look right, the feature is not used anywhere. Did you forget to commit some files?

ass3rt · 2022-04-06T14:10:32Z

This doesn't look right, the feature is not used anywhere. Did you forget to commit some files?

The only place I am aware of that requires serde-std is in tests, which I added. Are you aware of anywhere else that requires it?

Keep in mind, any feature which enables serde-std will break no-std builds (obviously), so IMO it should not be a requirement for any code which we expect to be no-std compatible

Kixunil · 2022-04-07T12:52:06Z

Sorry, it looks like I was confused and you don't need serde-std, just call tests with use-serde,std or use-serde,no-std (which is broken because of #947).

ass3rt · 2022-04-07T13:57:53Z

Ok, I'll back out serde-std. It was a useful thought experiment, if nothing else :)

Kixunil · 2022-04-07T14:05:30Z

Also to address @dr-orlovsky concern: users needing serde/std can and should enable it explicitly when depending on serde.

ass3rt · 2022-04-07T18:36:02Z

Oops I forgot to run the dep_test. I did have environment variables set to enable that test, but I must've cleared them before my last push 🤦.

I forgot to explicitly add serde with default features as a dependency for the dep_test. Fixed and force pushed. Tests are passing again. Sorry for all the churn!

Kixunil · 2022-04-08T11:18:37Z

Oh, shit. I got confused. For some reason the compatibility issue can't be triggered from normal cargo test, only from a dependency and I also forgot to set the env var. 🤦‍♂️

To clean up my shit, I looked at it properly and found a proper solution. Wanted to make a PR against your repo but was unable to do so. Here are my changes: Kixunil@28bb060

Explanation:

I started re-adding serde-std but then I realized the affected methods actually re-implement the default behavior (the existing ones already forward!) so I could just delete them. It looked like that was the only change required but then I noticed in case of script we can get perf benefit from overriding. I then realized we don't need serde-std but serde-alloc and soon after I realized we can depend on it unconditionally because the whole library already depends on alloc and I don't think it will stop depending on it any time soon. (Maybe bitcoin-units will one day.) Thus I just added alloc to the serde features and kept the other changes (even though they are off-topic).

If you want to be super-pedantic and make the history nicer you could split it up into three commits (changing serde features, removing default impls, adding override) but I personally wouldn't block the PR if you don't want to.

Suggested commit descriptions:

same as now
Removed reimplementations of default methods. (next paragraph) The default methods do the exact same thing thus our overrides are useless, potentially even problematic.
Override default visit_byte_buf on Script. (next paragraph) This override may avoid allocation and thus make the deserialization faster.

ass3rt · 2022-04-08T18:28:02Z

@Kixunil good finds! I like that approach. I am afk for several hours today, but I found the github setting to let others push commits to my PR. You should be able to push now. If not, I'll pull your changes in when I get a chance this evening.

Kixunil · 2022-04-09T07:46:52Z

Cool, pushed! Do you want to do the squashing?

Kixunil · 2022-04-09T07:59:15Z

Oh, shit, MSRV issues. Suggested solution: remove the optimization and alloc dependency and reintroduce them after MSRV bump.

Now I finally understand why serde didn't do std = ["alloc"] but conditions everything using any(feature = "std", feature = "alloc").

Kixunil · 2022-04-22T08:32:25Z

Let's just wait for #964

apoelstra · 2022-04-22T23:24:41Z

We have #964 now -- can you rebase?

ass3rt · 2022-04-22T23:57:33Z

We have #964 now -- can you rebase?

Awesome. Done ✔️

Kixunil · 2022-04-25T09:50:08Z

Could you clean up the git history please? Ideally make it three commits: 1. the topic of this PR, 2. removing unneeded method impls, 2 adding the optimization. But I can accept squashing to a single commit.

We do not need serde/std, only serde/alloc. Serde/std breaks no-std builds, but serde/alloc does not. Depending on serde/alloc is the more compatible approach, as the entire library already depends on alloc.

The default methods do the exact same thing thus our overrides are useless, potentially even problematic. Credit to Kixunil for this fix: rust-bitcoin#905 (comment)

This override may avoid allocation and thus make the deserialization faster. Credit to Kixunil for this fix: rust-bitcoin#905 (comment)

ass3rt · 2022-04-25T14:55:24Z

@Kixunil good call. Done. Thanks for all your assistance!

Kixunil

Perfect, thanks!

ACK 76fcf81

tcharding

ACK 76fcf81

apoelstra

ACK 76fcf81

The default methods do the exact same thing thus our overrides are useless, potentially even problematic. Credit to Kixunil for this fix: rust-bitcoin/rust-bitcoin#905 (comment)

This override may avoid allocation and thus make the deserialization faster. Credit to Kixunil for this fix: rust-bitcoin/rust-bitcoin#905 (comment)

76fcf81 Override default visit_byte_buf on Script (ass3rt) add100c Removed reimplementations of default methods (ass3rt) 7db03f2 Disable Serde's default-features (ass3rt) Pull request description: With this patch, existing users of the `use-serde` feature will no longer be compiling with `serde/std` enabled, but this allows dependent projects to import serde and enable `serde/alloc` as required by some no-std targets. ACKs for top commit: Kixunil: ACK 76fcf81 tcharding: ACK 76fcf81 apoelstra: ACK 76fcf81 Tree-SHA512: 5748e64e1f91f19dbfbf32bead6e6d759e448e92ed0dab731b3059f6b37bd811fad6654edc8fbd113e3be17fefaf9fc4912145d6b61484ced0517712361ecfdc

ass3rt mentioned this pull request Mar 24, 2022

No way to enable serde for no-std builds #904

Closed

ass3rt force-pushed the disable-serde-defaults branch from 5e25c01 to 046e52e Compare March 24, 2022 05:34

tcharding reviewed Mar 24, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

ass3rt force-pushed the disable-serde-defaults branch from 046e52e to f18c42c Compare March 25, 2022 01:35

apoelstra previously approved these changes Mar 26, 2022

View reviewed changes

dr-orlovsky suggested changes Mar 27, 2022

View reviewed changes

Cargo.toml Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

ass3rt dismissed apoelstra’s stale review via 580650f March 27, 2022 18:31

ass3rt force-pushed the disable-serde-defaults branch 2 times, most recently from dd63987 to 9cc4844 Compare April 6, 2022 03:56

ass3rt requested review from tcharding, dr-orlovsky, apoelstra and Kixunil April 6, 2022 04:12

tcharding reviewed Apr 6, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

Kixunil reviewed Apr 6, 2022

View reviewed changes

Kixunil mentioned this pull request Apr 7, 2022

b72f56c4 made no_std MSRV 1.53 #947

Closed

ass3rt force-pushed the disable-serde-defaults branch 2 times, most recently from 2ed3175 to fcbe52d Compare April 7, 2022 18:35

ass3rt force-pushed the disable-serde-defaults branch from fcbe52d to 29677df Compare April 7, 2022 18:38

ass3rt requested a review from Kixunil April 7, 2022 18:39

Kixunil added the before edition bump We want to merge this before edition is bumped label Apr 22, 2022

Kixunil added this to the 0.29.0 milestone Apr 22, 2022

ass3rt force-pushed the disable-serde-defaults branch from 28bb060 to 1541e14 Compare April 22, 2022 23:57

ass3rt added 3 commits April 25, 2022 09:51

Disable Serde's default-features

7db03f2

We do not need serde/std, only serde/alloc. Serde/std breaks no-std builds, but serde/alloc does not. Depending on serde/alloc is the more compatible approach, as the entire library already depends on alloc.

Removed reimplementations of default methods

add100c

The default methods do the exact same thing thus our overrides are useless, potentially even problematic. Credit to Kixunil for this fix: rust-bitcoin#905 (comment)

Override default visit_byte_buf on Script

76fcf81

This override may avoid allocation and thus make the deserialization faster. Credit to Kixunil for this fix: rust-bitcoin#905 (comment)

ass3rt force-pushed the disable-serde-defaults branch from 1541e14 to 76fcf81 Compare April 25, 2022 14:54

Kixunil approved these changes Apr 25, 2022

View reviewed changes

Kixunil added the one ack PRs that have one ACK, so one more can progress them label Apr 25, 2022

tcharding approved these changes Apr 25, 2022

View reviewed changes

apoelstra approved these changes Apr 30, 2022

View reviewed changes

apoelstra merged commit 9f81798 into rust-bitcoin:master Apr 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable Serde's default-features #905

Disable Serde's default-features #905

ass3rt commented Mar 24, 2022 •

edited

Loading

ass3rt commented Mar 24, 2022

tcharding commented Mar 24, 2022

ass3rt commented Mar 25, 2022

apoelstra left a comment

dr-orlovsky left a comment

ass3rt commented Mar 28, 2022

apoelstra commented Mar 28, 2022

Kixunil commented Mar 29, 2022

ass3rt commented Apr 6, 2022 •

edited

Loading

Kixunil left a comment

ass3rt commented Apr 6, 2022 •

edited

Loading

Kixunil commented Apr 7, 2022

ass3rt commented Apr 7, 2022

Kixunil commented Apr 7, 2022

ass3rt commented Apr 7, 2022

Kixunil commented Apr 8, 2022 •

edited

Loading

ass3rt commented Apr 8, 2022

Kixunil commented Apr 9, 2022

Kixunil commented Apr 9, 2022

Kixunil commented Apr 22, 2022

apoelstra commented Apr 22, 2022

ass3rt commented Apr 22, 2022

Kixunil commented Apr 25, 2022

ass3rt commented Apr 25, 2022

Kixunil left a comment

tcharding left a comment

apoelstra left a comment

Disable Serde's default-features #905

Disable Serde's default-features #905

Conversation

ass3rt commented Mar 24, 2022 • edited Loading

ass3rt commented Mar 24, 2022

tcharding commented Mar 24, 2022

ass3rt commented Mar 25, 2022

apoelstra left a comment

Choose a reason for hiding this comment

dr-orlovsky left a comment

Choose a reason for hiding this comment

ass3rt commented Mar 28, 2022

apoelstra commented Mar 28, 2022

Kixunil commented Mar 29, 2022

ass3rt commented Apr 6, 2022 • edited Loading

Kixunil left a comment

Choose a reason for hiding this comment

ass3rt commented Apr 6, 2022 • edited Loading

Kixunil commented Apr 7, 2022

ass3rt commented Apr 7, 2022

Kixunil commented Apr 7, 2022

ass3rt commented Apr 7, 2022

Kixunil commented Apr 8, 2022 • edited Loading

ass3rt commented Apr 8, 2022

Kixunil commented Apr 9, 2022

Kixunil commented Apr 9, 2022

Kixunil commented Apr 22, 2022

apoelstra commented Apr 22, 2022

ass3rt commented Apr 22, 2022

Kixunil commented Apr 25, 2022

ass3rt commented Apr 25, 2022

Kixunil left a comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

ass3rt commented Mar 24, 2022 •

edited

Loading

ass3rt commented Apr 6, 2022 •

edited

Loading

ass3rt commented Apr 6, 2022 •

edited

Loading

Kixunil commented Apr 8, 2022 •

edited

Loading