Skip to content
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

Replaced 'node' by 'nodejs' in Popen() and check_call() calls #1440

Conversation

neveldo
Copy link
Contributor

@neveldo neveldo commented Jan 15, 2016

Since the commit bf31fc0, I get this error message when I run the build script build.py :

[[build.py RunSmokeTest]] - entering ...
Traceback (most recent call last):
File "./build.py", line 350, in
RunSmokeTest(out_dir='dist')
File "./build.py", line 265, in RunSmokeTest
stdout=subprocess.PIPE, stderr=subprocess.PIPE)
File "/usr/lib/python2.7/subprocess.py", line 710, in init
errread, errwrite)
File "/usr/lib/python2.7/subprocess.py", line 1327, in _execute_child
raise child_exception
OSError: [Errno 2] No such file or directory

I think that 'node' should be replaced by 'nodejs' in subprocess.Popen() and subprocess.check_call() calls to fix it.

@dvoytenko
Copy link
Contributor

/cc @powdercloud

@erwinmombay
Copy link
Member

/cc @cramforce since i know he intentionally changed this to get travis to work

@cramforce
Copy link
Member

See failure of travis https://travis-ci.org/ampproject/amphtml/builds/102483529

This was done to make Travis happy. In general having a node binary instead of a nodejs binary is much more common.

Since I know that ubuntu, when you install via apt-get, does the stupid nodejs I think the best would be to support both.

Of course, you could fix it for yourself by adding alias node=nodejs to your .bashrc

@powdercloud
Copy link
Contributor

This command might be a good indicator:
node --eval 'console.log("42")'
If it manages to print "42", then we can assume node is the command. And same for nodejs.
If neither work, CheckPrereqs (after trying them) could give up in style. If one of them works, it could return that command name and then the rest of the build.py should use it, maybe even for the shebang lines (also since they've drifted a tad from the smoke test actually).

I think this is potentially a rabbit hole - the risk is to reinvent autoconf, but if it's a common enough problem maybe it's worth sticking in just this one big toe?

@cramforce
Copy link
Member

👍

@neveldo
Copy link
Contributor Author

neveldo commented Jan 15, 2016

Thanks for the explanation. Indeed, I run the validator on Ubuntu, so the default command for node is nodejs. As it is mentionned in the README.md of the validator :

Prerequisite: Linux Ubuntu 14 or similar

Install these packages using apt-get:

npm
nodejs
openjdk-7-jre
protobuf-compiler
python-protobuf
python2.7

So I think it could be a common problem because everyone who want to run the validator by following strictly the instructions could potentially encounter the same issue.

Where do we go ? Should I submit a new PR with the proposal of updating the CheckPrereqs() function from @powdercloud ? Or maybe, we could simply update the README.md to tell the users to add an alias from nodejs to node as @cramforce said ?

@cramforce
Copy link
Member

I think supporting both would be great!

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

Successfully merging this pull request may close these issues.

5 participants