Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

fixes #531 #536

Closed
wants to merge 1 commit into from
Closed

fixes #531 #536

wants to merge 1 commit into from

Conversation

hedefalk
Copy link

Looking at mapbox/node-pre-gyp@ef79b3f (which doesn't work for apm) I got the feeling that just stating runtime=electron would be a pretty good idea. And it was.

Tried it by simply running:

"/Applications/Atom Beta.app/Contents/Resources/app/apm/bin/node" "/Applications/Atom Beta.app/Contents/Resources/app/apm/node_modules/npm/bin/npm-cli.js" "--globalconfig" "/Users/viktor/.atom/.apm/.apmrc" "--userconfig" "/Users/viktor/.atom/.apmrc" "rebuild" "--target=0.34.5" "--arch=x64" "--runtime=electron"

Without --runtime=electron, node-pre-gyp fails like

@hedefalk
Copy link
Author

Can someone give me some hints on what testing I should produce to get this accepted?

Currently apm rebuild simply doesn't work for any package that uses fs-events since node-pre-gyp just blows up. If I just add --runtime=electron to /Applications/Atom.app/Contents/Resources/app/apm/lib/rebuild.js:

rebuildArgs = ['--globalconfig', config.getGlobalConfigPath(), '--runtime=electron', '--userconfig', config.getUserConfigPath(), 'rebuild', "--target=" + this.electronVersion, "--arch=" + (config.getElectronArch())];

it does work and my package then loads which it doesn't without it because:

Failed to require the main module of 'Ensime' because it requires an incompatible native module.
Run `apm rebuild` in the package directory to resolve.

@Glavin001
Copy link

@hedefalk : Thank you! This finally resolves my issues building atom-beautify. This was extremely frustrating because apm rebuild did not resolve the issue and there did not seem to be a clear solution. However, this is it! Works perfectly. Thank you for your research and creating this Pull Request. Hopefully this is merged soon!

@50Wliu and @lee-dohm : What is the expected workaround until this is merged? I just noticed the date of this Pull request is May 13th. Any progress on this issue? Maybe I am missing something and this is no longer needed (using Atom 1.8.0, apm 1.9.2). However, apm rebuild consistently failed to resolve this issue and running the following immediately resolved the issue:

"/Applications/Atom Beta.app/Contents/Resources/app/apm/bin/node" "/Applications/Atom Beta.app/Contents/Resources/app/apm/node_modules/npm/bin/npm-cli.js" "--globalconfig" "/Users/viktor/.atom/.apm/.apmrc" "--userconfig" "/Users/viktor/.atom/.apmrc" "rebuild" "--target=0.34.5" "--arch=x64" "--runtime=electron"

@ghost
Copy link

ghost commented Jul 10, 2016

Can we please get this PR merged already?

The build problems with fsevents have been reported several times across multiple repositories. This PR is a very small change that fixes it. If there is some reason why it cannot be merged, can we at least get a response to that effect from someone on the Atom team?

@winstliu
Copy link
Contributor

This has been fixed via #596.

@winstliu winstliu closed this Jul 22, 2016
@ghost
Copy link

ghost commented Jul 22, 2016

So it's nice that this was fixed but I have to say that as a potential contributor it's extremely disappointing to see that this PR, which fixed a pretty serious issue for some extension developers, was sitting here since March and basically ignored but a duplicate PR (with some additions, granted) submitted by an Atom member was merged in under 8 hours…

@Glavin001
Copy link

I agree with @freebroccolo.

Although I will point out that the other Pull Request ( #596 ) does implement a couple more changes than the one-liner in this PR: https://github.com/atom/apm/pull/596/files

@thomasjo
Copy link
Contributor

@freebroccolo I agree with you. We dropped the ball on this one. As mentioned by @Glavin001, the merged PR does have additional changes, but we probably should have built #596 on top of this PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants