Funsize tool handling is broken and incorrect

RESOLVED FIXED

Status

Release Engineering
General
--
minor
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: ffledgling, Assigned: Harsimran Singh, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [funsize])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Mentor: ffledgling@gmail.com
Whiteboard: [funsize]
(Assignee)

Comment 1

3 years ago
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 ?
(Reporter)

Comment 2

3 years ago
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)
Assignee: nobody → harsimran_hs4
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
(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 ?
(Reporter)

Comment 4

3 years ago
(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.
(Assignee)

Comment 5

3 years ago
Created attachment 8472127 [details] [diff] [review]
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.
Attachment #8472127 - Flags: feedback?(ffledgling)
(Assignee)

Comment 6

3 years ago
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) ?
(Reporter)

Comment 7

3 years ago
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?
Flags: needinfo?(harsimran_hs4)
(Assignee)

Comment 8

3 years ago
(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.
Flags: needinfo?(harsimran_hs4)
(Assignee)

Comment 9

3 years ago
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.
(Reporter)

Comment 10

3 years ago
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.
(Reporter)

Comment 11

3 years ago
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.
Attachment #8472127 - Flags: feedback?(ffledgling) → review+
(Reporter)

Comment 12

3 years ago
https://github.com/mozilla/build-funsize/commit/40cd7af9520caf3c9a71a188ce00f625d8f17168
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
This is awesome guys, thanks so much for your help with this! Very much appreciated. =)

Pete
Component: Tools → General
Product: Release Engineering → Release Engineering
You need to log in before you can comment on or make changes to this bug.