npm module: Fix a bug installing local packages globally with npm #5205

Closed
wants to merge 1 commit into from

5 participants

@brianz

When specifying a local package with path and global, the check for
whether that package was missing would always return false. Because the npm
list
command was run with the --global switch, the missing packages would
never contain the local package since there is no way for npm to know about it.

In order to see whether a local package is or isn't installed locally, we must
do an npm list without the global flag inside the local package, then
explicity add it to the list of globally missing packages.

This fixes #5204

@brianz

Adding @chrishoffman in hopes of a review.

@chrishoffman
Ansible member

I'll check it out this week

@jctanner
Ansible member

@chrishoffman any updates?

@chrishoffman
Ansible member

This seems like a non-standard use case for npm. If I understand correctly, you are trying to take a package.config in a directory and install it globally? Am I interpreting this correctly?

Looking at the original bug report, it looks like you are looking to install a checked out version of a repository globally. I don't think this is happening with your current implementation, it is just installing the checked out packages dependencies from the package.json.

If you want to install a package from another repo, you could always use the npm install tarball url/path. I am not sure that this would be idempotent then.

@brianz

Yes, you read the bug report and commit correctly.

I've tested this exact case and this patch solves the problem. The end result is exactly what npm install -g does when inside a package sitting on disk.

  • install local package into global directory (/usr/local/lib/node_modules in my Ubuntu VM)
  • install that packages dependencies into /usr/local/lib/node_modules/package_name/node_modules

Given, I'm new to node and npm so can't speak as to whether this is common or standard, but this change makes this ansible module agree with npm's behavior.

@chrishoffman
Ansible member

I find this to be a very unlikely scenario so I feel like this is an edge case. npm is so good because of how it handles dependencies without needing to set up different environments since everything gets installed locally and node understands this very well. I am hesitant to agree with this behavior even though npm allows you to do this.

Also, in reviewing your code, it doesn't look like like your code handles when there are more than one dependencies that are missing. It looks like it only handles when there is one.

@brianz

I can't argue one way or another with the behavior of npm since I'm new to it. I have no opinion whether this is good or bad. But, I do have an opinion on diverging from the default behavior...it's bad. If an ansible module executes a command and takes arguments, it really should behave exactly the same as executing that command with the same arguments on the host, regardless of whether it's unlikely.

As far as what this patch does, I have an in-house node package which has multiple dependencies. This change results is all of the dependencies being installed in /usr/local/lib/node_modules/package_name/node_modules.

@chrishoffman
Ansible member

It is not necessarily bad if it adds too much complexity and brittleness. The goal of this module is to provide support for the majority of use cases (as I believe most ansible modules do) and not necessarily provide 100% support for every function. The great part, which also provides the complexity, is making modules idempotent which this one does.

Have you deleted all the packages in your in-house package and then tried running it again? This is tough to check since they are all installed globally. From what I can tell from your code, it could only ever add one package to the missing list.

Here are a couple of articles that go into what should be local vs global.

http://blog.nodejitsu.com/npm-cheatsheet#Understanding_Global_versus_Local_installs_in_npm
http://himanshu.gilani.info/blog/2012/07/29/npm-and-package-dot-json/

@jimi-c
Ansible member

@chrishoffman beyond this being a corner-case, do you see any issues with the above code? I'm inclined to merge this in.

@chrishoffman
Ansible member

I have not tested this but in reviewing the code, I can only see it ever adding one package instead of all the missing packages globally. I may be wrong here but I don't think this code works as intended.

@brianz

@chrishoffman, it does work as intended. I've tested it and verified that it works.

@jimi-c
Ansible member

@brianz it seems like you're duplicating a lot of the functionality under the if statement on line 156. It would seem to make more sense to handle that check above first call to _exec like so:

    if self.path and self.glbl and not self.name:
        data = json.loads(self._exec(cmd, True, False, skip_global=True))
        self.name = data.get('name') 
    else:
        data = json.loads(self._exec(cmd, True, False))
    ...
    

Or am I missing something else?

@brianz

@jimi-c, nice catch, I think that would work also. I added it separately since I'm mostly new to npm and thought this was a bit safer...but without doing an actual test I believe your suggested change does the trick.

I don't have time to test this out today....but I'd be happy to do so next week.

@jimi-c
Ansible member

Yes, feel free to modify your code to do the above, and just use git commit --amend and git push --force to overwrite your existing pull request. That way you don't have to open up a new issue.

@jimi-c jimi-c self-assigned this Feb 14, 2014
@mpdehaan

Any updates on this one? Thanks!

@brianz brianz Fix a bug installing local packages globally with npm
When specifying a local package with `path` and `global`, the check for
whether that package was missing would always return false. Because the `npm
list` command was run with the `--global` switch, the missing packages would
never contain the local package since there is no way for npm to know about it.

 In order to see whether a local package is or isn't installed locally, we must
do an `npm list` *without* the global flag inside the local package, then
explicity add it to the list of globally missing packages.

This fixes #5204
e7e6a78
@mpdehaan mpdehaan added the module label Mar 27, 2014
@mpdehaan

So in reviewing the latest changes to files, I see there is a skip_global function argument added to exec that is used by the code below, but no way for anything to pass this function in or use this behavior.

As such (and givent, I'm going to close this pull request.

@mpdehaan mpdehaan closed this Mar 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment