download file to __dirname instead of os.tmpdir()

[zeke]

Possibly fixes electron-userland/electron-prebuilt#139

cc @mafintosh. Is this what you were suggesting?

[mafintosh]

Yea! The test would be to install this on a machine with /usr/local/bin owned by root and see if works without --unsafe-perm

[zeke]

@mafintosh I want to run this by you before I do it so I don't hose my permissions:

sudo chown root /usr/local/bin
node ./build/cli.js --version=1.4.4
sudo chown $(whoami) /usr/local/bin

Does that look right?

[mafintosh]

@zeke you wanna do it recursively with -R, sudo chown username -R /usr/local/bin

[groundwater]

Hey @zeke, I'm actually not 100% what is going wrong here. It looks like electron-prebuilt is downloaded to a temp directory, but there is an error when trying to move it?

[mafintosh]

@groundwater yea we're experimenting with getting around an install error that happens when you install using root. npm will downgrade the user to nobody when running post install scripts which can create some issues. this is an experiment to see if it is less problematic to write to the module folder instead (build scripts are allowed to do this apparently)

[zeke]

It "worked", but seems inconclusive as the same steps work on the master branch too:

$ sudo chown -R root /usr/local/bin

$ git checkout download-to-dirname
Switched to branch 'download-to-dirname'

$ rm ~/.electron/electron-v1.4.4-darwin-x64.zip

$ node ./build/cli.js --version=1.4.4          
Downloading electron-v1.4.4-darwin-x64.zip
[============================================>] 100.0% of 42 MB (3.11 MB/s)
Downloaded zip: /Users/zeke/.electron/electron-v1.4.4-darwin-x64.zip

$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.

$ npm run prepublish

> electron-download@3.0.1 prepublish /Users/zeke/electron-userland/electron-download
> babel --out-dir build lib

lib/cli.js -> build/cli.js
lib/index.js -> build/index.js

$ rm ~/.electron/electron-v1.4.4-darwin-x64.zip

$ node ./build/cli.js --version=1.4.4          
Downloading electron-v1.4.4-darwin-x64.zip
[============================================>] 100.0% of 42 MB (3.11 MB/s)
Downloaded zip: /Users/zeke/.electron/electron-v1.4.4-darwin-x64.zip

$ sudo chown -R $(whoami) /usr/local/bin

[zeke]

@mafintosh any suggestions on how to do a more conclusive test of this fix?

[develar]

👍 Downloading to tmp directory instead of __dirname (final location) is bad because in this case move can be not atomic (full copy instead of move). e.g. electron-builder downloads binaries https://github.com/electron-userland/electron-builder/blob/master/src/util/binDownload.ts#L42 to the same directory (so, rename is atomic).

[mafintosh]

@develar are you saying this is a good or bad change?

@zeke let me try and test it also

[develar]

@mafintosh It is good change. Current implementation of moveFileToCache is bad because atomic operation is not used in all cases. move from fs-extra should be not used, rename should be used instead to ensure that move operation is atomic. But it works only if source file device equals to target file device — so, __dirname (i.e. final directory) should be used instead of tmp directory.

[zeke]

Thanks for the input, @develar.

Please let me know how your testing goes, @mafintosh

[malept]

This sounds like it will also fix #39.

[develar]

I implemented atomic rename — develar@b40b6cf

Downloading to cache dir allows me to use macOS directory from VM Linux machine (Parallels shared folder).

    Unhandled rejection Error: ENOENT: no such file or directory, rename '/tmp/electron-tmp-download-30290-1479452359200/electron-v1.4.5-linux-x64.zip' -> '/home/develar/.electron/electron-v1.4.5-linux-x64.zip'
        at /media/psf/Home/Documents/electron-builder/src/packager/dirPackager.ts:34:7

After my changes it works.

I will create PR when it will be tested in the electron-builder. (electron-builder test runner downloads 6 electron versions for different platforms/archs in parallel).

[zeke]

@develar are you proposing that approach as an alternative to this PR?

[develar]

@zeke Yes. Because __dirname means in the project node_modules and it is not good (e.g. if node modules mapped to docker host volume).

[zeke]

@develar that's fine with me. Not married to a particular solution. Whatever works!