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

npm.installed mistakenly throws error for packages which are "installed via remote" #43138

Closed
F30 opened this Issue Aug 23, 2017 · 6 comments

Comments

Projects
None yet
5 participants
@F30
Contributor

F30 commented Aug 23, 2017

Description of Issue/Question

The npm.installed state throws an error when installing certain packages. I was able to track this down to packages where "is installed via remote" is printed to stdout. (Whatever that means, it's not really Google-able.) Examples for such packages are "sqlite3" or "bcrypt", and the exact line looks like this:

[bcrypt] Success: "/tmp/npm-test/node_modules/bcrypt/lib/binding/bcrypt_lib.node" is installed via remote"

However, the packages acutally are successfully installed, so a subsequent highstate will succeed.

The cause for this appears to be that the npm.install module cannot parse the output of npm install --silent --json because of that line. It therefore returns a string, which the npm.installed state counts as failure.

Since #35075, there already is the special _extract_json() function in place, which tries to isolate the JSON from the other output like this:

while lines and not lines[0].startswith('{') and not lines[0].startswith('['):
    lines = lines[1:]
while lines and not lines[-1].startswith('}') and not lines[-1].startswith(']'):
    lines = lines[:-1]

Unfortunately, the line from above starts with a '[', although it's not part of the JSON.

NPM appears to always output a JSON object (not a list), so the checks could probably be limited to startswith('{'). But what do I know, I can't even find documentation for NPM's JSON output.

Steps to Reproduce Issue

Create a target directory like "/tmp/npm-test" on the minion and try to install a NPM package like "sqlite3" or "bcrypt" using npm.installed like this:

sqlite3:
  npm.installed:
    - dir: /tmp/npm-test

It will fail with the error "Could not install package(s) 'sqlite3'", but the package will still be installed.

Versions Report

  • Salt 2016.11.2, but the relevant code is also present on the current "develop" branch
  • NPM 3.10.10
@Ch3LL

This comment has been minimized.

Contributor

Ch3LL commented Aug 24, 2017

We also had to do something similar here: #41776

seems we will need to also add a check for this output as well.

@Ch3LL Ch3LL added this to the Approved milestone Aug 24, 2017

@F30

This comment has been minimized.

Contributor

F30 commented Oct 5, 2017

Yeah, secial-casing all kinds of output feels rather ugly, but I suspect it's necessary.

Obviously, an alternative would be getting NPM to only output JSON to stdout in JSON mode. I suppose that's not really a solution for Salt, as older versions of NPM would still have to be supported? But maybe it is still worth trying for the future.

jchampemont added a commit to jchampemont/ghost-formula that referenced this issue Nov 12, 2017

@EvaSDK

This comment has been minimized.

Contributor

EvaSDK commented Feb 5, 2018

Just upgraded from 2016.11 to 2017.7 and hit this as well. Is there any fix planned or should I switch to cmd.wait ?

@Ch3LL

This comment has been minimized.

Contributor

Ch3LL commented Feb 12, 2018

This isn't currently on our roadmap due to other higher priority issues. Please feel free to submit a PR for the issue though.

@wyardley

This comment has been minimized.

Contributor

wyardley commented Jul 9, 2018

Ah, I see, this is triggering the normal check for the start of json output. Is the output of npm --json ever a [, or only ever {?

[grpc] Success: "/usr/lib/node_modules/@google-cloud/pubsub/node_modules/grpc/src/node/extension_binary/node-v57-linux-x64-glibc/grpc_node.node" is installed via remote
[...]
make: Entering directory '/usr/lib/node_modules/xyz/node_modules/webworker-threads/build'
  CXX(target) Release/obj.target/WebWorkerThreads/src/WebWorkerThreads.o
  SOLINK_MODULE(target) Release/obj.target/WebWorkerThreads.node
  COPY Release/WebWorkerThreads.node
make: Leaving directory '/usr/lib/node_modules/xyz/node_modules/webworker-threads/build'

Would the following work?

--- npm.py~	2018-07-09 19:19:36.130000000 +0000
+++ npm.py	2018-07-09 19:23:57.570000000 +0000
@@ -183,15 +183,10 @@
     log.error(lines)
 
     # Strip all lines until JSON output starts
-    while lines and not lines[0].startswith('{') and not lines[0].startswith('['):
+    while lines and not lines[0] == '{' and not lines[0] == '[':
         lines = lines[1:]
-    while lines and not lines[-1].startswith('}') and not lines[-1].startswith(']'):
+    while lines and not lines[-1] == '}' and not lines[-1] == ']':
         lines = lines[:-1]
-    # macOS with fsevents includes the following line in the return
-    # when a new module is installed which is invalid JSON:
-    #     [fsevents] Success: "..."
-    while lines and (lines[0].startswith('[fsevents]') or lines[0].startswith('Pass ')):
-        lines = lines[1:]
     try:
         return salt.utils.json.loads(''.join(lines))
     except ValueError:

wyardley added a commit to wyardley/salt that referenced this issue Jul 9, 2018

Improve handling of identifying json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 9, 2018

Improve handling of identifying json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 9, 2018

Improve handling of identifying json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 10, 2018

Improve handling of identifying json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 10, 2018

Improve handling of identifying json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 10, 2018

Improve handling of identifying json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 11, 2018

Improve handling of identifying json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 11, 2018

Improve handling of identifying json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 12, 2018

Improve handling of identifying json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 13, 2018

Improve handling of identifying json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 14, 2018

Improve handling of json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 14, 2018

Improve handling of json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 14, 2018

Improve handling of json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 16, 2018

Improve handling of json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 17, 2018

Improve handling of json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 18, 2018

Improve handling of json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 18, 2018

Improve handling of json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 18, 2018

Improve handling of json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 18, 2018

Improve handling of json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line, it looks for
equality. Since the JSON output of `npm install` seems to have the first
character on its own, this seems to work, at least for the npm version I
looked at.

wyardley added a commit to wyardley/salt that referenced this issue Jul 19, 2018

Improve handling of npm json output (saltstack#43138)
Rather than look for [ or { at the beginning of a line to identify the json
output, just use salt.utils(.json).findjson.
@wyardley

This comment has been minimized.

Contributor

wyardley commented Jul 20, 2018

Sorry for all the spam about various commits, but I think #48658 should resolve this, and similar issues.

rallytime added a commit that referenced this issue Jul 20, 2018

@rallytime rallytime closed this Sep 7, 2018

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