Ensure prebuilts are actually de-duplicated. [chromiumos/chromite : master]

15 views
Skip to first unread message

David James (Code Review)

unread,
Nov 4, 2012, 7:24:14 PM11/4/12
to Gerrit, Ryan Cui
David James has posted comments on this change.

Change subject: Ensure prebuilts are actually de-duplicated.
......................................................................


Patch Set 1: Verified

Verified on real builder to actually fix all prebuilt de-duplication... so upload_prebuilts stage should basically upload nothing now unless something is changed.

--
To view, visit https://gerrit.chromium.org/gerrit/37333
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I51ca148ff40171bc766dda640208c1c6f3750055
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>
Gerrit-Reviewer: David James <david...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Ryan Cui <rc...@chromium.org>

Ryan Cui (Code Review)

unread,
Nov 5, 2012, 1:57:59 PM11/5/12
to David James, Gerrit, Ryan Cui
Ryan Cui has posted comments on this change.

Change subject: Ensure prebuilts are actually de-duplicated.
......................................................................


Patch Set 1: (3 inline comments)

....................................................
File lib/binpkg.py
Line 65: oldpkg = db.get(sha1, _Package(0, None))
Why is this necessary? Are there duplicate entries in self.packages?

....................................................
File lib/gs.py
Line 25: def CanonicalizeURL(url):
What do you think about using the NormalizeGSPath function I have here? https://gerrit.chromium.org/gerrit/#/c/36020/10/lib/commandline.py

....................................................
File scripts/upload_prebuilts_unittest.py
Line 143: def testNormalIndex(self):
These tests don't identify the bug that this CL is fixing. Perhaps add a regression test for the case of different URI prefixes?

David James (Code Review)

unread,
Nov 5, 2012, 11:59:42 PM11/5/12
to Gerrit, Ryan Cui
David James has posted comments on this change.

Change subject: Ensure prebuilts are actually de-duplicated.
......................................................................


Patch Set 1: (3 inline comments)

....................................................
File lib/binpkg.py
Line 65: oldpkg = db.get(sha1, _Package(0, None))
There are no duplicates in self.packages. However, we're pulling packages from several sources, each of which may have different versions of the binary.

....................................................
File lib/gs.py
Line 25: def CanonicalizeURL(url):
That function is more complicated, and doesn't handle as many cases (e.g. it doesn't both the sandbox URLs and the commondatastorage URLs.)

....................................................
File scripts/upload_prebuilts_unittest.py
Line 143: def testNormalIndex(self):
OK, I added a bunch more unit tests and I also cleaned up these unit tests a bit to avoid duplication of logic.

Ryan Cui (Code Review)

unread,
Nov 6, 2012, 1:59:59 PM11/6/12
to David James, Gerrit, Ryan Cui
Ryan Cui has posted comments on this change.

Change subject: Ensure prebuilts are actually de-duplicated.
......................................................................


Patch Set 1: (2 inline comments)

....................................................
File lib/gs.py
Line 25: def CanonicalizeURL(url):
From discussion, will standardize on this function since it's simpler.

Line 27: for prefix in prefixes:
Maybe raise an exception if no prefixes were matched and url does not start with gs://?

Ryan Cui (Code Review)

unread,
Nov 6, 2012, 2:54:10 PM11/6/12
to David James, Gerrit, Ryan Cui
Ryan Cui has posted comments on this change.

Change subject: Ensure prebuilts are actually de-duplicated.
......................................................................


Patch Set 2: Looks good to me, approved

--
To view, visit https://gerrit.chromium.org/gerrit/37333
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I51ca148ff40171bc766dda640208c1c6f3750055
Gerrit-PatchSet: 2

David James (Code Review)

unread,
Nov 6, 2012, 3:00:39 PM11/6/12
to Gerrit, Ryan Cui
David James has posted comments on this change.

Change subject: Ensure prebuilts are actually de-duplicated.
......................................................................


Patch Set 2: Verified; Ready
Reply all
Reply to author
Forward
0 new messages