Fix missing and hardcoded flags #596
Conversation
|
Supersedes #536. |
| @@ -71,6 +72,7 @@ class Dedupe extends Command | |||
|
|
|||
| forkDedupeCommand: (options, callback) -> | |||
| dedupeArgs = ['--globalconfig', config.getGlobalConfigPath(), '--userconfig', config.getUserConfigPath(), 'dedupe'] | |||
| dedupeArgs.push("--runtime=electron") | |||
There was a problem hiding this comment.
Contrary to the name "dedupe", don't we have a lot of dups going on here? I guess we realized we needed '--runtime=electron' on all npm calls and then you had to find 6 places to add it… I found only one in #536 :)
Cleanup time?
There was a problem hiding this comment.
I believe the variable is called dedupeArgs because it is the list of arguments passed to the npm dedupe command.
There was a problem hiding this comment.
Ah nevermind, didn't get that this was a joke about code duplication 😄
There was a problem hiding this comment.
That's a great point, @hedefalk. I considered using the .apmrc file to store all these configuration settings, but it felt somewhat inconsistent because we still need to use dynamically generated options (e.g. the architecture and the electron version) and there is something nice about having those options close to one another.
I guess an alternative could be to extract a method (and put it in apm.coffee) named getNpmBuildOptions()/getNpmBuildFlags() which takes care of constructing an array with the necessary parameters. I'll open a pull-request with this change. 👍
Requires a more recent version of Atom, since this version of Chokidar was not compatible with native extensions in previous versions of apm See atom/apm#596
This pull-request fixes a problem when installing dependencies via
apmthat prevents the runtime from being recognized, causing packages such as nodegit to fail the build. In particular, we are adding the--runtime=electronflag to everynpmcommand that interfaces with modules installation and their subsequent compilation.Please, note that this is the approach that electron-rebuild uses too.
While in the process of adding such flag, we spotted a couple of places in the
dedupecommand where the target architecture of native modules was being hardcoded; this was probably not an issue, but it feels reasonable to useconfig.getElectronArch()as we do elsewhere.@atom/core: I'd ❤️ to have some more 👀 on this, so that we can 🚢 it before getting the new
apmversion into beta.