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

ci: properly test feature matrix of libp2p using cargo-hack #4880

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

ci: properly test feature matrix of libp2p using cargo-hack #4880

wants to merge 19 commits into from

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Nov 16, 2023 •
edited
Loading

Description

Instead of specifying a feature matrix manually, use cargo-hack to run through an entire matrix of features for the libp2p crate.

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@thomaseizinger thomaseizinger changed the base branch from master to fix/unreachable-pub-libp2p November 16, 2023 23:21
@thomaseizinger thomaseizinger force-pushed the fix/unreachable-pub-libp2p branch from bf253ce to 801cde5 Compare November 17, 2023 00:09
Base automatically changed from fix/unreachable-pub-libp2p to master November 17, 2023 03:04
@thomaseizinger thomaseizinger mentioned this pull request Nov 20, 2023
Closed
4 tasks
@thomaseizinger thomaseizinger marked this pull request as ready for review November 20, 2023 03:41
mxinden
mxinden approved these changes Nov 20, 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 fix and the updated CI!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
libp2p/src/builder/phase/quic.rs Outdated Show resolved Hide resolved
libp2p/src/builder/phase/tcp.rs Outdated Show resolved Hide resolved
libp2p/src/builder/phase/websocket.rs Outdated Show resolved Hide resolved
libp2p/src/builder/phase/websocket.rs Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor Author

@mxinden I've reduced the checking of feature matrices down to a select few transports. That keeps the combinations at a reasonable level such that we don't need to specify depth. Plus, I am not aware of any code that is conditional on multiple protocols, hence we don't need to check those combinations.

@mergify Mergify

This comment was marked as resolved.

Sign in to view
@thomaseizinger
Copy link
Contributor Author

@mxinden Feel free to merge the companion PR (libp2p/github-mgmt#198) and this at any time.

@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 29, 2024

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

1 similar comment
@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 29, 2024

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

@thomaseizinger thomaseizinger force-pushed the feat/libp2p-feature-matrix branch from 96af94f to d0c6443 Compare January 30, 2024 03:10
@thomaseizinger
Copy link
Contributor Author

@mxinden Feel free to merge the companion PR (libp2p/github-mgmt#198) and this at any time.

@mxinden This would be ready if the companion PR is also merged.

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 19, 2024

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

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

@mxinden mxinden mxinden approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thomaseizinger @mxinden