Ability to set tmpdir to false to disable using temp directory

[develar]

I use electron-packager programmatically in electron-complete-builder.

Currently, electron-packager cannot be used in parallel —my tests failed (I use ava). Because electron-packager uses one predefined temp directory instead of random — path.join(opts.tmpdir || os.tmpdir(), 'electron-packager')

This pull request doesn't fix it. Instead, I ask you ability to disable using of temp dir. And just use specified out directly.

Why?

• If you use programmatically, you maybe already manage temp dir. For example, temp dir and out dir management (overwrite check) is not required for electron-complete-builder — it is already implemented on my side.
• In tests, you have to copy project directory to temp in any case (if you run tests in parallel), so, yet another temp directory just add complexity and slowdown.

[malept]

I think I'm missing something - what is the downside to specifying opts.tempdir for each test to be run (probably via whatever Node's equivalent of mktmpdir is)?

FYI, If we want to move forward with this PR after more information is gathered:

• existing tests need to be fixed - this probably means that the new behavior shouldn't be default. (This is my guess as to why the failures happen, after skimming the build output.)
• tests specific to the new behavior need to be written.
• docs need to be updated to mention the new special value for opts.tempdir.

[develar]

The issue is — rimraf(tempBase, cb) is called after each packager() call. So, if you run linux/mac/win tests in parallel (or electron-complete-builder will call it in parallel), base temp dir will be deleted after each test. So, other tests will be failed (because build directory was deleted).

Yes, as a workaround, I can specify tmpdir for each packager() call. But it will be a dirty hack and in this case I should fix it on electron-packager side. But since temp dir on electron-packager side is not required fro me, I want just disable this behaviour.

Originally, I was not aware of tmpdir option (it is a new option and I have discovered it only after fix was implemented (I forgot to update my fork)).

existing tests need to be fixed
Fixed.

I will add test and doc.

[malept]

I think I'm going to defer to one of the other maintainers as to whether this should be merged (pending the test/doc fixes).

[kfranqueiro]

In addition to @malept's thought about specifying a unique tmpdir for each run, I'm curious what your package does that requires performing multiple runs in parallel. If the reason is simply to run builds for multiple architectures, electron-packager already generally allows you to do that via a single command, though it still runs them in series at the moment; we could look into changing that, but the hard disk is likely going to be the bottleneck either way so I'm not sure it is of much consequence.

I might be able to fathom a performance argument for this (since it presumably avoids one set of copy operations), but I can't really see a functionality argument, or whether it's worth the complexity it adds in terms of code paths.

I suppose one question I should be asking (cc @maxogden?) is, historically, what was the main motivation for copying to tmp first to begin with? I wouldn't want to be opening a can of worms that specifically avoided.

Additionally:

• I see no tests added for the new behavior
• If the 'false' to false coersion is intended for CLI use, it might make more sense in cli.js before the packager call (I presume it couldn't be marked as a boolean to minimist since tmpdir can still be a string... unless we made this flag a separate option altogether)
• Admittedly I've only had a few minutes to look over this, but can you explain the if (opts.tmpdir === false && file === opts.out) check added to the filter function?

[develar]

In addition to @malept's thought about specifying a unique tmpdir for each run

From my point of view, it smells bad. Because instead of fixing issue in the lib (electron-packager) I fix issue on my side. i. e. "I don't want to improve underlying library".

I'm curious what your package does that requires performing multiple runs in parallel.

Currently, only tests does so. Because:

• Multiple architectures cannot be build in parallel due to native dependencies (electron-complete-builder supports native deps compilation — there is only one node_modules directory, so, to build in parallel you have to copy project directory to temp directory — but it adds unnecessary complexity (caching and so on)).
• On CI you likely build artifacts only for one platform (again, due to native deps — appveyor to build windows artifacts and travis to build OS X artifacts).

So, yes, the question is why I still ask you ability to disable temp dir usage? It is described in the "Why" section in my first comment. Currently, temp dir is not required for me at all and just adds complexity.

If the 'false' to false coersion is intended for CLI use, it might make more sense in cli.js

Thanks, fixed (will be pushed in a few minutes after test).

can you explain the if (opts.tmpdir === false && file === opts.out) check added to the filter function?

Because

ncp(opts.dir, appPath, {filter: userIgnoreFilter(opts), dereference: true}, cb)

means: copy opts.dir (~/myProject) to appPath (i.e. out dir — ~/myProject/dist/${appRelativePath}).

So, we must exclude outDir because destination folder inside source folder if we don't use temp directory (it leads to cryptic errors "filename too long" if we don't exclude it).

[malept]

Could you extract the out change into a separate PR? It needs its own tests. (Which was the main problem with #155...)

[develar]

@malept My change is under check "opts.tmpdir === false " to avoid regression. Ok, I will finish #155 (add test).

[malept]

If you would like to finish #155, even better!

[max-mapper]

I must confess I read this thread and am confused about what is actually being proposed. IMO we should just make the folder name random to support parallel builds, I think that's a valid use case. It will be a one line fix also.

[kfranqueiro]

It would be more than a one-line change. IIRC right now we are only actually deleting the tmp directory on the start of the next run, which won't know the value from the previous run. We could presumably solve that either by deleting it at the end of a run (both in cases of success and failure) or by globbing for a common prefix we'd expect to use for all tmp folders.

Given that part of the reasoning for this PR is to just allow less file copying altogether, though, my previous question of "why does electron-packager use a tmpdir to begin with and would not using one be bad" also stands.

[max-mapper]

@kfranqueiro ah good points, I guess it is non-trivial to support parallel builds in the tmpdir.

To answer your question, the tmpdir is just a staging area so that the unzip -> rename -> modify cycle can happen somewhere that won't leave an inconsistent state in the users directory in the case of a crash. E.g. if the unzip fails halfway through, or the packager crashes halfway through. If we leave the tmpdir in an inconsistent state it doesnt matter, we can just delete it. Maybe that isn't an actual concern people have, but it seemed like a good idea at the time.

Now it seems like the simplest option would be to allow the staging directory to be specified, and default it to tmpdir

[kfranqueiro]

I agree RE avoiding an inconsistent state, which is probably why I vaguely recall leaving tmpdir cleanup at the beginning of the process when I refactored stuff in #88.

Now it seems like the simplest option would be to allow the staging directory to be specified, and default it to tmpdir

Isn't that what the tmpdir option now allows? Or did you mean more like what is being proposed here, which is allow there to optionally not be a staging area at all?

[max-mapper]

@kfranqueiro Ah ok, just grokked the convo here. I am still +1 to a staging area. I'd rather have the failure state of electron-packager be to not leave a bunch of in-progress files all over a directory.

Also, good point re: the --tmpdir option, that fixes the parallel builds issue (this is the first time parallel builds have been requested). I don't think it's a hack to specify different --tmpdirs, seems perfectly reasonable to me.

So basically that judgment that needs to be made here is if its worth it to add an option that touches a lot of existing code so that @develar can avoid a copy operation in his test suite. I would personally rank the importance of a feature like this pretty low since it's a lot of effort for very little upside, but if this PR got some tests added to it then I would be OK merging it now since most of the work is already done at this point anyway.

[max-mapper]

I'd rather handle this by setting the boolean flag for this argument in the minimist options (cli.js)

[kfranqueiro]

I commented related to this earlier; I suspect using the boolean flag is unsafe because tmpdir is normally a string (the location of the staging area); it is being overloaded basically to have false treated as a special value so that the staging area can be disabled.

[develar]

Yes, cannot be marked as "boolean". If you don't like this handling, maybe it will be better to add additional option use-temp-dir (default value: true) (but tmpdir and use-temp-dir are mutually exclusive, so, I think, it is better to allow specify false)?

[malept]

Just noting that this PR is now blocked on #255.

[develar]

Since #255 is merged (thanks), I continue work on this PR. But Github doesn't add my new commit Should I open new pull request or you can somehow fix it?

[malept]

A new PR would be good.