fix: support dump_syms on macOS #40

aminya · 2020-12-10T22:11:01Z

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

…a dependency

aminya · 2020-12-12T21:36:42Z

@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 · 2020-12-15T19:11:22Z

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 · 2020-12-15T19:26:36Z

@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 · 2020-12-15T19:40:59Z

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

aminya · 2020-12-15T20:07:33Z

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 · 2020-12-15T20:24:31Z

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 · 2020-12-15T20:26:33Z

Whichever method that works would be great!

nornagon · 2020-12-15T20:27:57Z

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 · 2020-12-15T20:39:04Z

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 · 2020-12-15T20:40:35Z

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

aminya · 2020-12-15T20:41:03Z

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 · 2020-12-15T20:44:31Z

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 · 2020-12-15T20:52:15Z

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 · 2020-12-15T20:55:22Z

@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.

README.md

aminya · 2020-12-15T21:02:42Z

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.

> [email protected] 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)

This reverts commit 8bddbe7.

aminya · 2020-12-15T21:07:07Z

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

aminya · 2020-12-15T21:30:20Z

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

aminya · 2020-12-16T02:05:35Z

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

nornagon · 2020-12-16T22:19:58Z

package.json

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


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"

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.

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 · 2020-12-21T02:16:24Z

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 · 2020-12-30T17:54:35Z

@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 · 2021-01-01T05:20:42Z

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 · 2021-01-01T05:45:25Z

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 · 2021-01-01T06:26:41Z

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 · 2021-01-01T18:14:28Z

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 · 2021-01-19T02:31:54Z

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

nornagon · 2021-02-01T23:46:06Z

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 · 2021-02-02T00:11:45Z

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 · 2021-02-02T00:37:04Z

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/[email protected]) 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.)

fix: submodule not being instantiated

c6f375f

aminya force-pushed the fix-build branch from 992383c to c6f375f Compare December 10, 2020 22:12

aminya changed the title ~~fix submodule not being instantiated~~ fix: submodule not being instantiated Dec 10, 2020

aminya mentioned this pull request Dec 10, 2020

Bump minidump @aminya/[email protected] atom/atom#21792

Merged

aminya force-pushed the fix-build branch from 680b862 to 7f57dd0 Compare December 10, 2020 23:40

fix: use postinstall to make sure the script runs on installation as …

06e1601

…a dependency

aminya force-pushed the fix-build branch 2 times, most recently from 59766ed to 49a2fda Compare December 11, 2020 01:08

aminya changed the title ~~fix: submodule not being instantiated~~ fix: submodule not being instantiated + support macos Dec 11, 2020

aminya force-pushed the fix-build branch 2 times, most recently from 038a912 to 03fefd3 Compare December 11, 2020 02:38

aminya added 2 commits December 15, 2020 12:17

fix: support macos - add new target and exe path

ab6210c

fix: add includes needed for mac target

a058317

aminya force-pushed the fix-build branch from 03fefd3 to a058317 Compare December 15, 2020 18:24

nornagon changed the title ~~fix: submodule not being instantiated + support macos~~ fix: submodule not being instantiated + support dump_syms on macos Dec 15, 2020

nornagon added 2 commits December 15, 2020 12:19

use xcodebuild to build dump_syms on mac

f93e26e

exit non-0 when build fails

88f85af

fix lint

e3d7d62

aminya changed the title ~~fix: submodule not being instantiated + support dump_syms on macos~~ fix: submodule not being instantiated + support dump_syms on Macos and Windows Dec 15, 2020

nornagon reviewed Dec 15, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

fix: change prepare to prepublishOnly + update readme

c7ab644

aminya force-pushed the fix-build branch from de9287e to c7ab644 Compare December 15, 2020 20:59

Revert "fix: add dump_syms support on windows"

0089bcc

This reverts commit 8bddbe7.

aminya changed the title ~~fix: submodule not being instantiated + support dump_syms on Macos and Windows~~ fix: submodule not being instantiated + support dump_syms on Macos Dec 15, 2020

aminya requested a review from nornagon December 15, 2020 21:30

nornagon reviewed Dec 16, 2020

View reviewed changes

nornagon changed the title ~~fix: submodule not being instantiated + support dump_syms on Macos~~ fix: support dump_syms on macOS Dec 16, 2020

nornagon added 3 commits December 16, 2020 14:22

Update package.json

38c7bb3

Update build.js

1c68340

Update build.js

f5f9ace

nornagon merged commit 813584e into electron:master Dec 16, 2020

aminya mentioned this pull request Apr 9, 2021

fix: chmod binaries before publishing #43

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support dump_syms on macOS #40

fix: support dump_syms on macOS #40

aminya commented Dec 10, 2020 •

edited

Loading

aminya commented Dec 12, 2020

nornagon commented Dec 15, 2020

aminya commented Dec 15, 2020 •

edited

Loading

nornagon commented Dec 15, 2020

aminya commented Dec 15, 2020 •

edited

Loading

nornagon commented Dec 15, 2020

aminya commented Dec 15, 2020

nornagon commented Dec 15, 2020

aminya commented Dec 15, 2020

nornagon commented Dec 15, 2020

aminya commented Dec 15, 2020

nornagon commented Dec 15, 2020

aminya commented Dec 15, 2020

nornagon commented Dec 15, 2020

aminya commented Dec 15, 2020 •

edited

Loading

aminya commented Dec 15, 2020

aminya commented Dec 15, 2020

aminya commented Dec 16, 2020

nornagon Dec 16, 2020

nornagon Dec 16, 2020

aminya Dec 16, 2020 •

edited

Loading

DeeDeeG commented Dec 21, 2020 •

edited

Loading

DeeDeeG commented Dec 30, 2020 •

edited

Loading

aminya commented Jan 1, 2021

DeeDeeG commented Jan 1, 2021 •

edited

Loading

aminya commented Jan 1, 2021 •

edited

Loading

DeeDeeG commented Jan 1, 2021

sadick254 commented Jan 19, 2021

nornagon commented Feb 1, 2021

DeeDeeG commented Feb 2, 2021 •

edited

Loading

DeeDeeG commented Feb 2, 2021 •

edited

Loading

fix: support dump_syms on macOS #40

fix: support dump_syms on macOS #40

Conversation

aminya commented Dec 10, 2020 • edited Loading

aminya commented Dec 12, 2020

nornagon commented Dec 15, 2020

aminya commented Dec 15, 2020 • edited Loading

nornagon commented Dec 15, 2020

aminya commented Dec 15, 2020 • edited Loading

nornagon commented Dec 15, 2020

aminya commented Dec 15, 2020

nornagon commented Dec 15, 2020

aminya commented Dec 15, 2020

nornagon commented Dec 15, 2020

aminya commented Dec 15, 2020

nornagon commented Dec 15, 2020

aminya commented Dec 15, 2020

nornagon commented Dec 15, 2020

aminya commented Dec 15, 2020 • edited Loading

aminya commented Dec 15, 2020

aminya commented Dec 15, 2020

aminya commented Dec 16, 2020

nornagon Dec 16, 2020

Choose a reason for hiding this comment

nornagon Dec 16, 2020

Choose a reason for hiding this comment

aminya Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

DeeDeeG commented Dec 21, 2020 • edited Loading

DeeDeeG commented Dec 30, 2020 • edited Loading

aminya commented Jan 1, 2021

DeeDeeG commented Jan 1, 2021 • edited Loading

aminya commented Jan 1, 2021 • edited Loading

DeeDeeG commented Jan 1, 2021

sadick254 commented Jan 19, 2021

nornagon commented Feb 1, 2021

DeeDeeG commented Feb 2, 2021 • edited Loading

DeeDeeG commented Feb 2, 2021 • edited Loading

aminya commented Dec 10, 2020 •

edited

Loading

aminya commented Dec 15, 2020 •

edited

Loading

aminya commented Dec 15, 2020 •

edited

Loading

aminya commented Dec 15, 2020 •

edited

Loading

aminya Dec 16, 2020 •

edited

Loading

DeeDeeG commented Dec 21, 2020 •

edited

Loading

DeeDeeG commented Dec 30, 2020 •

edited

Loading

DeeDeeG commented Jan 1, 2021 •

edited

Loading

aminya commented Jan 1, 2021 •

edited

Loading

DeeDeeG commented Feb 2, 2021 •

edited

Loading

DeeDeeG commented Feb 2, 2021 •

edited

Loading