Skip to content

Conversation

@dr-orlovsky
Copy link
Contributor

No description provided.

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

Concept ACK

elichai
elichai previously approved these changes Jun 28, 2021
Copy link
Member

@elichai elichai left a comment

Choose a reason for hiding this comment

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

LGTM

@dr-orlovsky
Copy link
Contributor Author

It will be nice to get it merged to complete no_std support. How many ACKs do we need for this? @TheBlueMatt

@devrandom
Copy link
Contributor

are we missing some CI tests to catch these things?

@dr-orlovsky
Copy link
Contributor Author

@devrandom we need cross-lib integration CI (impossible with GitHub) for that I assume

@devrandom
Copy link
Contributor

@devrandom we need cross-lib integration CI (impossible with GitHub) for that I assume

Actually, I'm confused now. If this works in practice (not test failures, no failures in rust-bitcoin that depends on it), then it probably means that we don't actually use anything from serde that is gated by std or alloc. If that's the case, then isn't it up to whatever crate that depends on this one to pull in features from serde that it needs, if it needs them?

@elichai elichai self-requested a review August 10, 2021 08:23
@elichai
Copy link
Member

elichai commented Aug 10, 2021

Let me take back my ACK, it seems to me that this library never needs serde for the alloc/std stuff, so I don't see a reason why we should enable those features.
if downstream crates use serde by themselves they should import it and enable the features they need.

(that's also part of the reason why no tests "failed" here)

@elichai elichai dismissed their stale review August 10, 2021 08:26

explained in the comment above

@apoelstra
Copy link
Member

Closing as the value proposition is unclear and this seems stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants