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.

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

make bincode optional for plexi_core #9

Merged
merged 7 commits into from
Oct 11, 2024
Merged

Conversation

haochenuw
Copy link
Collaborator

This PR hides bincode dependency behind a feature. So that it is not a required dependency for plexi_core.

There are multiple reasons, one being that bincode 2 is not officially released and according to the dev team of bincode, the API is subject to breakages.

Copy link
Collaborator

@thibmeu thibmeu left a comment

Choose a reason for hiding this comment

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

Thanks for the change! It looks great.

one comment to add bincode as default.
locally you can also run cargo fmt to fix the formatter.

some background: the decision to use bincode 2, even if alpha, is it provides a serialisation that's simpler to read, and has a definition, on the contrary to bincode 1, or protobuf that needs code generation.

plexi_core/Cargo.toml Outdated Show resolved Hide resolved

[dependencies]
akd = { workspace = true, features = ["whatsapp_v1", "public_auditing"], optional = true }
anyhow = { workspace = true }
bincode = { workspace = true }
bincode = { version = "2.0.0-rc.3", optional = true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can leave the workspace here. dependencies are managed at the higher level.

Copy link
Collaborator Author

@haochenuw haochenuw Oct 9, 2024

Choose a reason for hiding this comment

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

@thibmeu I tried it but encountered the following error during build

Caused by:
  bincode is optional, but workspace dependencies cannot be optional

Since my goal is to make bincode crate optional for plexi_core. It seems it's required to make it a non workspace dependency.

Copy link
Collaborator

Choose a reason for hiding this comment

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

something like this should work I think haochenuw#1
if you can confirm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure what you meant by "something like this". Could you elaborate?

Copy link
Collaborator

Choose a reason for hiding this comment

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

did you check out the PR haochenuw#1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I see. I've merged your PR :)

thibmeu and others added 2 commits October 10, 2024 13:18
Dependecnies are managed by the workspace
Add bincode as a workspace dependency
@thibmeu thibmeu merged commit 7f89b2e into cloudflare:main Oct 11, 2024
9 checks passed
@thibmeu
Copy link
Collaborator

thibmeu commented Oct 11, 2024

Merged. Thanks again for the contribution!

@haochenuw
Copy link
Collaborator Author

thanks! could we publish this to crates.io (if necessary, bump the version to 0.1.1)? so our internal codebase can ingest it properly

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.

2 participants