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: make the cache to respect an artifact url #113

Merged
merged 1 commit into from
Aug 26, 2019
Merged

feat: make the cache to respect an artifact url #113

merged 1 commit into from
Aug 26, 2019

Conversation

alexeykuzmin
Copy link
Contributor

@alexeykuzmin alexeykuzmin commented Jul 31, 2019

The cache right now ignores multiple mirrors used simultaneously on the same machine.
I can lead to some non-obvious build issues.

This changes the cache files structure to the following:

/
  httpsgithub.comelectronelectronreleasesdownloadv5.0.9chromedriver-v5.0.9-darwin-x64.zip /
    chromedriver-v5.0.9-darwin-x64.zip
  httpsmyawesomemirror.comelectronv5.0.9chromedriver-v5.0.9-darwin-x64.zip /
    chromedriver-v5.0.9-darwin-x64.zip  // from a mirror
  httpsgithub.comelectronelectronreleasesdownloadv5.0.9electron.d.ts /
    electron.d.ts
  httpsgithub.comelectronelectronreleasesdownloadv6.0.2electron.d.ts /
    electron.d.ts  // a different version

@malept
Copy link
Member

malept commented Jul 31, 2019

Is this a breaking change?

@MarshallOfSound
Copy link
Member

MarshallOfSound commented Jul 31, 2019

Is this a breaking change?

Came here to comment this, I've got two suggestions here.

  1. If the URL is the default URL (i.e. not a mirror) we shouldn't use a url_hash folder, it should just be top level, that way we can avoid a breaking change in the cache path.
  2. Can we use something more identifiable than the cache_path like a path safe version of the mirror URL.

E.g. if the mirror url is https://mysite.com/myprefix/download/electron.zip I want the cache path to be this

/http_mysite_com_myprefix_download/electron.zip

@alexeykuzmin
Copy link
Contributor Author

  1. It doesn't look like a breaking change to me.
    A cache miss, even if a file has been previously downloaded and saved in in the cache, is a normal thing. The file could have been removed by OS or manually by a user, and it's ok to redownload it.

  2. I wanted to have a reliable way to make sure there won't be any false positive cache hits.
    Are you aware of any standard ways to convert a URL to a safe file path? The one that would respect various schemes, basic credentials, and maybe even a search query parameters.

@malept
Copy link
Member

malept commented Jul 31, 2019

A cache miss, even if a file has been previously downloaded and saved in in the cache, is a normal thing. The file could have been removed by OS or manually by a user, and it's ok to redownload it.

But then you have a bunch of largish dead files just hanging around for a user, if they've been working with Electron for a while.

@alexeykuzmin
Copy link
Contributor Author

But then you have a bunch of largish dead files just hanging around for a user, if they've been working with Electron for a while.

Files will pile up anyway once you start working with newer releases.

Removing stale files from a cache might be an issue, but I think it's out of scope of this change.

@malept
Copy link
Member

malept commented Aug 1, 2019

I would prefer that @MarshallOfSound's suggestion 1 be implemented.

Are you aware of any standard ways to convert a URL to a safe file path? The one that would respect various schemes, basic credentials, and maybe even a search query parameters.

Electron Packager uses sanitize-filename

@alexeykuzmin
Copy link
Contributor Author

@malept , @MarshallOfSound another review?

@@ -116,7 +116,7 @@ describe('Public API', () => {
artifactName: 'electron.d.ts',
});
expect(await fs.pathExists(dtsPath)).toEqual(true);
expect(path.basename(dtsPath)).toEqual('v2.0.9-electron.d.ts');
Copy link
Member

Choose a reason for hiding this comment

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

This is deliberate, all other artifacts have the version name in them. If this artifact does not it will be duplicated and overwritten right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be only overwritten if the downloader called with force: true.
electron.d.ts files for other Electron versions/mirrors will be downloaded to other folders.

The goal of this PR is to make sure every downloaded file is saved to the cache with a unique path. A folder name serves as a unique name, and a file names does not matter for caching.

@MarshallOfSound MarshallOfSound changed the title fix: make the cache to respect an artifact url feat: make the cache to respect an artifact url Aug 26, 2019
@MarshallOfSound MarshallOfSound merged commit 5661538 into electron:master Aug 26, 2019
@electron-bot
Copy link
Contributor

🎉 This PR is included in version 1.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@alexeykuzmin alexeykuzmin deleted the respect-artifact-url-in-cache branch August 26, 2019 10:54
@malept
Copy link
Member

malept commented Aug 27, 2019

Hmmm. I was doing some work on Electron Packager and saw that @electron/get was doing this:

@electron/get:cache Moving $TMPDIR/electron-v6.0.0-darwin-x64.zip to $CACHE)DIR/httpsgithub.comelectronelectronreleasesdownloadv6.0.0electron-v6.0.0-darwin-x64.zip/electron-v6.0.0-darwin-x64.zip +479ms

Doesn't that effectively create one folder per file? Wouldn't it make more sense to do folders like

$CACHE_DIR/httpsgithub.comelectronelectronreleasesdownloadv6.0.0

It just feels like a lot of folders to me... it's effectively doubling the number of nodes on the FS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants