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

refactor: smoother large file download&proxy #463

Merged
merged 10 commits into from
Dec 12, 2024

Conversation

discord9
Copy link
Contributor

add a few changes to improve user experience:

  • make download files stream which improve experience when downloading larger files(like rust.zip)
  • read https_proxy/all_proxy from env var(cross platform behavior) from https_proxy/HTTPS_PROXY/all_proxy/ALL_PROXY var
  • make some blocking function call in async context into spawn_blocking so tokio wouldn't panic in debug mode

Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Did some testing and there couldn't any time improvements, but the way we handle proxies is definitely better. Mind solving the logging comments?

src/toolchain/mod.rs Outdated Show resolved Hide resolved
@discord9
Copy link
Contributor Author

Thanks for your contribution! Did some testing and there couldn't any time improvements, but the way we handle proxies is definitely better. Mind solving the logging comments?

Sure, let me think of a better way to show multiple downloading processes. Also, yep the streaming download solution is not to improve speed, but to show user that we are actively downloading and patient is needed if network speed is low, which is very reassuring

@discord9
Copy link
Contributor Author

discord9 commented Dec 11, 2024

bar_example
use indicatif to print multiple download bar, however due to printing download bar will cause logging message not able to print, I had to make a logic change which is all download task will wait for all download to complete before proceed, hopefully this wouldn't affect anything since after download then is just decompress? If it actually does then I need to found another way to display progress bar
fixed: now logging and process bar should not messed with each other

Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements! The bars are way better to display the download info! To avoid having 2 batches of download files and 2 All downloads complete messages:

[info]: Downloading 'rust.tar.xz'
[info]: Downloading 'idf_tool_xtensa_elf_clang.libs.tar.xz'
[info]: Downloading 'xtensa-esp-elf.tar.xz'
[info]: Creating symlink between '/home/esp/.rustup/toolchains/esp/xtensa-esp32-elf-clang/esp-18.1.2_20240912/esp-clang/lib' and '/home/esp/.espup/esp-clang'
[info]: All downloads complete
[info]: Installing 'rust' component for Xtensa Rust toolchain
[info]: Downloading 'rust-src.tar.xz'
[info]: All downloads complete

I would suggest one small change:

@@ -232,6 +232,14 @@ impl Installable for XtensaRust {
             }
             let tmp_dir = tempdir_in(path)?;
             let tmp_dir_path = &tmp_dir.path().display().to_string();
+            download_file(
+                self.src_dist_url.clone(),
+                "rust-src.tar.xz",
+                tmp_dir_path,
+                true,
+                false,
+            )
+            .await?;
 
             download_file(
                 self.dist_url.clone(),
@@ -243,7 +251,6 @@ impl Installable for XtensaRust {
             .await?;
 
             info!("Installing 'rust' component for Xtensa Rust toolchain");
-
             if !Command::new("/usr/bin/env")
                 .arg("bash")
                 .arg(format!(
@@ -266,14 +273,6 @@ impl Installable for XtensaRust {
                 return Err(Error::XtensaRust);
             }
 
-            download_file(
-                self.src_dist_url.clone(),
-                "rust-src.tar.xz",
-                tmp_dir_path,
-                true,
-                false,
-            )
-            .await?;
             info!("Installing 'rust-src' component for Xtensa Rust toolchain");
             if !Command::new("/usr/bin/env")
                 .arg("bash")

This results in:

[info]: Installing GCC (xtensa-esp-elf)
[info]: Downloading 'rust-src.tar.xz'
[info]: Downloading 'idf_tool_xtensa_elf_clang.libs.tar.xz'
[info]: Downloading 'xtensa-esp-elf.tar.xz'
[info]: Downloading 'rust.tar.xz'
[info]: Creating symlink between '/home/esp/.rustup/toolchains/esp/xtensa-esp32-elf-clang/esp-18.1.2_20240912/esp-clang/lib' and '/home/esp/.espup/esp-clang'
[info]: All downloads complete
[info]: Installing 'rust' component for Xtensa Rust toolchain
[info]: Installing 'rust-src' component for Xtensa Rust toolchain

Maybe we don't even need the Donwloading... message, we could maybe leave the progress bar with the finished message, but these are just nitpicks so feel free to ignore.

Thanks again for your work!

Copy link
Member

@SergioGasquez SergioGasquez 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!

@SergioGasquez SergioGasquez merged commit 1e044c5 into esp-rs:main Dec 12, 2024
18 checks passed
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.

2 participants