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

Mithril-client-lib: Gate snapshot download & message computation behind a feature #1344

Merged
merged 8 commits into from
Nov 22, 2023
Merged

Mithril-client-lib: Gate snapshot download & message computation behind a feature #1344

merged 8 commits into from
Nov 22, 2023

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Nov 13, 2023 •
edited by jpraynaud
Loading

Content

This PR gates the snapshot download and snapshot message computation of the mithril-client library behind a fs feature.

This feature is disabled by default because it simpler to enable a feature than to disable a default feature.

The doc is updated to add "Available on crate feature fs only" labels, ie:
image

But for those labels to show we need to trick cargo since this features is still unstable.
For that we took inspiration from tokio, a major crate that use this feature, cf their main toml (for how to trick doc rs to add a rustc flag that will be used to enable the feature) and their dedicated doc macros.

To test the the labels locally you need to:

  1. remove the cfg_attr(docsrs conditions from the lib.rs
  2. switch to the unstable rust (rustup default nightly)
  3. build and open the doc: cargo doc --no-deps -p mithril-client --open --all-features

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)

Comments

CI scripts had to be adapted to use the feature to trigger the tests and to include the gated functions in the doc.

Issue(s)

Relates to #1311

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 13, 2023 •
edited
Loading

Test Results

    3 files  ±0    30 suites  ±0   7m 27s ⏱️ +6s
736 tests ±0  736 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
844 runs  ±0  844 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit c1b9afb. ± Comparison against base commit d02447f.

This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both. 
mithril-client ‑ snapshot_client::tests::download_unpack_send_feedbacks

mithril-client ‑ snapshot_client::tests_download::download_unpack_send_feedbacks

♻️ This comment has been updated with latest results.

@Alenar Alenar force-pushed the djo/1311/feature_for_snapshot_download branch from 35cfe76 to a980484 Compare November 13, 2023 17:22
jpraynaud
jpraynaud approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jpraynaud jpraynaud marked this pull request as ready for review November 14, 2023 17:02
@jpraynaud jpraynaud force-pushed the djo/1311/feature_for_snapshot_download branch from a980484 to 5e03472 Compare November 14, 2023 17:07
@jpraynaud jpraynaud force-pushed the djo/1311/feature_for_snapshot_download branch 2 times, most recently from 306fadf to 51fa12b Compare November 15, 2023 10:43
@jpraynaud jpraynaud force-pushed the djo/1311/feature_for_snapshot_download branch from 51fa12b to 8e7a064 Compare November 20, 2023 09:22
Alenar and others added 7 commits November 21, 2023 18:23
@Alenar @jpraynaud
Put snapshot download & snapshot message compute behind a 'fs' feature
d2c98c9 
- This feature is disabled by default
- A `full` feature is also added to enable it and any other features that we
  would add in the future
@Alenar @jpraynaud
Update website library documentation
bcf7402
@Alenar @jpraynaud
Wip adapt CI to the common fs feature
0277958
@Alenar @jpraynaud
Allow publish-crate-package to receive additional cargo args
e917080 
This will allow to pass the features when building mithril-client.
@jpraynaud
Fix warnings when not using 'fs' feature
39a978b
@jpraynaud
Fix makefile for client
3affd4b
@jpraynaud
Make feature 'full' default
3c52f84 
This is a temporary fix to avoid red nix CI.
@jpraynaud jpraynaud force-pushed the djo/1311/feature_for_snapshot_download branch from 8e7a064 to 3c52f84 Compare November 21, 2023 17:24
@jpraynaud jpraynaud merged commit 221a003 into main Nov 22, 2023
33 checks passed
@jpraynaud jpraynaud deleted the djo/1311/feature_for_snapshot_download branch November 22, 2023 14:32
@jpraynaud
Copy link
Member

FYI @scarmuega, @falcucci you might need to add the full feature on the mithril-client dependency in your developments 🙂

falcucci pushed a commit to falcucci/mithril that referenced this pull request Nov 28, 2023
@jpraynaud
Merge pull request input-output-hk#1344 from input-output-hk/djo/1311…
b87cec8 
…/feature_for_snapshot_download

Mithril-client-lib: Gate snapshot download & message computation behind a feature
falcucci pushed a commit to falcucci/mithril that referenced this pull request Nov 28, 2023
@jpraynaud
Merge pull request input-output-hk#1344 from input-output-hk/djo/1311…
564197a 
…/feature_for_snapshot_download

Mithril-client-lib: Gate snapshot download & message computation behind a feature
falcucci pushed a commit to falcucci/mithril that referenced this pull request Nov 28, 2023
@jpraynaud
Merge pull request input-output-hk#1344 from input-output-hk/djo/1311…
90ac02a 
…/feature_for_snapshot_download

Mithril-client-lib: Gate snapshot download & message computation behind a feature
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpraynaud jpraynaud jpraynaud approved these changes

@ghubertpalo ghubertpalo ghubertpalo approved these changes

@dlachaume dlachaume dlachaume 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.
4 participants
@Alenar @jpraynaud @ghubertpalo @dlachaume