Opened on 08/23/2017 at 11:35:05 AM
Closed on 08/28/2017 at 08:55:36 AM
Last modified on 08/30/2017 at 02:24:33 PM
#5559 closed change (fixed)
Have ensure_dependencies.py install (production) Node.js dependencies when updating a dependency
| Reported by: | kzar | Assignee: | tlucas |
|---|---|---|---|
| Priority: | P2 | Milestone: | |
| Module: | Unknown | Keywords: | |
| Cc: | trev, sebastian, tlucas | Blocked By: | |
| Blocking: | #5535, #5560, #5596 | Platform: | Unknown / Cross platform |
| Ready: | yes | Confidential: | no |
| Tester: | Unknown | Verified working: | no |
| Review URL(s): | |||
Description (last modified by trev)
Background
We use JSDoc for document generation already, with #5535 we will use webpack for builds too. Unfortunately our dependency management system does not allow for Node.js npm dependencies to be specified and so the user has to manually install them globally instead.
We would like the ensure_dependencies.py script to call npm install --only=production --loglevel=warn after a dependency is updated which contains a package.json file. The --only=production argument avoids us installing development dependencies, for example adblockpluscore uses nodeunit for its tests which we don't want ensure_dependencies.py to install.
What to change
- Change the ensure_dependencies.py script so that it calls npm install --only=production --loglevel=warn after a dependency is cloned or updated, but only if the following is true:
- the dependency has a package.json file in its root
- the package.json file has a non-empty "dependencies" section
- the SKIP_DEPENDENCY_UPDATES environment variable isn't set.
- The command should be called from the dependency's path.
- If the npm command doesn't exist a warning should be displayed 'Failed to install Node.js dependencies for DEPENDENCY_NAME, please ensure Node.js is installed.'
Integration notes
When updating dependency on buildtools, this change and #5560 should be two separate dependency updates. This way ensure_dependencies.py will have support for Node.js dependencies once these dependencies are actually added.
Attachments (0)
Change History (18)
comment:2 Changed on 08/23/2017 at 11:58:50 AM by kzar
- Ready set
comment:3 Changed on 08/23/2017 at 01:11:47 PM by tlucas
comment:4 Changed on 08/23/2017 at 01:31:31 PM by trev
A warning I guess, same as when hg isn't available for example.
comment:7 Changed on 08/23/2017 at 01:38:07 PM by trev
I'd suggest handling JSDoc in a separate change, it's not as trivial as adding a package.json file - it needs to be called differently as well.
comment:8 Changed on 08/23/2017 at 01:47:22 PM by kzar
- Description modified (diff)
Fair enough, I removed the dependency. I'll open a separate issue for the JSDoc change now.
comment:9 Changed on 08/23/2017 at 01:56:52 PM by kzar
- Blocking 5560 added
comment:10 Changed on 08/23/2017 at 02:07:19 PM by kzar
- Description modified (diff)
comment:11 Changed on 08/23/2017 at 02:11:25 PM by kzar
- Description modified (diff)
comment:12 Changed on 08/23/2017 at 06:04:59 PM by tlucas
- Owner set to tlucas
comment:13 Changed on 08/24/2017 at 11:06:51 AM by tlucas
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:14 Changed on 08/25/2017 at 09:13:53 AM by tlucas
- Description modified (diff)
comment:15 Changed on 08/25/2017 at 09:17:04 AM by trev
- Description modified (diff)
comment:16 Changed on 08/28/2017 at 08:54:47 AM by abpbot
A commit referencing this issue has landed:
Issue 5559 - include Node.js in ensure_dependencies.py
comment:17 Changed on 08/28/2017 at 08:55:36 AM by tlucas
- Resolution set to fixed
- Status changed from reviewing to closed
comment:18 Changed on 08/30/2017 at 02:24:33 PM by kzar
- Blocking 5596 added
What should happen in case npm is not available?