LinusU / node-appdmg

💾 Generate your app dmgs
MIT License
1.68k stars 152 forks source link

hdiutil exit code 1, tmp file not tracked & destroyed #63

Closed krainboltgreene closed 9 years ago

krainboltgreene commented 9 years ago

So this has come up in #47 and #61 but was unresolved realistically. Restarting didn't solve my problem so I dug deeper. I attempted to recreate the command sent to hdiutil and this happened:

▶ hdiutil create /var/folders/w2/xf402_nd0pd395xy_d7276yh0000gn/T/115516-3052-1mykps3.dmg -fs FS+ -size 183.5m -volname Lowbrow -debug -verbose
create-content-spec:
    nbi-spec:
        volname: Lowbrow
        filesystem: FS+
verbose: true
debug: true
agent: hdiutil
create-target-spec:
    url: file:///var/folders/w2/xf402_nd0pd395xy_d7276yh0000gn/T/115516-3052-1mykps3.dmg
    sectors: 375808
    image-options:
...
DIHLDiskImageCreate() returned -48
hdiutil: create: returning -48
hdiutil: create failed - File exists

I believe one of the issues at play here is that node-appdmg doesn't track tempfiles as suggested here: https://github.com/bruce/node-temp#want-cleanup-make-sure-you-ask-for-it

krainboltgreene commented 9 years ago

In addition to making it track files I heavily suggest you also give a public API (preferably an ENV based one) that allows me to enable verbose/debug flags.

krainboltgreene commented 9 years ago

So after fixing the track() issue I continued to receive errors, I modified node-appdmg to get better information:

- Starting build for ´macos´ -
Writing temporary ´appdmg.json´
Wrote temporary ´appdmg.json´
Kicking off ´appdmg´
appdmg: [1] Looking for target
appdmg: [2] Reading JSON Specification
appdmg: [3] Parsing JSON Specification
appdmg: [4] Validating JSON Specification
appdmg: [5] Looking for files
appdmg: [6] Calculating size of image
appdmg: [7] Creating temporary image
Erroring out at create
[ 'create',
  '/var/folders/w2/xf402_nd0pd395xy_d7276yh0000gn/T/115516-4733-17s27ku.dmg',
  '-fs',
  'HFS+',
  '-size',
  '183.5m',
  '-volname',
  'Lowbrow' ]
null
appdmg: [8] Mounting temporary image
Erroring out at attach
[ 'attach',
  '/var/folders/w2/xf402_nd0pd395xy_d7276yh0000gn/T/115516-4733-17s27ku.dmg',
  '-nobrowse',
  '-noverify',
  '-noautoopen' ]
null
appdmg: [9] Making hidden background folder
appdmg: [10] Copying background
appdmg: [11] Reading background dimensions
appdmg: [12] Copying icon
appdmg: [13] Setting icon
appdmg: [14] Creating links
appdmg: [15] Copying files
appdmg: [16] Making all the visuals
appdmg: [17] Blessing image
appdmg: [18] Unmounting temporary image
Erroring out at detach
[ 'detach', '/Volumes/Lowbrow', '-debug' ]
null
appdmg: [19] Finalizing image
Erroring out at convert
[ 'convert',
  '/var/folders/w2/xf402_nd0pd395xy_d7276yh0000gn/T/115516-4733-17s27ku.dmg',
  '-format',
  'UDZO',
  '-imagekey',
  'zlib-level=9',
  '-o',
  '/Users/krainboltgreene/Code/krainboltgreene/lowbrow/releases/package/osx/x64/Lowbrow.dmg' ]
{ [Error: Error running `hdiutil`! Exit code was 1]
  exitCode: 1,
  stdout: 'Preparing imaging engine…\nReading Protective Master Boot Record (MBR : 0)…\n',
  stderr: '\nhdiutil: convert failed - Invalid argument\n' }
appdmg: [20] Removing temporary image
/Users/krainboltgreene/Code/krainboltgreene/lowbrow/node_modules/electron-builder/cli.js:30
    throw error;
          ^
Error: Error running `hdiutil`! Exit code was 1
    at ChildProcess.<anonymous> (/Users/krainboltgreene/Code/krainboltgreene/lowbrow/node_modules/electron-builder/node_modules/appdmg/lib/util.js:32:17)
    at ChildProcess.emit (events.js:110:17)
    at Process.ChildProcess._handle.onexit (child_process.js:1067:12)

Inside the error callback I print out the name of the command, the args, and the err object.

There are two interesting things going on:

  1. Error callbacks running despite a null value for err
  2. '\nhdiutil: convert failed - Invalid argument\n'

This should have been the error message that popped up, by the way.

LinusU commented 9 years ago

One problem was that I assumed that if hdiutil errored out, it wouldn't have left any files behind. This assumption didn't hold up and I have fixed that in fe19471 which is released as 0.3.2.

  1. Error callbacks running despite a null value for err

Where is this happening? Care to show what modifications you have done to the code?

hdiutil: convert failed - Invalid argument

This is the interesting part... Does the directory /Users/krainboltgreene/Code/krainboltgreene/lowbrow/releases/package/osx/x64/ exists?

krainboltgreene commented 9 years ago

Where is this happening? Care to show what modifications you have done to the code?

Here's the modifications I've made, first here to fix the tmp issue:

var temp = require('temp').track();
var util = require('./util');

And also I added these logs to each of the actions:

exports.convert = function (source, target, cb) {
  var args = [];
  args.push('convert', source);
  args.push('-format', 'UDZO');
  args.push('-imagekey', 'zlib-level=9');
  args.push('-o', target);
  util.sh('hdiutil', args, function (err) {
    console.log("Erroring out at convert");
    console.log(args);
    console.log(err);
    cb(err, ( err ? undefined : target ));
  });
};

This is the interesting part... Does the directory /Users/krainboltgreene/Code/krainboltgreene/lowbrow/releases/package/osx/x64/ exists?

No, in fact releases/package/ doesn't exist, which is very odd, because before it was creating that directory.

krainboltgreene commented 9 years ago

Actually, no, that's incorrect, the issue is that (on a higher level) I was passing in a directory that didn't exist. So ultimately what we have here is a very low level issue (hdiutil getting a bad parameter, a file that doesn't exist), that bubbles up as a very generic error (hdiutil exit code 1).

krainboltgreene commented 9 years ago

...Or not, argh. I thought I had found the error but no, it wasn't because I was passing a incorrect file path. I wonder why it's behaving differently.

LinusU commented 9 years ago
  1. Error callbacks running despite a null value for err

This is how node.js works, when the work is done the callback is called. If an error occurred during the work, the err parameter holds an error, otherwise it holds null.


Could you try to apple this patch to hdituil.js

 var fs = require('fs');
 var temp = require('temp');
 var util = require('./util');
+var mkdirp = require('mkdirp');

 exports.convert = function (source, target, cb) {
+  mkdirp.sync(target);
   var args = [];
   args.push('convert', source);
   args.push('-format', 'UDZO');

And run npm install mkdirp, then try and run again.

krainboltgreene commented 9 years ago

Added the require, added the call to convert, added the dependency, installed the dependency. Here's what happens:

- Starting build for ´macos´ -
Writing temporary ´appdmg.json´
Wrote temporary ´appdmg.json´
Kicking off ´appdmg´
appdmg: [1] Looking for target
appdmg: [2] Reading JSON Specification
appdmg: [3] Parsing JSON Specification
appdmg: [4] Validating JSON Specification
appdmg: [5] Looking for files
appdmg: [6] Calculating size of image
appdmg: [7] Creating temporary image
appdmg: [8] Mounting temporary image
appdmg: [9] Making hidden background folder
appdmg: [10] Copying background
appdmg: [11] Reading background dimensions
appdmg: [12] Copying icon
appdmg: [13] Setting icon
appdmg: [14] Creating links
appdmg: [15] Copying files
appdmg: [16] Making all the visuals
appdmg: [17] Blessing image
appdmg: [18] Unmounting temporary image
appdmg: [19] Finalizing image
Erroring out at convert
[ 'convert',
  '/var/folders/w2/xf402_nd0pd395xy_d7276yh0000gn/T/115517-3387-1vdswr3.dmg',
  '-format',
  'UDZO',
  '-imagekey',
  'zlib-level=9',
  '-o',
  '/Users/krainboltgreene/Code/krainboltgreene/lowbrow/releases/package/osx/x64/Lowbrow.dmg' ]
{ [Error: Error running `hdiutil`! Exit code was 1]
  exitCode: 1,
  stdout: 'Preparing imaging engine…\nReading Protective Master Boot Record (MBR : 0)…\n',
  stderr: '\nhdiutil: convert failed - File exists\n' }
appdmg: [20] Removing temporary image
/Users/krainboltgreene/Code/krainboltgreene/lowbrow/node_modules/electron-builder/cli.js:30
    throw error;
          ^
Error: Error running `hdiutil`! Exit code was 1
    at ChildProcess.<anonymous> (/Users/krainboltgreene/Code/krainboltgreene/lowbrow/node_modules/electron-builder/node_modules/appdmg/lib/util.js:32:17)
    at ChildProcess.emit (events.js:110:17)
    at Process.ChildProcess._handle.onexit (child_process.js:1067:12)
krainboltgreene commented 9 years ago

(I also moved my logs to if(err) blocks)

LinusU commented 9 years ago

Oops, I made a small error there.

Add

var path = require('path')

And change to:

mkdirp.sync(path.dirname(target))

Also make sure to clean out any existing files...

krainboltgreene commented 9 years ago

Worked! Should I tell the electron-packager people to upgrade?

LinusU commented 9 years ago

Hmm, I'm actually not sure if it should be appdmgs responsibility to make sure that the output folder exists. I don't know of any other unix commands that does it. E.g.

$ echo 'test' > /tmp/no-exists/my-file 
zsh: no such file or directory: /tmp/no-exists/my-file

$ touch /tmp/no-exists/my-file 
touch: /tmp/no-exists/my-file: No such file or directory

$ cp linus.jpg /tmp/no-exists/my-file
cp: /tmp/no-exists/my-file: No such file or directory

$ cat test.c 
int main(void) { return 0; }
$ gcc -o /tmp/no-exists/my-file test.c 
ld: can't open output file for writing: /tmp/no-exists/my-file, errno=2 for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Now, I would like a much better error output thought. I think that the best way here is to try and create the file earlier on, and then raise helpful error if something goes wrong, and then tell hdiutil to overwrite the created empty file. This prevents ant race conditions that other approaches might have (e.g. checking if the directory exists, then starting hdiutil).

krainboltgreene commented 9 years ago

Agreed, tar doesn't create the directory sadly. I have to contribute to electron-builder anywys, so I'll add that.

On Thu, Jun 18, 2015 at 3:14 AM, Linus Unnebäck notifications@github.com wrote:

Hmm, I'm actually not sure if it should be appdmgs responsibility to make sure that the output folder exists. I don't know of any other unix commands that does it. E.g.

$ echo 'test' > /tmp/no-exists/my-file zsh: no such file or directory: /tmp/no-exists/my-file

$ touch /tmp/no-exists/my-file touch: /tmp/no-exists/my-file: No such file or directory

$ cp linus.jpg /tmp/no-exists/my-file cp: /tmp/no-exists/my-file: No such file or directory

$ cat test.c int main(void) { return 0; } $ gcc -o /tmp/no-exists/my-file test.c ld: can't open output file for writing: /tmp/no-exists/my-file, errno=2 for architecture x86_64 clang: error: linker command failed with exit code 1 (use -v to see invocation)

Now, I would like a much better error output thought. I think that the best way here is to try and create the file earlier on, and then raise helpful error if something goes wrong, and then tell hdiutil to overwrite the created empty file. This prevents ant race conditions that other approaches might have (e.g. checking if the directory exists, then starting hdiutil).

— Reply to this email directly or view it on GitHub https://github.com/LinusU/node-appdmg/issues/63#issuecomment-113070197.

Kurtis Rainbolt-Greene, Hacker Software Developer 1631 8th St. New Orleans, LA 70115

LinusU commented 9 years ago

Nice! Do you have anything committed yet? Let me know if there is anything you need help with, I would love to answer questions, review code, etc :+1: