Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#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):

https://codereview.adblockplus.org/29526588/

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.

Change History (18)

comment:1 Changed 2 years ago by kzar

  • Description modified (diff)

comment:2 Changed 2 years ago by kzar

  • Ready set

comment:3 Changed 2 years ago by tlucas

What should happen in case npm is not available?

comment:4 Changed 2 years ago by trev

A warning I guess, same as when hg isn't available for example.

comment:5 Changed 2 years ago by kzar

  • Description modified (diff)

comment:6 Changed 2 years ago by kzar

  • Description modified (diff)

comment:7 Changed 2 years ago 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 2 years ago 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 2 years ago by kzar

  • Blocking 5560 added

comment:10 Changed 2 years ago by kzar

  • Description modified (diff)

comment:11 Changed 2 years ago by kzar

  • Description modified (diff)

comment:12 Changed 2 years ago by tlucas

  • Owner set to tlucas

comment:13 Changed 2 years ago by tlucas

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:14 Changed 2 years ago by tlucas

  • Description modified (diff)

comment:15 Changed 2 years ago by trev

  • Description modified (diff)

comment:16 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 5559 - include Node.js in ensure_dependencies.py

comment:17 Changed 2 years ago by tlucas

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:18 Changed 2 years ago by kzar

  • Blocking 5596 added
Note: See TracTickets for help on using tickets.