Opened on 01/22/2016 at 01:26:00 AM

Closed on 02/09/2016 at 05:52:30 PM

#3559 closed defect (rejected)

ensure_dependencies.py should run npm install when a package.json exists

Reported by: erikvold Assignee: erikvold
Priority: Unknown Milestone:
Module: Automation Keywords:
Cc: trev, sebastian Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

I think that ensure_dependencies.py should run npm install when a package.json exists this way we can have the python in buildtools use subprocess to run node could, which can help us run tests against our extensions. Using this method we could simply use node or the build.py file in many cases, it opens up quite a few different options for us to utilize later, and allows us to shift to using more javascript for our buildtools if we are interested in doing so.

Attachments (0)

Change History (5)

comment:1 Changed on 01/22/2016 at 01:36:10 AM by erikvold

@trev could you take a look at:

https://github.com/adblockplus/buildtools/compare/master...erikvold:build-npm
https://github.com/adblockplus/adblockplus/compare/master...erikvold:build-npm

The primary change here is to run npm install when running ensure_dependencies.py for the root and for each dependency, only if there is a package.json in that working directory. The other changes test this by adding a package.json to the adblockplus repo and the buildtools repo, and including some simple tests.

I would clean up the commits, and not include the tests which are just for demonstration of how this can be used afterwards to add node tests to our repos.

I was thinking about whether or not we should include tests with the repos the are testing which is done for most repos, except adblockplus and adblockplustests repos which have divided these functions into separate repos. I think we should include tests in the repo they are testing in the future, so supporting both methods was important, and I figured these changes would be the easiest to make in order to support both methods.

Please let me know what you think!



comment:2 Changed on 02/09/2016 at 11:35:08 AM by trev

  • Cc sebastian added

Sebastian, could you take a look? This is your module.

comment:3 Changed on 02/09/2016 at 12:07:52 PM by sebastian

I'm not sure whether ensure_dependencies.py is the right place to call npm install. So far ensure_dpendencies.py is agnostic of the used technologies in the repos. Even more important, npm install would touches files outside of the cloned repos. So it might be unexpected that updating dependencies will install software user-wide. Wladimr, what do you think?

comment:4 Changed on 02/09/2016 at 05:14:37 PM by trev

Note that npm install without additional parameters won't actually install anything user-wide, all the downloads are placed in the current directory. Still, I agree. The purpose of ensure_dependencies.py is connecting repositories, not installing third-party software. We wouldn't call pip to install dependencies of our Python scripts either - it just isn't the job of ensure_dependencies.py. Note that ensure_dependencies.py is called as part of the build process, yet calling npm install on each build isn't always desirable. Also, we cannot know whether somebody wants to install development dependencies or merely run npm install --production.

comment:5 Changed on 02/09/2016 at 05:52:30 PM by sebastian

  • Resolution set to rejected
  • Status changed from new to closed

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from erikvold.
 
Note: See TracTickets for help on using tickets.