Issue 6291923287408640: Issue 1093 - Separate update manifest generation (Closed)
|
Created:
July 22, 2014, 8:03 a.m. by Felix Dahlke Modified:
July 23, 2014, 6:35 a.m. Reviewers:
Wladimir Palant Visibility:
Public. |
DescriptionIssue 1093 - Separate update manifest generation
Patch Set 1 #
Total comments: 21
Patch Set 2 : Address comments #
Total comments: 3
Patch Set 3 : Fix issue #
MessagesTotal messages: 6
http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... File sitescripts/extensions/bin/updateUpdateManifests.py (right): http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateUpdateManifests.py:30: def writeUpdateManifest(links): This hasn't been changed, just moved. Since I kept the logic as it is, we have to keep a few repositories in sync on the update server (in addition to downloads): adblockplus, adblockpluschrome and adblockplusandroid. I'm a bit undecided - shall we create a follow-up for removing that requirement? http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... File sitescripts/extensions/utils.py (right): http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/utils.py:263: def _getLocalLink(repo): These functions have just been moved. The only change I did was to prefix local functions with a _.
http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... File sitescripts/extensions/bin/updateDownloadLinks.py (right): http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateDownloadLinks.py:26: import sys, os, re, urllib, urllib2, subprocess, time sys, os, subprocess are no longer being used here. Please convert this into one import per line while at it. http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateDownloadLinks.py:27: import xml.dom.minidom as dom dom should no longer be required here. http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateDownloadLinks.py:29: from StringIO import StringIO StringIO is no longer being used here. http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateDownloadLinks.py:30: from sitescripts.utils import get_config, get_template get_template is no longer being used here. http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateDownloadLinks.py:32: getSafariCertificateID, getDownloadLinks, readMetadata) compareVersions, getSafariCertificateID, readMetadata aren't being used here. http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateDownloadLinks.py:86: def getOperaDownloadLink(galleryID): From the look of it, this function and the ones above it should be located in the utils module where they are used. http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... File sitescripts/extensions/bin/updateUpdateManifests.py (right): http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateUpdateManifests.py:19: Update update manifests Generate update manifests? http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateUpdateManifests.py:30: def writeUpdateManifest(links): On 2014/07/22 08:11:11, Felix H. Dahlke wrote: > Since I kept the logic as it is, we have to keep a few repositories in sync on > the update server (in addition to downloads): adblockplus, adblockpluschrome and > adblockplusandroid. Quite a few more actually: elemhidehelper, abpcustomization, abpwatcher etc. > I'm a bit undecided - shall we create a follow-up for > removing that requirement? How would you do that? You need parameters like compatibility info for the update manifest. The only alternative is IMHO downloading https://hg.adblockplus.org/adblockpluschrome/raw-file/1.8.3/metadata.chrome. However, this path is dependent on the server software (e.g. will be https://hg.adblockplus.org/adblockpluschrome/raw/1.8.3/metadata.chrome with RhodeCode) and it also won't be simple (things like inheritance need to be considered). http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateUpdateManifests.py:51: print extensions[repoType] Debug code?
Uploaded a new patch set. http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... File sitescripts/extensions/bin/updateDownloadLinks.py (right): http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateDownloadLinks.py:26: import sys, os, re, urllib, urllib2, subprocess, time On 2014/07/22 11:24:26, Wladimir Palant wrote: > sys, os, subprocess are no longer being used here. Please convert this into one > import per line while at it. Done. http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateDownloadLinks.py:27: import xml.dom.minidom as dom On 2014/07/22 11:24:26, Wladimir Palant wrote: > dom should no longer be required here. Done. http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateDownloadLinks.py:29: from StringIO import StringIO On 2014/07/22 11:24:26, Wladimir Palant wrote: > StringIO is no longer being used here. Done. http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateDownloadLinks.py:30: from sitescripts.utils import get_config, get_template On 2014/07/22 11:24:26, Wladimir Palant wrote: > get_template is no longer being used here. Done. http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateDownloadLinks.py:32: getSafariCertificateID, getDownloadLinks, readMetadata) On 2014/07/22 11:24:26, Wladimir Palant wrote: > compareVersions, getSafariCertificateID, readMetadata aren't being used here. Done. http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateDownloadLinks.py:86: def getOperaDownloadLink(galleryID): On 2014/07/22 11:24:26, Wladimir Palant wrote: > From the look of it, this function and the ones above it should be located in > the utils module where they are used. Done. http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... File sitescripts/extensions/bin/updateUpdateManifests.py (right): http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateUpdateManifests.py:19: Update update manifests On 2014/07/22 11:24:26, Wladimir Palant wrote: > Generate update manifests? Done. http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateUpdateManifests.py:30: def writeUpdateManifest(links): On 2014/07/22 11:24:26, Wladimir Palant wrote: > On 2014/07/22 08:11:11, Felix H. Dahlke wrote: > > Since I kept the logic as it is, we have to keep a few repositories in sync on > > the update server (in addition to downloads): adblockplus, adblockpluschrome > and > > adblockplusandroid. > > Quite a few more actually: elemhidehelper, abpcustomization, abpwatcher etc. It doesn't look to me like we're generating update manifests for those - do we? > > I'm a bit undecided - shall we create a follow-up for > > removing that requirement? > > How would you do that? You need parameters like compatibility info for the > update manifest. The only alternative is IMHO downloading > https://hg.adblockplus.org/adblockpluschrome/raw-file/1.8.3/metadata.chrome. > However, this path is dependent on the server software (e.g. will be > https://hg.adblockplus.org/adblockpluschrome/raw/1.8.3/metadata.chrome with > RhodeCode) and it also won't be simple (things like inheritance need to be > considered). Alright, let's stick to syncing the repositories then. No real harm in that. http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateUpdateManifests.py:51: print extensions[repoType] On 2014/07/22 11:24:26, Wladimir Palant wrote: > Debug code? Ouch, done. http://codereview.adblockplus.org/6291923287408640/diff/5122315436163072/site... File sitescripts/extensions/bin/updateUpdateManifests.py (right): http://codereview.adblockplus.org/6291923287408640/diff/5122315436163072/site... sitescripts/extensions/bin/updateUpdateManifests.py:34: def readMetadata(repo, version): Noticed that this is actually only used here, so it doesn't belong in utils.
http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... File sitescripts/extensions/bin/updateUpdateManifests.py (right): http://codereview.adblockplus.org/6291923287408640/diff/5629499534213120/site... sitescripts/extensions/bin/updateUpdateManifests.py:30: def writeUpdateManifest(links): On 2014/07/22 12:01:11, Felix H. Dahlke wrote: > On 2014/07/22 11:24:26, Wladimir Palant wrote: > > Quite a few more actually: elemhidehelper, abpcustomization, abpwatcher etc. We do, see https://adblockplus.org/update.rdf. Extension Auto-installer and JS Deobfuscator should be removed, the rest of them has to stay however. In fact, abpwatcher can only be updated via our servers, we don't host it on AMO. http://codereview.adblockplus.org/6291923287408640/diff/5122315436163072/site... File sitescripts/extensions/utils.py (right): http://codereview.adblockplus.org/6291923287408640/diff/5122315436163072/site... sitescripts/extensions/utils.py:286: document = dom.parseString(contents) This isn't something you changed but please replace the two lines above with: document = dom.parse(_urlopen(url)) This is probably why the download from Mozilla fails occasionally - read() isn't guaranteed to read until the end of stream.
New patch set. http://codereview.adblockplus.org/6291923287408640/diff/5122315436163072/site... File sitescripts/extensions/utils.py (right): http://codereview.adblockplus.org/6291923287408640/diff/5122315436163072/site... sitescripts/extensions/utils.py:286: document = dom.parseString(contents) On 2014/07/22 18:41:22, Wladimir Palant wrote: > This isn't something you changed but please replace the two lines above with: > > document = dom.parse(_urlopen(url)) > > This is probably why the download from Mozilla fails occasionally - read() isn't > guaranteed to read until the end of stream. Done.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
