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

Fix missing and hardcoded flags #596

Merged
as-cii merged 3 commits into from
Jul 22, 2016
Merged

Fix missing and hardcoded flags #596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
500aba9
Add missing --runtime=electron flag
Jul 22, 2016
88d5cbd
Don't hardcode architecture into install commands
Jul 22, 2016
a5d4676
Add leftover --runtime=electron flag
Jul 22, 2016
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
6 changes: 4 additions & 2 deletions src/dedupe.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,10 @@ class Dedupe extends Command

installNode: (callback) ->
installNodeArgs = ['install']
installNodeArgs.push("--runtime=electron")
installNodeArgs.push("--target=#{@electronVersion}")
installNodeArgs.push("--dist-url=#{config.getElectronUrl()}")
installNodeArgs.push('--arch=ia32')
installNodeArgs.push("--arch=#{config.getElectronArch()}")
installNodeArgs.push('--ensure')

env = _.extend({}, process.env, HOME: @atomNodeDirectory)
Expand Down Expand Up @@ -71,8 +72,9 @@ class Dedupe extends Command

forkDedupeCommand: (options, callback) ->
dedupeArgs = ['--globalconfig', config.getGlobalConfigPath(), '--userconfig', config.getUserConfigPath(), 'dedupe']
dedupeArgs.push("--runtime=electron")
Copy link

@hedefalk hedefalk Jul 23, 2016

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@maxbrunsfeld maxbrunsfeld Jul 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I believe the variable is called dedupeArgs because it is the list of arguments passed to the npm dedupe command.

Copy link
Contributor

@maxbrunsfeld maxbrunsfeld Jul 24, 2016

Choose a reason for hiding this comment

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

Ah nevermind, didn't get that this was a joke about code duplication 😄

Copy link
Contributor Author

@as-cii as-cii Jul 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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

dedupeArgs.push("--target=#{@electronVersion}")
dedupeArgs.push('--arch=ia32')
dedupeArgs.push("--arch=#{config.getElectronArch()}")
dedupeArgs.push('--silent') if options.argv.silent
dedupeArgs.push('--quiet') if options.argv.quiet

Expand Down
4 changes: 4 additions & 0 deletions src/install.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class Install extends Command

installNode: (callback) =>
installNodeArgs = ['install']
installNodeArgs.push("--runtime=electron")
installNodeArgs.push("--target=#{@electronVersion}")
installNodeArgs.push("--dist-url=#{config.getElectronUrl()}")
installNodeArgs.push("--arch=#{config.getElectronArch()}")
Expand Down Expand Up @@ -95,6 +96,7 @@ class Install extends Command

installArgs = ['--globalconfig', config.getGlobalConfigPath(), '--userconfig', config.getUserConfigPath(), 'install']
installArgs.push(modulePath)
installArgs.push("--runtime=electron")
installArgs.push("--target=#{@electronVersion}")
installArgs.push("--arch=#{config.getElectronArch()}")
installArgs.push("--global-style") if installGlobally
Expand Down Expand Up @@ -189,6 +191,7 @@ class Install extends Command

forkInstallCommand: (options, callback) ->
installArgs = ['--globalconfig', config.getGlobalConfigPath(), '--userconfig', config.getUserConfigPath(), 'install']
installArgs.push("--runtime=electron")
installArgs.push("--target=#{@electronVersion}")
installArgs.push("--arch=#{config.getElectronArch()}")
installArgs.push('--silent') if options.argv.silent
Expand Down Expand Up @@ -418,6 +421,7 @@ class Install extends Command

buildArgs = ['--globalconfig', config.getGlobalConfigPath(), '--userconfig', config.getUserConfigPath(), 'build']
buildArgs.push(path.resolve(__dirname, '..', 'native-module'))
buildArgs.push("--runtime=electron")
buildArgs.push("--target=#{@electronVersion}")
buildArgs.push("--arch=#{config.getElectronArch()}")

Expand Down
1 change: 1 addition & 0 deletions src/rebuild.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ class Rebuild extends Command
'--userconfig'
config.getUserConfigPath()
'rebuild'
'--runtime=electron'
"--target=#{@electronVersion}"
"--arch=#{config.getElectronArch()}"
]
Expand Down