-
-
Notifications
You must be signed in to change notification settings - Fork 300
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
show the progressbar while downloading spec tests #7242
base: unstable
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be ideal if we just show one progress bar per download which overrides / updates itself and add a label to know which progress bar belongs to which download.
@@ -89,5 +89,8 @@ | |||
"loupe": "^2.3.6", | |||
"vite": "^5.3.4", | |||
"testcontainers/**/nan": "^2.19.0" | |||
}, | |||
"dependencies": { | |||
"progress-stream": "^2.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this added here? progress-stream
should only be a dependency of spec-test-util package
@@ -132,6 +132,7 @@ | |||
"@lodestar/utils": "^1.22.0", | |||
"@lodestar/validator": "^1.22.0", | |||
"@multiformats/multiaddr": "^12.1.3", | |||
"axios": "^1.7.7", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these dependencies added here?
log(`Downloaded ${url}`); | ||
// Use pipeline to handle the stream and extract the tar | ||
await promisify(stream.pipeline)(data.pipe(progress), extractTar({ cwd: outputDir })); | ||
console.log(); // Move to the next line after the download is complete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty console log?
@@ -0,0 +1,13 @@ | |||
declare module 'progress-stream' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this custom type definition here?
In case this is rea/lly required it should be in https://github.com/ChainSafe/lodestar/tree/unstable/types folder
@@ -2,6 +2,8 @@ | |||
"extends": "./tsconfig.build.json", | |||
"compilerOptions": { | |||
"emitDeclarationOnly": false, | |||
"esModuleInterop": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modifying tsconfig should not be required
Thanks for the PR! Echoing what @nflaig said, there shouldn't be one new progress bar printed out every time there is a progress update. Ideally, we should have 3 progress bar for downloading general, mainnet and minimal. New progress should refer back to the progress bar it belongs to. I think using
is too limiting nor clean. I would looking into MultiBar from Alternatively if 3 bars are too much to manage, we can do a single bar that shows combined download progress of the 3 files. Like first calculate the combined total size of the 3 files, make a variable |
Motivation
Currently while downloading the download-spec-tests, we don't see the progress of downloads.
Description
Used progress-stream to show the progress bar while downloading the download-spec-test.
Closes #6991
Steps to test or reproduce