Bump minidump @aminya/[email protected]

[aminya]

Description of the change

Resolves the mindump issues on MacOS and Linux for Electron 9 in #21777

Verification

I fixed minidump's building for MacOS and Linux in my fork:

• The submodules were not being instantiated and built correctly.
• MacOS files were not included in the build script

My fork: https://github.com/aminya/node-minidump

I have made a PR to the original package, but it may take some time until they merge it, so I have registered it with my name temporarily.

electron/node-minidump#40

Release Notes

N/A

[aminya]

MacOS executable is not found. I don't have a Mac machine to test build and test this.

Can someone on Mac run npm install in my fork and see what is the name of the executable under build/src/tools/mac/dump_syms/
https://github.com/aminya/node-minidump

[sadick254]

Did an npm install on Mac.

        └── mac
            └── dump_syms
                └── .deps
                    └── src_tools_mac_dump_syms_dump_syms_mac-dump_syms_tool.Po

[aminya]

Thanks! Does it actually build the executable? Is this file readable or it is an executable file?

[sadick254]

The file is readable and not an executable file.

-rw-r--r--  src_tools_mac_dump_syms_dump_syms_mac-dump_syms_tool.Po 

Contents

# dummy

[aminya]

I am setting up a virtual machine to troubleshoot the issue.

[aminya]

I didn't realize that is so hard to set up a Mac virtual machine. I'm afraid I cannot troubleshoot the build script. The original developers at Electron also have not responded.

Should we just disable minidump for Mac instead until Electron people fix it?

[aminya]

@sadick254 MacOS error is fixed now. If you want you can merge my fork now, or wait until my PR merges into node-minidump.

[sadick254]

@aminya Thank you for looking into this.

[sadick254]

Am happy with using the fork. 👍 . node-minidump maintainers seem to delay doing a release with your fix.

[DeeDeeG]

Very brief summary of this comment: This PR changes the build requirements for Atom. Is there a benefit to using newer minidump? If not, can we please strongly consider reverting this? Thank you.

---

Hi folks. I should have raised this here earlier, but this PR changes the build requirements of Atom on macOS.

This changes the requirement from "either the command line tools OR XCode", to strictly requiring XCode. So at minimum it will require changes to the docs at Hacking Atom Core in the Flight Manual.

I was hoping this would be resolved upstream, but it wasn't.

I personally prefer to use the lighter "command line tools", so I was personally hoping this would not be merged to master of Atom, and reverted in the Electron 9 PR.

I notice this updated minidump package makes smaller package-name.node symbol dumps. Are they better-quality somehow? Or does it impact debugging performance in some way? (Is there a technical benefit to using the newer minidump package?) As I recall this ended up having no impact on the error we were trying to get rid of in the Electron 9 PR.

Does this have a benefit that outweighs the downside (increasing build requirements on macOS, and requiring a follow-up PR for documentation?

[DeeDeeG]

As mentioned above, this PR gives me an error when bootstrapping:

Error message (click to expand):

% script/bootstrap
Node:	v12.20.1
Npm:	v6.14.8
Python:	v2.7.16
Installing script dependencies
xcode-select: error: tool 'xcodebuild' requires Xcode, but active developer directory '/Library/Developer/CommandLineTools' is a command line tools instance
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! @aminya/[email protected] build: `node build.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the @aminya/[email protected] build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/[user]/.npm/_logs/2021-02-01T19_01_47_483Z-debug.log
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! @aminya/[email protected] postinstall: `npm run build`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the @aminya/[email protected] postinstall script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/[user]/.npm/_logs/2021-02-01T19_01_49_169Z-debug.log
child_process.js:656
    throw err;
    ^

Error: Command failed: /Users/[user]/atom/script/node_modules/.bin/npm --loglevel=error install
xcode-select: error: tool 'xcodebuild' requires Xcode, but active developer directory '/Library/Developer/CommandLineTools' is a command line tools instance
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! @aminya/[email protected] build: `node build.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the @aminya/[email protected] build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/[user]/.npm/_logs/2021-02-01T19_01_47_483Z-debug.log
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! @aminya/[email protected] postinstall: `npm run build`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the @aminya/[email protected] postinstall script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/[user]/.npm/_logs/2021-02-01T19_01_49_169Z-debug.log

    at checkExecSyncError (child_process.js:635:11)
    at Object.execFileSync (child_process.js:653:15)
    at module.exports (/Users/[user]/atom/script/lib/install-script-dependencies.js:12:16)
    at Object.<anonymous> (/Users/[user]/atom/script/bootstrap:37:1)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12)
    at internal/main/run_main_module.js:17:47 {
  status: 1,
  signal: null,
  output: [
    null,
    Buffer(38173) [Uint8Array] [
       10,  62,  32,  64,  97, 109, 105, 110, 121,  97,  47, 109,
      105, 110, 105, 100, 117, 109, 112,  64,  48,  46,  49,  57,
       46,  48,  45,  56,  32, 112, 111, 115, 116, 105, 110, 115,
      116,  97, 108, 108,  32,  47,  85, 115, 101, 114, 115,  47,
      109,  97, 116, 101,  47,  97, 116, 111, 109,  47, 115,  99,
      114, 105, 112, 116,  47, 110, 111, 100, 101,  95, 109, 111,
      100, 117, 108, 101, 115,  47, 109, 105, 110, 105, 100, 117,
      109, 112,  10,  62,  32, 110, 112, 109,  32, 114, 117, 110,
       32,  98, 117, 105,
      ... 38073 more items
    ],
    Buffer(1009) [Uint8Array] [
      120,  99, 111, 100, 101,  45, 115, 101, 108, 101,  99, 116,
       58,  32, 101, 114, 114, 111, 114,  58,  32, 116, 111, 111,
      108,  32,  39, 120,  99, 111, 100, 101,  98, 117, 105, 108,
      100,  39,  32, 114, 101, 113, 117, 105, 114, 101, 115,  32,
       88,  99, 111, 100, 101,  44,  32,  98, 117, 116,  32,  97,
       99, 116, 105, 118, 101,  32, 100, 101, 118, 101, 108, 111,
      112, 101, 114,  32, 100, 105, 114, 101,  99, 116, 111, 114,
      121,  32,  39,  47,  76, 105,  98, 114,  97, 114, 121,  47,
       68, 101, 118, 101,
      ... 909 more items
    ]
  ],
  pid: 6856,
  stdout: Buffer(38173) [Uint8Array] [
     10,  62,  32,  64,  97, 109, 105, 110, 121,  97,  47, 109,
    105, 110, 105, 100, 117, 109, 112,  64,  48,  46,  49,  57,
     46,  48,  45,  56,  32, 112, 111, 115, 116, 105, 110, 115,
    116,  97, 108, 108,  32,  47,  85, 115, 101, 114, 115,  47,
    109,  97, 116, 101,  47,  97, 116, 111, 109,  47, 115,  99,
    114, 105, 112, 116,  47, 110, 111, 100, 101,  95, 109, 111,
    100, 117, 108, 101, 115,  47, 109, 105, 110, 105, 100, 117,
    109, 112,  10,  62,  32, 110, 112, 109,  32, 114, 117, 110,
     32,  98, 117, 105,
    ... 38073 more items
  ],
  stderr: Buffer(1009) [Uint8Array] [
    120,  99, 111, 100, 101,  45, 115, 101, 108, 101,  99, 116,
     58,  32, 101, 114, 114, 111, 114,  58,  32, 116, 111, 111,
    108,  32,  39, 120,  99, 111, 100, 101,  98, 117, 105, 108,
    100,  39,  32, 114, 101, 113, 117, 105, 114, 101, 115,  32,
     88,  99, 111, 100, 101,  44,  32,  98, 117, 116,  32,  97,
     99, 116, 105, 118, 101,  32, 100, 101, 118, 101, 108, 111,
    112, 101, 114,  32, 100, 105, 114, 101,  99, 116, 111, 114,
    121,  32,  39,  47,  76, 105,  98, 114,  97, 114, 121,  47,
     68, 101, 118, 101,
    ... 909 more items
  ]
}

Can we please revert this?

Or can someone provide a strong reason why this PR is actually needed? (One that makes this PR worth the downside of changing the build requirements of Atom? Again, properly documenting this change would require a PR over at the Flight Manual repo. I'd rather not have the build requirements change and would rather not have to update the docs to reflect that if it's not providing a substantial and well-understood benefit.)

I appreciate the work that went into this, but [email protected] has been working fine for a while and doesn't require a full XCode install/doesn't require xcodebuild.

[aminya]

Instead of removing features, we can prebuild minidump. Setting up a CI for building the executable is not that hard.

[DeeDeeG]

My suggestion is toward using an older version of this package (0.9.0), which used to work on macOS just fine, but with the benefit of having easier-to-meet build requirements. (Yes, minidump 0.9.0 can dump symbols on macOS).

It also used an older version of Google's breakpad, but I'm not clear on the difference or benefit there, if there is any benefit to newer breakpad. My suggestion is arguably a "sidegrade" toward an equivalent solution, rather than a meaningful downgrade. Without being able to describe the benefit of the newer minidump version, I am inclined to focus on its downside.

[aminya]

I already made a PR for prebuilding minidump:
electron/node-minidump#42

Without my prebuild PR, the downside only affects a small demographics. The developers who want to build Atom locally and are on macOS and do not want to install XCode.

[DeeDeeG]

So I want to at least attempt to look at the pros and cons here.

This main Atom repo only uses the minidump package in script/lib/dump-symbols.js.

This file is in turn only used by script/build to dump some symbols in out/symbols.

And finally the only obvious place I see these being "used" is compressing them and uploading them as an artifact in CI/for releases. atom-mac-symbols.zip. What are these artifacts used for? Does anyone need these files? What is the use case/ how many people are using these?

If no-one is using this zipped symbol dump archive, then having newer minidump in this repo is pointless. If it's almost nobody, then it's almost pointless. And so on. I'd like to know what the atom-mac-symbols.zip is actually used for, if anything.

[DeeDeeG]

Okay, so symbol dumping was actually added here: #1618

Supposedly it is useful for debugging some crashes. Per the author who added the feature, writing in a follow-up issue that never got resolved:

zcbenz commented on Sep 8, 2014

As with #1618, we need to implement it for Windows so we can have more information of some crashes on Windows.

[aminya]

In statically compiled languages (in contrast to interpreted and JIT languages), the stack traces and error messages require symbols to be included with the binaries, so in case of an error or crash, the error message points to the original code location. Without symbols debugging any binary is impossible.

[DeeDeeG]

Thanks. That's useful context. I already vaguely understood this, but it's confusing to me because they're not actually included in any of the binaries. They're just a separate release artifact. I think users would have to download the debug symbols and put them in the right place, or manually load them in their debugger.

From the author of that PR:

This PR adds the dump-symbols task which would generate debug symbols for all the native modules, and the symbols would be uploaded to GitHub Release when publishing build.

I checked the build scripts and the symbols really do not appear to be included in the built app.

[DeeDeeG]

Reading this documentation (see link), I think the built-in Electron dev tools can debug the renderer process, and Chrome can now natively debug the main process: https://www.electronjs.org/docs/tutorial/application-debugging

Since this dump-symbols feature of Atom's build scripts is so old (February 2014) I believe this feature is from before Electron was separate from Atom. And before Chromium and Electron authors collaborated to make debugging easier.

As far as I can tell, there is no obvious way to actually use these debug symbols anymore, so I'd like to propose deleting dump-symbols from Atom's build scripts. If I'm wrong about that, then hopefully someone can provide a clearer explanation.

[aminya]

#21916 fixes this.