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

feat: support file:// URLs for snapshot locations #1885

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

michalrus
Copy link
Member

@michalrus michalrus commented Aug 9, 2024

LW-11112

Content

This PR includes support for the file:// scheme in URLs of the snapshots.

This is very useful for live demos, where you don’t want to repeatedly download from Google Cloud Storage because of the wait time and cost.

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is 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

It requires a proxy modifying responses from the aggregator, e.g. this.

Issue(s)

LW-11112

Copy link
Collaborator

@Alenar Alenar left a comment

Choose a reason for hiding this comment

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

Looks good, good work 🚀.

I've left a few suggestions below.

mithril-client/src/snapshot_downloader.rs Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
mithril-client/Cargo.toml Outdated Show resolved Hide resolved
mithril-client/src/snapshot_downloader.rs Show resolved Hide resolved
mithril-client/src/snapshot_downloader.rs Outdated Show resolved Hide resolved
@michalrus
Copy link
Member Author

michalrus commented Aug 22, 2024

Thanks for the positive reviews! 😃 I will try to update it today ⏳

@michalrus michalrus force-pushed the LW-11112-file-scheme-urls branch from 650f2db to 775b7d6 Compare August 22, 2024 16:15
@michalrus michalrus requested review from Alenar and jpraynaud August 22, 2024 16:17
@michalrus
Copy link
Member Author

michalrus commented Aug 23, 2024

@jpraynaud, @Alenar, I followed your suggestions, but I'm not sure why the macos-14 job failed? No log, but since macos-12 worked, it might be enough to just restart it, but I don't have the necessary privileges.

@jpraynaud
Copy link
Member

@jpraynaud, @Alenar, I followed your suggestions, but I'm not sure why the macos-14 job failed? No log, but since macos-12 worked, it might be enough to just restart it, but I don't have the necessary privileges.

Apparently the jobs failed to start @michalrus, I just restarted them 👍

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 🚀

Thanks for the contribution @michalrus!

Can you update the CHANGELOG and bump the mithril-client patch version before we can merge your PR?

LW-11112

This is very useful for live demos, where you don’t want to repeatedly
download from Google Cloud Storage because of the wait time and cost.
@michalrus michalrus force-pushed the LW-11112-file-scheme-urls branch from 775b7d6 to ecb9af3 Compare August 23, 2024 08:23
@michalrus
Copy link
Member Author

Thank you!

Can you update the CHANGELOG and bump the mithril-client patch version before we can merge your PR?

Done in ecb9af3. I also had to rebase onto the current main.

@michalrus
Copy link
Member Author

michalrus commented Aug 23, 2024

Oh, sorry, I didn't update the Cargo.lock 🤦 ⏳

Edit: done (diff).

@michalrus michalrus force-pushed the LW-11112-file-scheme-urls branch from ecb9af3 to 6ffd08c Compare August 23, 2024 09:33
@michalrus
Copy link
Member Author

For some reason ci/hydra-build:x86_64-darwin.mithril-signer is failing:

     Running `/private/tmp/nix-build-mithril-signer-0.2.173.drv-0/source/target/release/deps/mithril_signer-ffb4edaeea67228b`
<jemalloc>: Error allocating TSD
error: test failed, to rerun pass `-p mithril-signer --lib`

I have restarted the evaluation.

@michalrus
Copy link
Member Author

ci/hydra-build:x86_64-darwin.mithril-signer failed again, with the same error message. Could it be my fault? 🤔 I'll try to run it locally on my MacBook.

@michalrus
Copy link
Member Author

I was able to reproduce this failure locally on an x86_64-darwin:

❯ nix build --fallback -L .#hydraJobs.x86_64-darwin.mithril-signer

[…]

   Compiling mithril-signer v0.2.173 (/private/tmp/nix-build-mithril-signer-0.2.173.drv-0/source/mithril-signer)

[…]

    Finished `release` profile [optimized] target(s) in 55.52s
     Running `/private/tmp/nix-build-mithril-signer-0.2.173.drv-0/source/target/release/deps/mithril_signer-ffb4edaeea67228b`
<jemalloc>: Error allocating TSD
error: test failed, to rerun pass `-p mithril-signer --lib`

Caused by:
  process didn't exit successfully: `/private/tmp/nix-build-mithril-signer-0.2.173.drv-0/source/target/release/deps/mithril_signer-ffb4edaeea67228b` (signal: 6, SIGABRT: process abort signal)

I'm not sure how to proceed, @jpraynaud, @Alenar. 🤔

@jpraynaud
Copy link
Member

ci/hydra-build:x86_64-darwin.mithril-signer failed again, with the same error message. Could it be my fault? 🤔 I'll try to run it locally on my MacBook.

There is some flakiness on the Hydra CI sometimes, but it's not related to your PR which doesn't modify the mithril-signer 👍

We're good to merge now, thanks again @michalrus!

@jpraynaud jpraynaud merged commit b3faff0 into input-output-hk:main Aug 23, 2024
38 of 39 checks passed
@michalrus michalrus deleted the LW-11112-file-scheme-urls branch August 23, 2024 12:44
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.

3 participants