build: fail a build if "npm install" fails

[alexeykuzmin]

Description of Change

It can fail you know.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• PR title follows semantic commit guidelines
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: no-notes

[alexeykuzmin]

FIXME instal

[alexeykuzmin]

@nornagon
The typo has been made intentionally to make the npm command fail and exit with code 1.
The problem is that it didn't work
https://circleci.com/gh/electron/electron/115735

________ running '/usr/bin/python -c import os; os.chdir("src"); os.chdir("electron"); exit_code = os.system("npm instal"); import sys; sys.exit(exit_code)' in '/home/builduser/project'

Usage: npm <command>

<...>

Did you mean one of these?
    install
    unstar
    uninstall

<...>

Running hooks: 100% (79/79), done.

I'll replace all os.system() calls with subprocess.* calls.

[alexeykuzmin]

I'll replace all os.system() calls with subprocess.* calls.

Done.
Now it works: https://circleci.com/gh/electron/electron/116496

[nornagon]

how come npm.cmd is now needed on windows? afaik just npm was working fine previously.

If this is truly needed, then this has become too complicated for a one-liner & should be moved to a script.

[alexeykuzmin]

npm was working fine previously

Only when you use os.system().

subprocess.call() needs different npm executable names on different platforms, see here for example

electron/script/lib/util.py

Lines 28 to 30 in 52fe92d

	NPM = 'npm'
	if sys.platform in ['win32', 'cygwin']:
	NPM += '.cmd'

[alexeykuzmin]

I can move the npm call to a Python script.

You think it would be better to move the whole thing to a new script?
It doesn't look that different now from those python setup.py build calls for boto and requests.

[nornagon]

IMO the if statement is enough to push it over the edge of "shouldn't be inlined in a string in a config file". The only reason these are in Python instead of just being direct commands is because there's no way to cd in a cross-platform way, and npm doesn't have a -C argument like make and ninja.

I'd be fine with having a script/cd-npm.py that took a directory as its first argument and passed the rest of its arguments directly to npm (or npm.cmd).

[alexeykuzmin]

I feel like calling os.chdir(...) before calling the npm.py is good enough. At least it's not worse than calling the same for boto and requests.

os.chdir(os.path.join("src", "electron"));
subprocess.check_call(["python", "script/lib/npm.py", "install"]);

This ^ change is already there btw )

[alexeykuzmin]

@nornagon Any thoughts?

[nornagon]

it's weird to invoke python to cd and then invoke python :/

Would the --prefix option work? i.e. npm install --prefix src/electron

[alexeykuzmin]

Works on Linux, but doesn't work on Windows.

[nornagon]

ok fine :)

[release-clerk]

No Release Notes

[trop]

I have automatically backported this PR to "4-0-x", please check out #16505