Opened 5 years ago

Closed 5 years ago

#1757 closed change (fixed)

Integrate ensure_dependencies.py with the adblockpluschrome repository

Reported by: kzar Assignee: poz2k4444
Priority: P4 Milestone: Adblock-Plus-1.8.10-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: sebastian Blocked By: #1364, #1588
Blocking: #1800 Platform: Chrome
Ready: yes Confidential: no
Tester: Verified working:
Review URL(s):

http://codereview.adblockplus.org/5710085762318336/

Description (last modified by kzar)

Background

In #170 we introduced our custom dependency mechanism, it should be used by the adblockpluschrome repository.

What to change

  • Add the ensure_dependencies.py script to the root of the adblockpluschrome repository.
  • Migrate the contents of the .hgsub and .hgsubstate files to a dependencies file for use by the ensure_dependencies.py script. (See ticket #170 and the script itself for more information about the format. See the infrastructure repository for a working example.)
  • Update build.py to execute the ensure_dependencies.py script as a subprocess before running tasks. (See this code review for an example of how to do it.)

Change History (15)

comment:1 Changed 5 years ago by poz2k4444

  • Blocked By 1364 added

comment:2 Changed 5 years ago by sebastian

  • Cc sebastian added
  • Component changed from Unknown to Platform
  • Priority changed from Unknown to P4

The description isn't clear, please improve it.

Replying to kzar:

Update dependency on buildtools repository.

As time of writing, we already use the latest version of the buildtools. Also what's the point of updating it, when removing .hgsub and .hgsubstate in the same step?

Add a dependencies file and remove .hgsub and .hgsubstate files.

Nobody except the authors of ensure_dependencies.py would know what this means. Also it is slightly misleading, since .hgsub and .hgsubstate aren't supposed to be simply removed, but their content needs to be migrated to the dependencies file. Please phrase it like that and refer to documentation or examples of the required format.

Change the build.py script to call ensure_dependencies.py first.

This isn't accurate. Since ensure_dependencies.py is both, a Python module and executable script, you should specify how it is supposed to be called. Also when exactly is first? I suppose you mean before importing buildtools.build, to make sure that the correct buildtools version is used. Also it's not clear whether ensure_dependencies.py should be called from the buildtools repo or if there is supposed to be a copy in the current repository.

comment:3 Changed 5 years ago by poz2k4444

  • Blocked By 1588 added

comment:4 Changed 5 years ago by kzar

  • Description modified (diff)

comment:5 Changed 5 years ago by kzar

  • Description modified (diff)

comment:6 Changed 5 years ago by kzar

  • Description modified (diff)

comment:7 Changed 5 years ago by poz2k4444

FYI: ensure_dependencies.py should be called as a script, here is a codereview with a LGTM with a working example as well.

comment:8 Changed 5 years ago by poz2k4444

  • Description modified (diff)

comment:9 Changed 5 years ago by kzar

  • Description modified (diff)

comment:10 Changed 5 years ago by kzar

  • Description modified (diff)

comment:11 Changed 5 years ago by kzar

  • Description modified (diff)

comment:12 Changed 5 years ago by sebastian

  • Ready set

comment:13 Changed 5 years ago by trev

  • Blocking 1800 added

comment:14 Changed 5 years ago by poz2k4444

  • Review URL(s) modified (diff)

comment:15 Changed 5 years ago by sebastian

  • Milestone set to Adblock-Plus-1.8.10-for-Chrome-Opera-Safari
  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.