Issue 5757 - Update buildtools dependency, (re)move legacy extensions

Issue 5757 - Update buildtools dependency, (re)move legacy extensions

Repository: sitescripts
Base: https://codereview.adblockplus.org/29589674/

[tlucas, 2017-11-02 10:03:30]

Hey all.

This patch fixes #5757, please find some comments below.

I verified the nightly builds for adblockplussafari to be identical after
updating to this.

Best,
Tristan

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/extensi...
File sitescripts/extensions/bin/legacy/packagerSafari.py (right):

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/extensi...
sitescripts/extensions/bin/legacy/packagerSafari.py:1: # This Source Code Form
is subject to the terms of the Mozilla Public
This code was (apart from the below exception) completely copied from buildtools
@ revision 1b8ddae7f0b3

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/extensi...
sitescripts/extensions/bin/legacy/packagerSafari.py:126: def convertJS(params,
files):
Thins function was copied from packagerChrome.py in buildtools @ revision
1b8ddae7f0b3.

The respective imports were, additional required imports added.

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/extensi...
File sitescripts/extensions/bin/legacy/xarfile.py (right):

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/extensi...
sitescripts/extensions/bin/legacy/xarfile.py:1: # This Source Code Form is
subject to the terms of the Mozilla Public
This code was completely copied from buildtools @ revision 1b8ddae7f0b3

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/stats/b...
File sitescripts/stats/bin/logprocessor.py (right):

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/stats/b...
sitescripts/stats/bin/logprocessor.py:41: KNOWN_APPS = {
I am not sure whether we (still) need all of these. Please let me know!

[Vasily Kuznetsov, 2017-11-30 17:34:00]

Hi Tristan,

Sorry for taking so long to review this. Somehow I never got to finishing it.

The changes look good and there are only minor comments (see below). I have one
question though: what is the purpose of moving Safari packager into
sitescripts/extensions/bin/legacy? Is it expected to be called there, in which
situations?

Cheers,
Vasily

https://codereview.adblockplus.org/29589691/diff/29589692/ensure_dependencies.py
File ensure_dependencies.py (right):

https://codereview.adblockplus.org/29589691/diff/29589692/ensure_dependencies...
ensure_dependencies.py:1: #!/usr/bin/env python
I have not properly reviewed this script, assuming that it's just copied from
buildtools.

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/extensi...
File sitescripts/extensions/bin/legacy/packagerSafari.py (right):

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/extensi...
sitescripts/extensions/bin/legacy/packagerSafari.py:1: # This Source Code Form
is subject to the terms of the Mozilla Public
On 2017/11/02 10:03:29, tlucas wrote:
> This code was (apart from the below exception) completely copied from
buildtools
> @ revision 1b8ddae7f0b3

Acknowledged.

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/extensi...
File sitescripts/extensions/bin/legacy/xarfile.py (right):

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/extensi...
sitescripts/extensions/bin/legacy/xarfile.py:1: # This Source Code Form is
subject to the terms of the Mozilla Public
On 2017/11/02 10:03:29, tlucas wrote:
> This code was completely copied from buildtools @ revision 1b8ddae7f0b3

Acknowledged.

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/stats/b...
File sitescripts/stats/bin/logprocessor.py (right):

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/stats/b...
sitescripts/stats/bin/logprocessor.py:41: KNOWN_APPS = {
On 2017/11/02 10:03:29, tlucas wrote:
> I am not sure whether we (still) need all of these. Please let me know!

I suppose they might be needed because of users with legacy versions. In any
case, if we remove them, it should be a separate ticket, and we should probably
also remove all the related code in the file.

[tlucas, 2017-11-30 18:15:27]

On 2017/11/30 17:34:00, Vasily Kuznetsov wrote:
> Hi Tristan,
> 
> Sorry for taking so long to review this. Somehow I never got to finishing it.
> 
> The changes look good and there are only minor comments (see below). I have
one
> question though: what is the purpose of moving Safari packager into
> sitescripts/extensions/bin/legacy? Is it expected to be called there, in which
> situations?
> 
> Cheers,
> Vasily

Hey Vasily, 

no worries, this wasn't to urgent.
As discussed with Sebastian (and as mentioned in the Ticket
https://issues.adblockplus.org/ticket/5752#comment2 ), the buildtools latest
versions do not need to support legacy-gecko / safari anymore. However, since we
seem to occasionally still need to do minor upgrades for the safari extension,
we need to have some tool capable of building the extension.

I hope this somehow shed light on this!

Best,
Tristan

https://codereview.adblockplus.org/29589691/diff/29589692/ensure_dependencies.py
File ensure_dependencies.py (right):

https://codereview.adblockplus.org/29589691/diff/29589692/ensure_dependencies...
ensure_dependencies.py:1: #!/usr/bin/env python
On 2017/11/30 17:34:00, Vasily Kuznetsov wrote:
> I have not properly reviewed this script, assuming that it's just copied from
> buildtools.

Yes, this is the result of ensure_dependencies.py's self update.

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/stats/b...
File sitescripts/stats/bin/logprocessor.py (right):

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/stats/b...
sitescripts/stats/bin/logprocessor.py:41: KNOWN_APPS = {
On 2017/11/30 17:34:00, Vasily Kuznetsov wrote:
> On 2017/11/02 10:03:29, tlucas wrote:
> > I am not sure whether we (still) need all of these. Please let me know!
> 
> I suppose they might be needed because of users with legacy versions. In any
> case, if we remove them, it should be a separate ticket, and we should
probably
> also remove all the related code in the file.

By removing the code, you also mean "in a separate issue" - am i right?

[Vasily Kuznetsov, 2017-11-30 19:10:08]

LGTM.

Feel free to update the ensure_dependecies.py script again if you would like --
I think there's an additional change that landed in buildtools since this was
uploaded.

Cheers,
Vasily

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/stats/b...
File sitescripts/stats/bin/logprocessor.py (right):

https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/stats/b...
sitescripts/stats/bin/logprocessor.py:41: KNOWN_APPS = {
On 2017/11/30 18:15:26, tlucas wrote:
> On 2017/11/30 17:34:00, Vasily Kuznetsov wrote:
> > On 2017/11/02 10:03:29, tlucas wrote:
> > > I am not sure whether we (still) need all of these. Please let me know!
> > 
> > I suppose they might be needed because of users with legacy versions. In any
> > case, if we remove them, it should be a separate ticket, and we should
> probably
> > also remove all the related code in the file.
> 
> By removing the code, you also mean "in a separate issue" - am i right?

Yes.

[tlucas, 2017-12-04 10:24:45]

On 2017/11/30 19:10:08, Vasily Kuznetsov wrote:
> LGTM.
> 
> Feel free to update the ensure_dependecies.py script again if you would like
--
> I think there's an additional change that landed in buildtools since this was
> uploaded.
> 
> Cheers,
> Vasily
> 
>
https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/stats/b...
> File sitescripts/stats/bin/logprocessor.py (right):
> 
>
https://codereview.adblockplus.org/29589691/diff/29589692/sitescripts/stats/b...
> sitescripts/stats/bin/logprocessor.py:41: KNOWN_APPS = {
> On 2017/11/30 18:15:26, tlucas wrote:
> > On 2017/11/30 17:34:00, Vasily Kuznetsov wrote:
> > > On 2017/11/02 10:03:29, tlucas wrote:
> > > > I am not sure whether we (still) need all of these. Please let me know!
> > > 
> > > I suppose they might be needed because of users with legacy versions. In
any
> > > case, if we remove them, it should be a separate ticket, and we should
> > probably
> > > also remove all the related code in the file.
> > 
> > By removing the code, you also mean "in a separate issue" - am i right?
> 
> Yes.

I think this would be overkill and of no benefit for the sitescripts :)