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

Save downloaded DB archives to disk before unzipping #700

Merged
merged 2 commits into from
Dec 14, 2020

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Dec 7, 2020

This fixes two classes of DBs that can't be installed directly from
downloading:

  1. DBs whose central directories do not align with their file headers.
    We need to download and save the entire archive before we can read
    the central directory and use that to guide the unzipping.
  2. Large DBs require too much memory so can't be downloaded and unzipped
    in a single stream.

We also add proper progress notifications to the download progress
monitor so users are aware of how many more MBs are left to download.

It's not yet possible to do the same for unzipping using the current
unzipper library, since unzipping using the central directory does not
expose a stream.

Fixes #621
Fixes #622

Integration tests are passing and added two more for progress monitoring.

Manually tested on the linux database from lgtm https://lgtm.com/projects/g/torvalds/linux/ci and it is working.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [n/a] @github/docs-content-dsp has been cc'd in all issues for UI or other user-facing changes made by this pull request.

@aeisenberg aeisenberg marked this pull request as draft December 7, 2020 23:38
This fixes two classes of DBs that can't be installed directly from
downloading:

1. DBs whose central directories do not align with their file headers.
   We need to download and save the entire archive  before we can read
   the central directory and use that to guide the unzipping.
2. Large DBs require too much memory so can't be downloaded and unzipped
   in a single stream.

We also add proper progress notifications to the download progress
monitor so users are aware of how many more MBs are left to download.

It's not yet possible to do the same for unzipping using the current
unzipper library, since unzipping using the central directory does not
expose a stream.
@aeisenberg aeisenberg force-pushed the aeisenberg/download-to-file branch from 9571223 to 747ff45 Compare December 8, 2020 00:25
@aeisenberg aeisenberg marked this pull request as ready for review December 8, 2020 00:27
@alexet alexet self-requested a review December 14, 2020 16:22
Copy link
Contributor

@alexet alexet left a comment

Choose a reason for hiding this comment

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

LGTM. I tried to resolve the merge conflict in the ui but have realised it would have probably been cleaner for you to rebase instead. Feel free to remove my merge commit and rebase instead.

@alexet
Copy link
Contributor

alexet commented Dec 14, 2020

Actually squash and merge will do the right thing.

@alexet alexet merged commit 9ffb3a1 into github:main Dec 14, 2020
@aeisenberg aeisenberg deleted the aeisenberg/download-to-file branch December 14, 2020 16:26
@aeisenberg
Copy link
Contributor Author

Thanks for the review!

aofaof0907 pushed a commit to aofaof0907/vscode-codeql that referenced this pull request Jul 27, 2021
This fixes two classes of DBs that can't be installed directly from
downloading:

1. DBs whose central directories do not align with their file headers.
   We need to download and save the entire archive  before we can read
   the central directory and use that to guide the unzipping.
2. Large DBs require too much memory so can't be downloaded and unzipped
   in a single stream.

We also add proper progress notifications to the download progress
monitor so users are aware of how many more MBs are left to download.

It's not yet possible to do the same for unzipping using the current
unzipper library, since unzipping using the central directory does not
expose a stream.

Co-authored-by: Alexander Eyers-Taylor <[email protected]>
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.

Unable to Download from LGTM for certain databases. Importing a .zip database kills the extension
2 participants