fix: support dump_syms on macOS

[aminya]

This fixes the scripts:

• prepare: runs before publishing or when npm install runs in the cloned repo -> makes sure that the submodules will be included in the npm package
• postinstall -> makes sure that the build.js script runs when minidump is installed as a dependency

This is blocking electron upgrade of Atom
atom/atom#21777

[aminya]

@nornagon This fixes the build for Linux. But for some reason, MacOS does not build. Could you look into this? I don't have a Mac.

[nornagon]

I'm confused, this already works on macOS.

$ git clone --recursive https://github.com/electron/node-minidump
$ cd node-minidump
$ npm install

builds just fine on my macOS machine.

What support is this adding for macOS?

[aminya]

@nornagon How does it build when the target is not added to build.js, and how do you use the package when the executable does not exist.

node-minidump/lib/minidump.js

Line 12 in 9d75f18

return path.resolve(__dirname, '..', 'build', 'src', 'tools', 'mac', 'dump_syms', 'dump_syms_mac')

https://github.visualstudio.com/Atom/_build/results?buildId=91948&view=logs&j=e2cf4b02-5697-54ad-cf7c-fc2a840d53af&t=9fd275d3-5d0a-5af8-58ae-a01a7d61dbd5&l=30

[Error: spawn /Users/runner/work/1/s/script/node_modules/minidump/build/src/tools/mac/dump_syms/dump_syms_mac ENOENT
  at Process.ChildProcess._handle.onexit (internal/child_process.js:267:19)
  at onErrorNT (internal/child_process.js:469:16)
  at processTicksAndRejections (internal/process/task_queues.js:84:21)

Emitted 'error' event on ChildProcess instance at:
  at Process.ChildProcess._handle.onexit (internal/child_process.js:273:12)
  at onErrorNT (internal/child_process.js:469:16)
  at processTicksAndRejections (internal/process/task_queues.js:84:21)
] {
  errno: 'ENOENT',
  code: 'ENOENT',
  syscall: 'spawn /Users/runner/work/1/s/script/node_modules/minidump/build/src/tools/mac/dump_syms/dump_syms_mac',
  path: '/Users/runner/work/1/s/script/node_modules/minidump/build/src/tools/mac/dump_syms/dump_syms_mac',
}

[nornagon]

@aminya oh, i see, this specifically adds dump_syms support on macOS. I've edited the PR title to specify.

[aminya]

Yes. That's what Atom uses minidump for. The dump_syms support of Mac was dropped at some point. It was working in the old versions of node-minidump.

Anyways, I don't have a Mac machine, and I was trying to fix this by live chatting with one of my friends and asking back and forth to try my branch. We have gone this far, but it is really hard for me to fix the problem fully.

This is the latest log I have got from them. From the log, it seems that it is building.
build.log

cc: @utkarshgupta137

[nornagon]

The configure && make system for dump_syms on mac in breakpad seems to be broken. In CI and on my machine I get link errors. xcodebuild with the provided XCode project in the repo seems to work OK though.

[aminya]

Whichever method that works would be great!

[nornagon]

On my machine when I check this out and run npm install, it seems like the build script is invoked twice, which is less than ideal. Can it be changed to only be invoked once?

[aminya]

On my machine when I check this out and run npm install, it seems like the build script is invoked twice, which is less than ideal. Can it be changed to only be invoked once?

We can remove prepare, but the developers should be sure to instantiate the submodulels. This only happens when developing. During produciton installation only postinstall runs.

[nornagon]

I think it's fine to just document in the README that this repo should be cloned with --recursive.

[aminya]

By the way, I noticed that breakpad already ships with Windows binaries. We can just add Windows support by adding this line:

node-minidump/lib/minidump.js

Line 15 in 9d75f18

}

    } else if (process.platform === 'win32') {
      return path.resolve(__dirname, '..', 'deps', 'breakpad', 'src', 'tools', 'windows', 'binraies', 'dump_syms.exe')
    }

[nornagon]

hmm, i'm not sure how i feel about using a random binary from the repo instead of building from source. how often is that binary updated? is it 32-bit or 64-bit?

perhaps better to support that than nothing though...

also, it's a shame to have just dump_syms and not the other minidump features on Windows.

[aminya]

For now, if you don't mind let's just include this commit for Windows. Building other features from the source can be done later.
8bddbe7

I can confirm that dump_syms.exe test passes now on Windows.

[nornagon]

@aminya CI doesn't currently run on a Windows configuration—happy to use the current test without modification, but it's not useful unless we run it! You'll need to edit .travis.yml. I'd still like that change to be in a separate PR.

As for it being "official binaries" and "trustable", that doesn't cover the full spectrum of my concerns around shipping binary blobs. But, some support is better than no support, I think, so I'm OK going with this direction for now.

[aminya]

Other tests on Windows fail, so running CI on Windows will result in red arrows. So, currently the dump_syms test pass but other ones require more work. I am afraid I can't do this in a near future. So, if you just accept this minor change for Windows, it would be good, otherwise, I will just revert the commit.

> minidump@0.19.0 test C:\Users\aminy\Documents\GitHub\JavaScript\@atom\node-minidump
> mocha test && standard



  minidump
    walkStack()
      macOS dump
        1) calls back with a report
      Windows dump
        2) calls back with a report
      Linux dump
        3) calls back with a report
    dumpSymbol()
      4) calls back with a minidump
    dump()
      5) calls back with minidump info
    moduleList()
      on a Linux dump
        √ calls back with a module list
      on a Windows dump
        √ calls back with a module list
      on a macOS dump
        √ calls back with a module list


  3 passing (7s)
  5 failing

  1) minidump walkStack() macOS dump calls back with a report:
     Uncaught Error: spawn C:\Users\aminy\Documents\GitHub\JavaScript\@atom\node-minidump\build\src\processor\minidump_stackwalk.exe ENOENT
      at Process.ChildProcess._handle.onexit (node:internal/child_process:276:19)
      at onErrorNT (node:internal/child_process:476:16)
      at processTicksAndRejections (node:internal/process/task_queues:80:21)

  2) minidump walkStack() Windows dump calls back with a report:
     Uncaught Error: spawn C:\Users\aminy\Documents\GitHub\JavaScript\@atom\node-minidump\build\src\processor\minidump_stackwalk.exe ENOENT
      at Process.ChildProcess._handle.onexit (node:internal/child_process:276:19)
      at onErrorNT (node:internal/child_process:476:16)
      at processTicksAndRejections (node:internal/process/task_queues:80:21)

  3) minidump walkStack() Linux dump calls back with a report:
     Uncaught Error: spawn C:\Users\aminy\Documents\GitHub\JavaScript\@atom\node-minidump\build\src\processor\minidump_stackwalk.exe ENOENT
      at Process.ChildProcess._handle.onexit (node:internal/child_process:276:19)
      at onErrorNT (node:internal/child_process:476:16)
      at processTicksAndRejections (node:internal/process/task_queues:80:21)

  4) minidump dumpSymbol() calls back with a minidump:
     Error: CoCreateInstance CLSID_DiaSource {E6756135-1E65-4D17-8576-610761398C3C} failed (msdia*.dll unregistered?)
Open failed

      at ChildProcess.<anonymous> (lib\minidump.js:33:25)
      at ChildProcess.emit (node:events:376:20)
      at maybeClose (node:internal/child_process:1063:16)
      at Process.ChildProcess._handle.onexit (node:internal/child_process:295:5)

  5) minidump dump() calls back with minidump info:
     Uncaught Error: spawn C:\Users\aminy\Documents\GitHub\JavaScript\@atom\node-minidump\build\src\processor\minidump_dump.exe ENOENT
      at Process.ChildProcess._handle.onexit (node:internal/child_process:276:19)
      at onErrorNT (node:internal/child_process:476:16)
      at processTicksAndRejections (node:internal/process/task_queues:80:21)

[aminya]

Sorry, the test does not actually pass, it fails with another error rather the spawn error.

[aminya]

Travis has become very slow lately. It failed to download Electron. Could you reset the failed job.

[aminya]

@nornagon Could you merge this? I reverted the Windows changes.

[nornagon]

I'm still not convinced this is correct, preinstall scripts run when the package is a dependency. I checked this with the following steps:

$ mkdir test && cd test
$ cat > package.json
{
  "dependencies": {
    "minidump": "*"
  }
}
^D
$ npm install

The preinstall script ran and the module was compiled.

Suggested change

	"build": "node build.js",
	"submodule": "git submodule update --recursive --init",
	"prepublishOnly": "npm run submodule && npm run build",
	"postinstall": "npm run build"
	"preinstall": "node build.js"

[nornagon]

I'll commit the dump_syms on macOS code for now and we can come back to the submodule/install stuff later if we need to.

[aminya]

• At the minimum, you should not remove prepublishOnly! That way, you or someone else may forgot to instantiate submodules, and they publish the package without instantiating the submodules that are the required files

• preinstall works but it does not make sense here. It works because minidump does not use any dependency in build.js. postinstall is more general. It allows bootstrapping the dependencies, and then running the build. -> Using preinstall also makes it impossible to use prepublishOnly.

[DeeDeeG]

Hi folks...

I get this error on my machine with this PR landed. It's because I don't have a full Xcode install, just the command line tools.

xcode-select: error: tool 'xcodebuild' requires Xcode, but active developer directory '/Library/Developer/CommandLineTools' is a command line tools instance

That raises the requirements for building this package beyond the usual "Xcode OR command line tools" to strictly requiring a full Xcode install, unlike most of the Node ecosystem.

---

I understand that the third-party breakpad macOS code is set up as an Xcode project, but it's going to be a breaking change for a lot of users of this package if this package fails to build all of a sudden on their macOS machines. Can there be a way to disable building this? And either save this for a significant version bump, or set building this off by default?

(Edit: Or even better, magically make it buildable with just the command line tools? If someone knows how, that would be great.)

Thanks.

[DeeDeeG]

@nornagon @aminya can you try this with newer breakpad?

There are several commits since the submodule was last synced that claim to "fix the mac build" in various ways. Including supposedly fixing the non-Xcode build. (I'm hopeful this would negate the need for calling xcodebuild, and that configure && make might work again. So that more mac users can continue to build node-minidump.)

Here's some links to review those commits: log of main branch vs the synced commit for this repo's submodule: b62101d

If updating the submodule, please sync with breakpad's main branch; I noticed that the upstream breakpad repository now has its default/active development branch configured as main, not master; master branch is still there, but it is a few commits behind now.

How to properly update a submodule: https://stackoverflow.com/a/5828396

Edit: I'm not very familiar with building C/C++ projects, but I'm trying to build this myself without Xcode. No luck so far.

Edit: There's more mac fixes in the review queue for upstream breakpad: https://chromium-review.googlesource.com/q/project:breakpad%252Fbreakpad.

[aminya]

Updating the breakpad submodule is a separate issue.

You should have xcode to build Mac apps. If you want to write a manual make file, go ahead. But I don't think make comes on Mac without xcode anyway. So not sure if it has any benefits.

[DeeDeeG]

Xcode is the full graphical IDE app. You can get all the basic stuff (like make, gcc, g++, and clang) in something called the "command line tools" package. (See: https://developer.apple.com/library/archive/technotes/tn2339/_index.html#//apple_ref/doc/uid/DTS40014588-CH1-WHAT_IS_THE_COMMAND_LINE_TOOLS_PACKAGE_)

Regarding this PR: I'm only missing the xcodebuild binary. I prefer not to download and install a multi-gigabytes-large IDE (Xcode) just to get xcodebuild so I can build this one package. I've been contributing to various Node projects, including Atom, and I have not needed the full Xcode install yet.

I can open a separate issue about this, I guess... But my point is: it's inconvenient to need full Xcode just for this feature no-one has needed yet. (I noticed that Atom doesn't need this feature to fix its CI issue; it was actually a problem with outdated fswin. The error message was simply manually ignored... atom/atom@3dd68bb)

[aminya]

dump_syms

Before this PR the dump_syms executable simply "did not exist".

xcodebuild

Instead of banning everyone else that has xcodebuld from using dump_syms on MacOs, you'd better set up a multi-os CI that builds the binaries and includes them in the package.

breakpad already includes Windows binaries, so there is no need for rebuilding them in the CI.

Something like this that I do for the native build addons:

https://github.com/atom-ide-community/zadeh/blob/00b28b8f7e5d405030e71cc1672e8a94a611a8de/.github/workflows/CI.yml#L70-L73

The build script needs to change once the binaries are included:

if (!dump_syms_exists) {
   // then build
}

Atom

After this PR, that fswin error popped up. So, I recommended sadick to ignore that error. We can wrap dump_syms function in a try-catch as it is an optional operation.

atom/text-buffer#336 (comment)

[DeeDeeG]

set up a multi-os CI that builds the binaries and includes them in the package.

This would be a good solution for folks like me, if the electron team is interested in it. (Or make it configurable: look for an environment variable such as NODE_MINIDUMP_NO_BUILD_DUMP_SYMS_MACOS to toggle off building this binary).

[sadick254]

@nornagon When can we expect a release with this fix?

[nornagon]

In addition to the prebuilt binaries, I'd love to have a build that works without xcodebuild. @DeeDeeG if you're interested in preparing a PR, I'd be happy to review it.

[DeeDeeG]

Thanks for the ping, but I'm only slightly acquainted with C/C++, so the chances I can fix this any time soon seem kind of slim at the moment. (I'm not sure exactly how to fully read the build scripts at the moment, so fixing them seems beyond reach.)

For what it's worth, I tried (in my own not-acquainted-with-C way) building things in the latest breakpad without XCode, but I didn't get very far with that. Maybe it's doable and I just don't know what I'm doing. But if I recall correctly configure and make weren't enough.

[DeeDeeG]

I can reproduce that npm install @aminya/minidump works for me though, without full XCode installed.

(Tested and confirmed installable with @aminya/minidump versions 0.19.0-9 and 0.19.0-10. I find that the older version 0.19.0-8 still tries to build and fails if user doesn't have full XCode --> xcodebuild.)

And Atom's CI appears to validate that his fork (@aminya/minidump@0.19.0-8) produces symbol dumps on macOS. (Well, since I think I figured out today that Atom doesn't use the symbol dumps it produces, there's no step confirming that they're valid in Atom's build scripts. It's validated that they're produced at all, at least.)