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 cached releases to standard directories #24

Closed
Siilwyn opened this issue Aug 30, 2016 · 9 comments
Closed

Save cached releases to standard directories #24

Siilwyn opened this issue Aug 30, 2016 · 9 comments
Labels
✨ enhancement New feature or request

Comments

@Siilwyn
Copy link
Contributor

Siilwyn commented Aug 30, 2016

Instead of placing the release archives in the home directory they should be placed in the appropriate directory per OS. I've done the same for Greenkeeper's RC so I'd like to create a PR for it.

Before doing that I'd like to know if there are any special things I should take into consideration and if the core contributors are on board with this so I don't do it in vain. :)

@kevinsawicki kevinsawicki added the ✨ enhancement New feature or request label Sep 2, 2016
@malept
Copy link
Member

malept commented Sep 6, 2016

What do you propose to do with the existing cache directory? Is that still taken into account?

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Sep 6, 2016

We could use the old location if it exists or move the old location to the new location. At Greenkeeper the moving is done. Both have their own pros and cons.

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Oct 13, 2016

@malept any preference for one of the solutions?

@malept
Copy link
Member

malept commented Oct 13, 2016

I would prefer moving it.

@develar
Copy link
Contributor

develar commented Apr 9, 2017

@Siilwyn I see error in your merged changes, that leads to #47 (comment)

Code

const oldLocation = path.join(os.homedir(), './.electron', this.filename)
    if (pathExists.sync(oldLocation)) {
      return oldLocation
    }

in the cachedZip

But! We call cachedZip in the ensureCacheDir twice — one for check, and another one in the call downloadZip. So, 2 sync calls are performed, and latter can lead to race conditions — downloadZip must download only to a new location.

In version of electron-download that used by electron-builder, I will remove this code for now.

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Apr 9, 2017

Oh that is bad. :/

@develar could you expand your explanation? I don't quite follow: cachedZip is not being called directly from ensureCacheDir and downloadZip is called once.

@develar
Copy link
Contributor

develar commented Apr 9, 2017

@Siilwyn downloadZip calls cachedZip, but cachedZip will check and return old location. Not evident how does it lead to error, but in any case it is not good — cachedZip should return constant result (based on options) and old location should be checked in another place.

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Apr 9, 2017

The function that checks if the zip file is already cached sounds like the appropriate place for this check to me. Where would you place it?

Edit: I replicated the error @develar, will debug it!
Edit 2: might have to do with the checksum file location handling.
Edit 3: definitely something to do with checksum validating, downloading a version below 1.3.2 works fine.

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Apr 9, 2017

@develar moving the logic to the directory location seems to fix it. Could you confirm this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants