-
Notifications
You must be signed in to change notification settings - Fork 262
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
Recognize electron as a valid runtime #175
Conversation
The clang-3.5 doesn't seem to be available on Travis anymore.
bless u @zcbenz, bless u |
I'd love if you released it :) We have issues with building electron project because fsevents 1.0.0 depends on node-pre-gyp. |
@sheerun In the short term, |
/cc @springmeyer for review, this solves modules depending on node-pre-gyp not building for Electron, making many important modules like node-sqlite3 being able to be used in Electron without hacks. |
Thank you for this @zcbenz - will merge and release now. Therefore node-pre-gyp |
Recognize electron as a valid runtime
|
Thank you guys for this. <3 |
Bless you all. 🎆 🎆 🎆 |
Yesterday I made a script to fix this issue. To feel better I'm pasting it here :) #!/usr/bin/env bash
for path in $(find . -name package.json -type f -follow | xargs grep '"fsevents":' -l | sort | uniq); do
(
cd "$(dirname "$path")"
VERSION=$(node -e "console.log(require('./node_modules/fsevents/package').version)" 2> /dev/null)
if [[ "$VERSION" != "0.3.8" ]]; then
echo "FIXING: $(dirname "$path")"
npm uninstall --save fsevents
npm uninstall --save-optional fsevents
npm install --save fsevents@0.3.8
fi
)
done |
If someone needs Electron detection (like Atom package which requires |
@zcbenz: One project I use (node-sqlite3) require electron set as runtime when importing AddOn (https://github.com/mapbox/node-sqlite3/blob/master/lib/sqlite3.js#L3). I've created a PR that adresses this issue (#177). |
@@ -55,6 +69,8 @@ function get_runtime_abi(runtime, target_version) { | |||
} | |||
if (runtime === 'node-webkit') { | |||
return get_node_webkit_abi(runtime, target_version || process.versions['node-webkit']); | |||
} else if (runtime === 'electron') { | |||
return get_electron_abi(runtime, target_version || process.versions.electron); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the BKM for getting my runtime & target_version in, if I have a project that uses a package which consumes node-pre-gyp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, best-known-method. preferred method, really*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the policy that https://github.com/electronjs/electron-rebuild uses is, assume your package is a sibling package of electron-prebuilt
, then use https://github.com/electronjs/electron-rebuild/blob/master/src/main.js#L42 to get the version info.
This isn't perfect, but it makes the Easy case Easy, and if you let devs also specify explicitly, you have an escape hatch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm. so if I'm on electron-rebuild@1.0.1
and running it on postinstall
, it should set it for me, eh? curious why I still get the Error: Empty target version is not supported if electron...
error from above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will if electron-prebuilt
is in the same folder as electron-rebuild
in your node_modules
folder. If not, nbd, you can use the cmd line flags to work around it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @paulcbetts, really appreciate it. perhaps you'd be willing to help me ID what I've missed. electron-prebuilt and electron-rebuild are good to go, it would seem.
cdieringer@Snapper-osx:~/node/coinstac$ ll node_modules/ | grep electron
drwxr-xr-x 15 cdieringer staff 510 Oct 12 13:36 electron-prebuilt/
drwxr-xr-x 10 cdieringer staff 340 Oct 12 14:03 electron-rebuild/
cdieringer@Snapper-osx:~/node/coinstac$ cat package.json | grep electron
"electron-prebuilt": "^0.33.7",
"electron-rebuild": "^1.0.1",
however, it still yields the following, against expectation:
...
> fsevents@1.0.2 install /Users/cdieringer/node/coinstac/node_modules/babel/node_modules/chokidar/node_modules/fsevents
> node-pre-gyp install --fallback-to-build
node-pre-gyp ERR! install error
node-pre-gyp ERR! stack Error: Empty target version is not supported if electron is the target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this is a different error than I thought it was - I'll look into this. Can you file a bug on electronjs/electron-rebuild?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, id be happy to. i might spend a little time looking into it tonight. i'd really like to give back somehow to a project that has helped me so much!
This doesn't make node-pre-gyp fully support Electron, but this makes
node-pre-gyp install --fallback-to-build
work for Electron whennpm_config_runtime
is specified, so modules relying on node-pre-gyp can be compiled against Electron by building from source.I'll try to add full support for Electron in other PRs.