Skip to content

extract file-download crate to remove solana-runtime dep from cargo-build-sbf#3460

Merged
joncinque merged 9 commits intoanza-xyz:masterfrom
kevinheavey:file-downloader-crate
Nov 6, 2024
Merged

extract file-download crate to remove solana-runtime dep from cargo-build-sbf#3460
joncinque merged 9 commits intoanza-xyz:masterfrom
kevinheavey:file-downloader-crate

Conversation

@kevinheavey
Copy link
Copy Markdown

Problem

cargo-build-sbf has a superfluous dependency on solana-runtime, which is a huge crate, via solana-download-utils.

Summary of Changes

move solana_download_utils::download_file to its own crate and use this new crate in cargo-build-sbf

Copy link
Copy Markdown

@LucasSte LucasSte left a comment

Choose a reason for hiding this comment

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

In general looks good. I just requested @joncinque's review, for I believe he is more familiar with this part of the code.

Comment thread download-utils/src/lib.rs Outdated
Comment on lines +2 to +5
pub use solana_file_downloader::DownloadProgressRecord;
use {
console::Emoji,
indicatif::{ProgressBar, ProgressStyle},
log::*,
solana_file_downloader::{download_file, DownloadProgressCallbackOption},
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you merge the solana_file_downloader imports here?

Copy link
Copy Markdown
Author

@kevinheavey kevinheavey Nov 5, 2024

Choose a reason for hiding this comment

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

I want to re-export DownloadProgressRecord specifically because it's part of the public API via download_snapshot_archive. If it's not re-exported then solana_validator needs to add solana_file_downloader as a direct dependency to use DownloadProgressRecord in download_snapshot_archive

Comment thread download-utils/src/lib.rs Outdated
Copy link
Copy Markdown

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for doing the work! The only thing I would ask is to put the new crate under sdk. That'll make it easier when splitting everything out of this repo.

Actually, with this change, are there any other dependencies from sdk crates into non-sdk crates? (All I see is frozen-abi, frozen-abi-macro, logger, define-syscall, short-vec, which can all definitely move into sdk)

Comment thread file-downloader/Cargo.toml Outdated
@kevinheavey
Copy link
Copy Markdown
Author

Actually, with this change, are there any other dependencies from sdk crates into non-sdk crates? (All I see is frozen-abi, frozen-abi-macro, logger, define-syscall, short-vec, which can all definitely move into sdk)

#3498

Copy link
Copy Markdown

@joncinque joncinque 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 to me! @yihau can you accept ownership of solana-file-download?

@kevinheavey kevinheavey added automerge automerge Merge this Pull Request automatically once CI passes and removed automerge automerge Merge this Pull Request automatically once CI passes labels Nov 6, 2024
@kevinheavey kevinheavey force-pushed the file-downloader-crate branch from eac87bf to 5d07e22 Compare November 6, 2024 22:36
@joncinque joncinque added the automerge automerge Merge this Pull Request automatically once CI passes label Nov 6, 2024
@joncinque joncinque changed the title extract file-downloader crate to remove solana-runtime dep from cargo-build-sbf extract file-download crate to remove solana-runtime dep from cargo-build-sbf Nov 6, 2024
@joncinque joncinque merged commit c061774 into anza-xyz:master Nov 6, 2024
@kevinheavey kevinheavey deleted the file-downloader-crate branch November 7, 2024 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge automerge Merge this Pull Request automatically once CI passes need:merge-assist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants