Skip to content
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.

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

Remove apm's node-gyp dependency #19189

Merged
merged 26 commits into from May 13, 2019

Conversation

Projects
None yet
3 participants
@50Wliu
Copy link
Member

commented Apr 20, 2019

None of the templates really apply here.

apm has long used a pinned version of node-gyp to install Node (or rather, Electron). The intent was always to update node-gyp every time npm was updated; however, this never happened and apm's version of node-gyp has become significantly dated as a result.

We now attempt to go back to how it was before and directly depend on npm's version of node-gyp.

For more information, see atom/apm#832.

Fixes #18540?

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

/bin/sh: /Users/vsts/agent/2.150.0/work/1/s/apm/node_modules/atom-package-manager/script/postinstall.sh: Permission denied
Hmm...not sure how to fix this as all the permissions look correct on my end.

50Wliu added some commits Apr 21, 2019

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2019

☝️ turns out you need to publish on a Unix machine in order for file permissions to stick around...the build's green now, though 😀

@vinkla

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2019

I've tried running this locally on macOS and it seems to fix #18540. I'm no longer getting error messages from node-gyp.

@vinkla vinkla referenced this pull request Apr 21, 2019

Closed

Mac: Script/bootstrap fails to install apm #18540

1 of 1 task complete
@rafeca

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Thanks!! 🚀

@50Wliu: Based on atom/apm#832 comments, please update this PR once the stable release of apm is published

@rafeca rafeca self-assigned this Apr 26, 2019

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

🤦‍♂ forgot to publish from WSL again

50Wliu added some commits Apr 26, 2019

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

@rafeca I'm getting consistent timeouts on the x86 Windows build when running specs, and I'm not sure why. script\test doesn't even use apm.

@rafeca

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

That's strange 🤷‍♂️ I'm gonna take a look at it, but I have no clue why would this happen 😟

@rafeca rafeca force-pushed the wl-update-apm branch from e83b1f2 to f603ced May 2, 2019

@rafeca rafeca force-pushed the wl-update-apm branch from f603ced to bd05990 May 2, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

It seems like somewhere in the start.js script the Atom executable gets halted, I'm adding some console.log statements to figure out where this is happening...

@rafeca

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@50Wliu looks like the process gets stalled between these lines:

console.log('START ATOM!')
const {app} = require('electron')
const nslog = require('nslog')
const path = require('path')
const temp = require('temp').track()
const parseCommandLine = require('./parse-command-line')
const startCrashReporter = require('../crash-reporter-start')
const atomPaths = require('../atom-paths')
const fs = require('fs')
const CSON = require('season')
const Config = require('../config')
console.log('everything required correctly!')

I'm adding more specific logging to see which is the module that's causing it

@rafeca

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

After adding the debugging messages this is what gets logged in the CI now:

-- requiring module:  electron from D:\a\1\s\src\main-process\start.js
-- requiring module:  ../app from D:\a\1\s\out\Atom Dev\resources\electron.asar\browser\api\exports\electron.js
-- requiring module:  nslog from D:\a\1\s\src\main-process\start.js
-- requiring module:  ../build/Release/nslog.node from D:\a\1\s\node_modules\nslog\lib\nslog.js
-- requiring module:  electron from D:\a\1\s\out\Atom Dev\resources\electron.asar\browser\init.js
-- requiring module:  ../dialog from D:\a\1\s\out\Atom Dev\resources\electron.asar\browser\api\exports\electron.js
-- requiring module:  electron from D:\a\1\s\out\Atom Dev\resources\electron.asar\browser\api\dialog.js
-- requiring module:  ../app from D:\a\1\s\out\Atom Dev\resources\electron.asar\browser\api\exports\electron.js
-- requiring module:  ../browser-window from D:\a\1\s\out\Atom Dev\resources\electron.asar\browser\api\exports\electron.js

(source).

The browser-window.js file seems quite inoffensive, so this may not be telling us much about the root cause of the issue... Sorry if I haven't been able to provide much help here

50Wliu added some commits May 2, 2019

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Oh, my hunch is that we're attempting to show a dialog box because of a failure, and then hanging after the dialog is successfully displayed.

On the Windows x64 build, here's what the require chain looks like:
windows-x64-requires

On the hanging Windows x86 build, here's what the require chain looks like:
windows-x86-requires

I expect apm was unable to correctly rebuild nslog's dependencies.

@50Wliu

This comment has been minimized.

50Wliu added some commits May 3, 2019

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

Error: \\?\D:\a\1\s\node_modules\nslog\build\Release\nslog.node is not a valid Win32 application

@rafeca

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

Good find @50Wliu !

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

@rafeca I think I'm going to try adding back the node-gyp dependency but updating it to 3.7.0, the latest version. Let me know if that sounds like a good plan to you.

50Wliu added some commits May 7, 2019

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

50Wliu added some commits May 7, 2019

⬆️ apm@2.2.2
Also contains another fix for permissions

@50Wliu 50Wliu force-pushed the wl-update-apm branch from 31255ec to c37239b May 8, 2019

@@ -13,7 +13,7 @@ module.exports = function (packagePath, ci, stdioOptions) {
installEnv.npm_config_target = CONFIG.appMetadata.electronVersion
childProcess.execFileSync(
CONFIG.getApmBinPath(),
['--loglevel=error', ci ? 'ci' : 'install'],
[ci ? 'ci' : 'install'],

This comment has been minimized.

Copy link
@50Wliu

50Wliu May 8, 2019

Author Member

(apm doesn't recognize the --loglevel option)

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Green! @rafeca think this is ready to go?

@rafeca

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

Wow! Awesome job @50Wliu !

How risky do you think is merging this? I'm asking because on Friday we're planning to roll the railcars for the next Atom release, so if there's a chance it'll break things (or native modules) I'd rather wait until Monday to merge it to minimize the potential release blockers.

@rafeca

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

LOL! why does my comment appear above yours? 🤯

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Yeah, we can totally wait until Monday to be safe.

50Wliu added some commits May 11, 2019

@50Wliu 50Wliu merged commit 136c36c into master May 13, 2019

2 checks passed

Atom Pull Requests #20190512.1 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@50Wliu 50Wliu deleted the wl-update-apm branch May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.