1048753 - Funsize tool handling is broken and incorrect

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Anhad Jai Singh (:ffledgling)]

The way funsize handles downloading tools is incorrect and sort of broken.
Right now it relies on a static file checked-in with the repo that contains the list of the "right" hashes. It downloads the tools if these hashes do no match. This way of doing things is fundamentally broken and doesn't lend itself to good development. Right now the hash matching almost always fails and the tools are downloaded afresh each time. This defeats the purpose of the https://github.com/mozilla/build-funsize/blob/master/senbonzakura/backend/tools.py ; What we can do instead is disable the always "failing" check altogether and assume we never have the tools and download them everytime.

This should work as fix until a better hash based verification scheme is figured out.

Comment 1

[Harsimran Singh]

This is the function which does the checking of the hashes https://github.com/mozilla/build-funsize/blob/master/senbonzakura/backend/tools.py#L33 and if it fails we download the tools.

So i think all we need to do is, remove this line ( https://github.com/mozilla/build-funsize/blob/master/senbonzakura/backend/tools.py#L78 ) so that the tools are downloaded everytime.

Is it so ?

Comment 2

[Anhad Jai Singh (:ffledgling)]

We need to remove https://github.com/mozilla/build-funsize/blob/master/senbonzakura/backend/tools.py#L78 and since this implies we do not use the function (https://github.com/mozilla/build-funsize/blob/master/senbonzakura/backend/tools.py#L33-L47) either, we can safely remove that function and the related variable that stores the path to the verification file as well (https://github.com/mozilla/build-funsize/blob/master/senbonzakura/backend/tools.py#L33-L47)

Comment 3

[Harsimran Singh]

(In reply to Anhad Jai Singh (:ffledgling) from comment #2)
> We need to remove
> https://github.com/mozilla/build-funsize/blob/master/senbonzakura/backend/
> tools.py#L78 and since this implies we do not use the function
> (https://github.com/mozilla/build-funsize/blob/master/senbonzakura/backend/
> tools.py#L33-L47) either, we can safely remove that function and the related
> variable that stores the path to the verification file as well
> (https://github.com/mozilla/build-funsize/blob/master/senbonzakura/backend/
> tools.py#L33-L47)

just a quick advice when you say remove does that mean like commenting out things or simply removing the code pieces from the file ?

Comment 4

[Anhad Jai Singh (:ffledgling)]

(In reply to Harsimran Singh from comment #3)

> just a quick advice when you say remove does that mean like commenting out
> things or simply removing the code pieces from the file ?

Since we're working under revision control it's safe to remove the code entirely rather than commenting it, we can always look at the old commits/history in-case we need the deletions back.

Comment 5

[Harsimran Singh]

Attachment: 0001-Bug-1048753-Funsize-tool-handling-is-broken-and-inco.patch

please have a look at this patch.

i tried running the test i could and manually verifying the if the tools were being downloaded but i could not run integration-test.sh on my machine due to the hash problem.

Comment 6

[Harsimran Singh]

I tried setting up Travis account to run tests on push at my end but even those fail due to the hash issue. Aren`t those independent of the system (like if they run for you they should also for me) ?

Comment 7

[Anhad Jai Singh (:ffledgling)]

Sorry about the delay on this one.
I'll answer both your questions one at a time.

(i) The integration test should be fine as long as it's *only* the hash matching step that fails, if you see *any* other errors in the meantime, there's a problem.
(ii) Travis tests aren't "independent" in the true sense of the word, they're just uniform because they're all run on Linux Boxes with identical configurations, if the travis tests are failing, something somewhere is wrong. There's a chance that it could be travis mis-config, but other errors are a little more likely.

Do you need any other info?

Comment 8

[Harsimran Singh]

(In reply to Anhad Jai Singh (:ffledgling) from comment #7)
> Sorry about the delay on this one.
> I'll answer both your questions one at a time.
> 
> (i) The integration test should be fine as long as it's *only* the hash
> matching step that fails, if you see *any* other errors in the meantime,
> there's a problem.

yes it was the hash matching step which caused the error.

> (ii) Travis tests aren't "independent" in the true sense of the word,
> they're just uniform because they're all run on Linux Boxes with identical
> configurations, if the travis tests are failing, something somewhere is
> wrong. There's a chance that it could be travis mis-config, but other errors
> are a little more likely.

Now i understand why the test failed at my end.

> 
> Do you need any other info?

No, Thanks for the above info.

Comment 9

[Harsimran Singh]

Just a reminder :ffledgling in case you didn't notice the need feedback tag.

Looking for to feedback to make necessary changes if any to make it usable.

Comment 10

[Anhad Jai Singh (:ffledgling)]

Comment on attachment 8472127 [details] [diff] [review]
0001-Bug-1048753-Funsize-tool-handling-is-broken-and-inco.patch

Sorry it took so long to get back to this, the patch looks good.

I'm going to apply it and push to the repo, if the tests pass then we can let the commit stay and close this bug.

Comment 11

[Anhad Jai Singh (:ffledgling)]

Comment on attachment 8472127 [details] [diff] [review]
0001-Bug-1048753-Funsize-tool-handling-is-broken-and-inco.patch

Review of attachment 8472127 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like the tests passed (https://travis-ci.org/mozilla/build-funsize/builds/33582659), so giving an r+ and committing.
Great work and again apologies for the delay.

Comment 12

[Anhad Jai Singh (:ffledgling)]

https://github.com/mozilla/build-funsize/commit/40cd7af9520caf3c9a71a188ce00f625d8f17168

Comment 13

[Pete Moore [:pmoore][:pete]]

This is awesome guys, thanks so much for your help with this! Very much appreciated. =)

Pete