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

chore(gossipsub): include in libp2p meta crate when compiling for wasm #4217

Merged
merged 4 commits into from
Jul 20, 2023
Merged

chore(gossipsub): include in libp2p meta crate when compiling for wasm #4217

merged 4 commits into from
Jul 20, 2023

Conversation

onur-ozkan
Copy link
Contributor

@onur-ozkan onur-ozkan commented Jul 18, 2023 •
edited by thomaseizinger
Loading

Description

Since #3973, gossipsub now fully supports wasm targets. It functions properly when added as a dependency on its own. However, when trying to use it under libp2p by activating a feature, it's not possible and the compiler will raise an error like unresolved import libp2p::gossipsub. This pull request enables the use of gossipsub for wasm targets when it's activated as a feature of the libp2p dependency.

onur-ozkan
onur-ozkan commented Jul 18, 2023
View reviewed changes
libp2p/src/lib.rs Outdated Show resolved Hide resolved
@onur-ozkan onur-ozkan marked this pull request as draft July 18, 2023 18:25
@onur-ozkan onur-ozkan marked this pull request as ready for review July 18, 2023 18:32
@mergify
Copy link
Contributor

mergify bot commented Jul 19, 2023

This pull request has merge conflicts. Could you please resolve them @ozkanonur? 🙏

@onur-ozkan
update wasm compilation conditions of gossipsub
4930ecf 
Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan
reorder libp2p-metrics dependencies
e730c0d 
Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan onur-ozkan changed the title chore(wasm): update compilation conditions of gossipsub chore(wasm): remove compilation conditions of gossipsub Jul 19, 2023
@thomaseizinger
Copy link
Contributor

thomaseizinger commented Jul 19, 2023 •
edited
Loading

Meta-note: Please don't delete the pull-request description template, our mergify automation relies on the markdown headers being present to extract the final commit message.

@thomaseizinger thomaseizinger changed the title chore(wasm): remove compilation conditions of gossipsub chore(gossipsub): include in libp2p meta crate when compiling for wasm Jul 19, 2023
thomaseizinger
thomaseizinger approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Great, thank you!

Sorry that we missed this. With this change, our CI will now properly check that gossipsub actually continuous to compile for WASM!

@thomaseizinger
Copy link
Contributor

Can you include a changelog entry in libp2p please and bump the patch version?

@onur-ozkan
Copy link
Contributor Author

Can you include a changelog entry in libp2p please and bump the patch version?

What about libp2p-metrics ? Should I do the same for it too?

@thomaseizinger
Copy link
Contributor

Can you include a changelog entry in libp2p please and bump the patch version?

What about libp2p-metrics ? Should I do the same for it too?

We just remove a comment there, right? I don't think that is a release-worth change.

@thomaseizinger
Copy link
Contributor

Can you include a changelog entry in libp2p please and bump the patch version?

What about libp2p-metrics ? Should I do the same for it too?

We just remove a comment there, right? I don't think that is a release-worth change.

Ah my bad, just looked at the changes again. Yes that will need a bump too, well spotted!

thomaseizinger
thomaseizinger reviewed Jul 19, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks, some nits around formatting!

libp2p/CHANGELOG.md Outdated Show resolved Hide resolved
misc/metrics/CHANGELOG.md Outdated Show resolved Hide resolved
misc/metrics/CHANGELOG.md Outdated Show resolved Hide resolved
misc/metrics/Cargo.toml Show resolved Hide resolved
libp2p/CHANGELOG.md Outdated Show resolved Hide resolved
@onur-ozkan
bump libp2p and libp2p-metrics
a0a3d76 
Signed-off-by: ozkanonur <work@onurozkan.dev>
thomaseizinger
thomaseizinger previously approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@mergify mergify bot dismissed thomaseizinger’s stale review July 20, 2023 07:33

Approvals have been dismissed because the PR was updated after the send-it label was applied.

@mergify mergify bot merged commit 660fadb into libp2p:master Jul 20, 2023
@onur-ozkan onur-ozkan deleted the gossipsub-wasm branch July 20, 2023 09:19
mxinden
mxinden reviewed Jul 23, 2023
View reviewed changes
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thank you for the patch.

thomaseizinger pushed a commit that referenced this pull request Aug 20, 2023
@onur-ozkan @thomaseizinger
chore(gossipsub): include in libp2p meta crate when compiling for wasm
eed92ab 
Since #3973, gossipsub now fully supports wasm targets. It functions properly when added as a dependency on its own. However, when trying to use it under libp2p by activating a feature, it's not possible and the compiler will raise an error like `unresolved import libp2p::gossipsub`. This pull request enables the use of gossipsub for wasm targets when it's activated as a feature of the libp2p dependency.

Pull-Request: #4217.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxinden mxinden mxinden left review comments

@dariusc93 dariusc93 dariusc93 left review comments

@thomaseizinger thomaseizinger thomaseizinger approved these changes

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@onur-ozkan @thomaseizinger @dariusc93 @mxinden