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

build: fail a build if "npm install" fails #16369

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@alexeykuzmin
Copy link
Contributor

alexeykuzmin commented Jan 11, 2019

Description of Change

It can fail you know.

Checklist

Release Notes

Notes: no-notes

@alexeykuzmin alexeykuzmin requested a review from nornagon Jan 11, 2019

@alexeykuzmin alexeykuzmin requested a review from electron/reviewers as a code owner Jan 11, 2019

DEPS Outdated
@@ -107,7 +107,7 @@ hooks = [
'action': [
'python',
'-c',
'import os; os.chdir("src"); os.chdir("electron"); os.system("npm install")',
'import os; os.chdir("src"); os.chdir("electron"); exit_code = os.system("npm instal"); import sys; sys.exit(exit_code)',

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 11, 2019

Contributor

FIXME instal

@alexeykuzmin

This comment has been minimized.

Copy link
Contributor

alexeykuzmin commented Jan 12, 2019

@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 alexeykuzmin force-pushed the fail-the-build-on-failed-npm-install branch from 3ba1aa0 to 969c98e Jan 12, 2019

@alexeykuzmin alexeykuzmin removed the wip label Jan 12, 2019

@alexeykuzmin alexeykuzmin force-pushed the fail-the-build-on-failed-npm-install branch from 969c98e to 05ed0b5 Jan 12, 2019

@alexeykuzmin alexeykuzmin added the wip label Jan 13, 2019

@alexeykuzmin alexeykuzmin force-pushed the fail-the-build-on-failed-npm-install branch 2 times, most recently from 7e90ac8 to 3be1d2f Jan 14, 2019

@alexeykuzmin

This comment has been minimized.

Copy link
Contributor

alexeykuzmin commented Jan 14, 2019

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

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

@alexeykuzmin alexeykuzmin removed the wip label Jan 14, 2019

DEPS Outdated
@@ -107,7 +107,7 @@ hooks = [
'action': [
'python',
'-c',
'import os; os.chdir("src"); os.chdir("electron"); os.system("npm install")',
'import os, subprocess, sys; os.chdir(os.path.join("src", "electron")); npm = "npm.cmd" if sys.platform == "win32" else "npm"; subprocess.check_call([npm, "install"]);',

This comment has been minimized.

@nornagon

nornagon Jan 14, 2019

Contributor

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.

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 15, 2019

Contributor

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

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

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 15, 2019

Contributor

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.

This comment has been minimized.

@nornagon

nornagon Jan 15, 2019

Contributor

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

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 15, 2019

Contributor

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 )

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 18, 2019

Contributor

@nornagon Any thoughts?

@alexeykuzmin alexeykuzmin force-pushed the fail-the-build-on-failed-npm-install branch from 3be1d2f to fb80ca7 Jan 15, 2019

@@ -107,7 +107,7 @@ hooks = [
'action': [
'python',
'-c',
'import os; os.chdir("src"); os.chdir("electron"); os.system("npm install")',
'import os, subprocess; os.chdir(os.path.join("src", "electron")); subprocess.check_call(["python", "script/lib/npm.py", "install"]);',

This comment has been minimized.

@nornagon

nornagon Jan 19, 2019

Contributor

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

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

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 21, 2019

Contributor

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

@alexeykuzmin alexeykuzmin force-pushed the fail-the-build-on-failed-npm-install branch 3 times, most recently from 6224090 to 16f2203 Jan 20, 2019

@alexeykuzmin alexeykuzmin force-pushed the fail-the-build-on-failed-npm-install branch from 16f2203 to d883686 Jan 22, 2019

@nornagon
Copy link
Contributor

nornagon left a comment

ok fine :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment