Skip to content

fix: support dump_syms on macOS #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Dec 16, 2020
Merged

fix: support dump_syms on macOS #40

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c6f375f
fix: submodule not being instantiated
aminya Dec 10, 2020
06e1601
fix: use postinstall to make sure the script runs on installation as …
aminya Dec 10, 2020
ab6210c
fix: support macos - add new target and exe path
aminya Dec 15, 2020
a058317
fix: add includes needed for mac target
aminya Dec 15, 2020
f93e26e
use xcodebuild to build dump_syms on mac
nornagon Dec 15, 2020
88f85af
exit non-0 when build fails
nornagon Dec 15, 2020
e3d7d62
fix lint
nornagon Dec 15, 2020
8bddbe7
fix: add dump_syms support on windows
aminya Dec 15, 2020
c7ab644
fix: change prepare to prepublishOnly + update readme
aminya Dec 15, 2020
0089bcc
Revert "fix: add dump_syms support on windows"
aminya Dec 15, 2020
38c7bb3
Update package.json
nornagon Dec 16, 2020
1c68340
Update build.js
nornagon Dec 16, 2020
f5f9ace
Update build.js
nornagon Dec 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
npm install minidump
```

## Building
## Building (for development)

* `git clone --recurse-submodules https://github.com/electron/node-minidump`
* `npm install`
Expand Down
23 changes: 20 additions & 3 deletions build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,40 @@
const fs = require('fs')
const path = require('path')
const { spawnSync } = require('child_process')
const childProcess = require('child_process')

function spawnSync (...args) {
const result = childProcess.spawnSync(...args)
if (result.status !== 0) {
process.exit(result.status)
}
}

const buildDir = path.join(__dirname, 'build')
if (!fs.existsSync(buildDir)) {
fs.mkdirSync(buildDir, { recursive: true })
}

let includes = `-I${path.relative(buildDir, path.join(__dirname, 'deps'))}`

spawnSync(path.join(__dirname, 'deps', 'breakpad', 'configure'), [], {
cwd: buildDir,
env: {
...process.env,
CPPFLAGS: `-I${path.relative(buildDir, path.join(__dirname, 'deps'))}`
CPPFLAGS: includes
},
stdio: 'inherit'
})
const targets = ['src/processor/minidump_stackwalk', 'src/processor/minidump_dump']
if (process.platform === 'linux') {
targets.push('src/tools/linux/dump_syms/dump_syms')
}
spawnSync('make', ['-C', buildDir, '-j', require('os').cpus().length, ...targets], {

spawnSync('make', [includes, '-C', buildDir, '-j', require('os').cpus().length, ...targets], {
stdio: 'inherit'
})

if (process.platform === 'darwin') {
spawnSync('xcodebuild', ['-project', path.join(__dirname, 'deps', 'breakpad', 'src', 'tools', 'mac', 'dump_syms', 'dump_syms.xcodeproj'), 'build'], {
stdio: 'inherit'
})
}
2 changes: 1 addition & 1 deletion lib/minidump.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const commands = {
minidump_dump: path.resolve(__dirname, '..', 'build', 'src', 'processor', 'minidump_dump') + exe,
dump_syms: (() => {
if (process.platform === 'darwin') {
return path.resolve(__dirname, '..', 'build', 'src', 'tools', 'mac', 'dump_syms', 'dump_syms_mac')
return path.resolve(__dirname, '..', 'deps', 'breakpad', 'src', 'tools', 'mac', 'dump_syms', 'build', 'Release', 'dump_syms')
} else if (process.platform === 'linux') {
return path.resolve(__dirname, '..', 'build', 'src', 'tools', 'linux', 'dump_syms', 'dump_syms')
}
Expand Down
5 changes: 4 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@
},
"scripts": {
"test": "mocha test && standard",
"preinstall": "node build.js"
"build": "node build.js",
"submodule": "git submodule update --recursive --init",
"prepublishOnly": "npm run submodule && npm run build",
"postinstall": "npm run build"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@aminya aminya Dec 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
"devDependencies": {
"electron-download": "^3.0.1",
Expand Down
2 changes: 0 additions & 2 deletions test/minidump-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,6 @@ describe('minidump', function () {

describe('dumpSymbol()', function () {
it('calls back with a minidump', function (done) {
if (process.platform !== 'linux') return this.skip()

downloadElectron(function (error, binaryPath) {
if (error) return done(error)
minidump.dumpSymbol(binaryPath, function (error, minidump) {
Expand Down