Opened 7 months ago

Closed 6 months ago

Last modified 3 months ago

#7371 closed change (fixed)

Generate subscriptions.json in adblockpluscore

Reported by: sebastian Assignee: tlucas
Priority: P2 Milestone:
Module: Automation Keywords:
Cc: tlucas, mjethani, greiner, hfiguiere Blocked By:
Blocking: #6977, #7338, #7360, #7425, #7459 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/41

Description (last modified by mjethani)

Background

Currently, chrome/content/ui/subscriptions.xml in adblockpluscore is generated by sitescripts/subscriptions/bin/processTemplate.py in sitescripts.

For this one needed a sitescripts.ini with at least the following content:

[subscriptions]
recommendationsTemplate=subscriptions/template/recommendations.xml
repository=<subscriptionlist repository path>

, run the command

$ python -m sitescripts.subscriptions.bin.processTemplate recommendations subscriptions.xml

and finally copy the resulting subscriptions.xml to it's designated location.

This should rather be done by an npm script within adblockpluscore. Also we should rather use JSON (than XML), as it can be parsed more easily, in particular in manifest V3 background pages which no longer have access to DOMParser.

What to change

In adblockpluscore, add a script to the build/ folder and configure it as script in package.json, that generates data/subscriptions.json from the contents of https://hg.adblockplus.org/subscriptionlist, including the same metadata provided in chrome/content/ui/subscriptions.xml but in JSON format.

The script should be called as follows:

$ npm run generate-recommended-subscriptions

Integration Notes

  • The prefixes field is now an array and is named languages
  • The fields author and specialization are no longer available.
  • subscriptions.xml is no longer available, instead please use data/subscriptions.json

Change History (25)

comment:1 Changed 7 months ago by tlucas

  • Owner set to tlucas

comment:2 Changed 7 months ago by greiner

  • Cc greiner added

comment:3 Changed 7 months ago by kzar

  • Blocking 7338 added

comment:4 Changed 7 months ago by hfiguiere

  • Cc hfiguiere added

comment:5 Changed 7 months ago by tlucas

  • Description modified (diff)

comment:6 Changed 7 months ago by tlucas

  • Description modified (diff)

comment:7 Changed 7 months ago by tlucas

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

comment:8 in reply to: ↑ description ; follow-up: Changed 7 months ago by sebastian

$ npm run generate-recommended-subscriptions <subscriptionlist repository location>

This is inconsistent wit the description above, which states that the script will put the file in data/subscriptions.json. There is no need for the path be configurable. FWIW updatepsl also hard-codes the patch ot the output file.

  • Once abpui adopted the new format, subscriptions.xml in chrome/content/ui/ should be deleted.

You can go ahead and delete chrome/content/ui/subscriptions.xml here. It won't break anything right away, as we use a hard-coded revision of adblockpluscore. Once we update the dependencies, we adapt for any breaking changes.

Last edited 7 months ago by sebastian (previous) (diff)

comment:9 in reply to: ↑ 8 Changed 7 months ago by tlucas

Replying to sebastian:

$ npm run generate-recommended-subscriptions <subscriptionlist repository location>

This is inconsistent wit the description above, which states that the script will put the file in data/subscriptions.json. There is no need for the path be configurable. FWIW updatepsl also hard-codes the patch ot the output file.

This is not an option to alter the output location, but to tell the script where to find the subscriptionlist repository.

  • Once abpui adopted the new format, subscriptions.xml in chrome/content/ui/ should be deleted.

You can go ahead and delete chrome/content/ui/subscriptions.xml here. It won't break anything right away, as we use a hard-coded revision of adblockpluscore. Once we update the dependencies, we adapt for any breaking changes.

Alright.

comment:10 Changed 7 months ago by tlucas

  • Description modified (diff)

comment:11 follow-up: Changed 7 months ago by greiner

What about mentioning in the integration notes that chrome/content/ui/subscriptions.xml has been renamed to data/subscriptions.json? It's quite an important information for anyone depending on it.

comment:12 Changed 7 months ago by tlucas

  • Description modified (diff)

comment:13 in reply to: ↑ 11 Changed 7 months ago by tlucas

Replying to greiner:

What about mentioning in the integration notes that chrome/content/ui/subscriptions.xml has been renamed to data/subscriptions.json? It's quite an important information for anyone depending on it.

IMHO that's already implicit in the description, but fair enough - added it.

comment:14 Changed 7 months ago by greiner

  • Blocking 7425 added

comment:15 Changed 7 months ago by sebastian

  • Blocking 6977 added

comment:16 Changed 7 months ago by mjethani

As per a suggestion by Thomas in ticket:6977#comment:8, we might want to add some validation here as well.

comment:17 Changed 7 months ago by tlucas

Let me please ask about some clarification (since i am accumulating information from #6977, this (#7371) and the Merge Request, as well as myself not being familiar with the overall topic). What i understand so far:

The suggested validation should

a) ensure that there is only one subscription of each type (but type=ads)
b) skip the validation on subscriptions having a potential new arbitrary key named recommended(=true)

[Please note: this will likely induce confusion. subscriptions.xml was already created from lists / variants of a subscription, which were marked as a complete "recommendation" (see e.g. here), meaning it is already consisting of recommendations only. So to mark a subscription as a valid nonunique type of subscription, we might want to us a different key, e.g. nonunique or arbitrary]

c) Essentially enable us to actually safely / sanely fix #6977, by the time we introduce the new subscriptions.json (i.e. when this ticket is fixed)

Are there additional assumptions mentioned nowhere so far, which should be validated?
Maybe i'm overcomplicating this, but since this is unknown territory for me (so far), would somebody please be so kind to update the ticket description so that i'm not missing anything (or contact me on other channels)? Thanks in advance!

comment:18 Changed 7 months ago by mjethani

OK ... my bad, since #6977 is still under discussion, let's not bring that into this change here. The goal here is to port the old script to Node.js.

(I myself do not know the answers, by the way, and I do not see a point in delaying this change for something else still under discussion.)

comment:19 Changed 7 months ago by sebastian

It seems the assumption rather is that the combination of type and language is unique (than type=ads being a special case). So if we implement any validation here, rather than introducing a new flag indicating subscriptions that must be the only one of its type (or the other way around), we rather might want to validate that for no combination of type+language there is more than one candidate.

Then again, I don't feel too strong if any validation is worth to be implemented here. We are in control of the repository subscriptions.json is compiled from, plus the script added here doesn't run automatically, but manually with its output being reviewed and tested.

comment:20 Changed 7 months ago by sebastian

  • Description modified (diff)

comment:21 Changed 6 months ago by abpbot

A commit referencing this issue has landed:
Issue 7371 - Create subscriptions.json via Node.js

comment:22 Changed 6 months ago by tlucas

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

comment:23 Changed 6 months ago by greiner

  • Blocking 7459 added

comment:24 Changed 6 months ago by mjethani

  • Description modified (diff)

comment:25 Changed 3 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as described and doesn't look to have caused any regressions with the build.

ABP 0.9.15.2339
Microsoft Edge 44.17763.1.0 / Windows 10 1809

ABP 3.5.2.2340
Chrome 49.0.2623.75 / Windows 10 1809
Chrome 75.0.3770.142 / Windows 10 1809
Opera 36.0.2130.65 / Windows 10 1809
Opera 62.0.3331.72 / Windows 10 1809
Firefox 51.0 / Windows 10 1809
Firefox 68.0 / Windows 10 1809
Firefox Mobile 68.0 / Android 7.2.2

Note: See TracTickets for help on using tickets.