1139873 - [mozinstall] get_binary() should not remove the build folder if the specified binary cannot be found

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Right now get_binary() is removing the whole build folder if the expected binary is not found. I consider this a dataloss.

http://mxr.mozilla.org/comm-central/source/mozilla/testing/mozbase/mozinstall/mozinstall/mozinstall.py#84

IMO the calling code has to make a decision what to do with the folder, if the build has been installed. If code is using a real binary folder without installation, we currently are removing this folder!

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Andrew, and William what do you think? When we have an agreement I would like to set this up as a mentored bug. Thanks.

Comment 2

[Andrew Halberstadt [:ahal]]

Yeah, that seems pretty bad! Should definitely be fixed.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Alright. Lets see if we can find someone who has interest to work on this bug. I will put it up as mentored.

Comment 4

[Jason Gersztyn]

Hey there. I would definitely like to take this on. This will be my second bug, so I still may need a little guidance, but am very excited to work on it.

I have found the file location in my local database. So what exactly should I do here? Have another check which does not delete the folder under a circumstance?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It's even easier than that. Just remove the lines which delete files of the installation folder in case the binary cannot be found. That's all. Afterward please run all the mozbase tests via tests.py to ensure that all tests are still passing.

Comment 6

[Jason Gersztyn]

Right, right. That's almost too easy! I will get it changed, test, and submit the patch soon.

Comment 7

[parthbakshi]

Hey there, I would like to work on this bug if no progress has been made yet. This is my first bug so will definitely need guidance. Let me know if the patch has not been submitted yet.

Comment 8

[Jason Gersztyn]

No progress. Got too busy with college and have had no time for open source. Hopefully one of these fine people can give you access to it, since I don't need it anymore.

Comment 9

[Andrew Halberstadt [:ahal]]

Hi, thanks for your interest! The mentor of this bug (whimboo) is away until next week, but it looks like there is enough information in here for you to get started. See the link in comment 0 and the hint in comment 5. Feel free to ask questions if you get stuck.

Comment 10

[parthbakshi]

I have used Mercurial in the past, do i just submit a patch in this ticket, or merge somewhere?

Comment 11

[Andrew Halberstadt [:ahal]]

Yes, you can just attach a patch to this bug. You may want to take a quick look through:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide

Especially the "How to submit a patch" section. Cheers!

Comment 12

[parthbakshi]

Attachment: Bug 1139873 - [mozinstall] get_binary() will not remove the build folder

I have attached the patch for review, let me know if i need to make any changes.

Comment 13

[parthbakshi]

All tests were successful after applying the patch.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8599638 [details] [diff] [review]
Bug 1139873 - [mozinstall] get_binary() will not remove the build folder

Thank you for the patch. I will set the review flag for me so that I can have a look at it soon.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8599638 [details] [diff] [review]
Bug 1139873 - [mozinstall] get_binary() will not remove the build folder

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

f+ with just a nit to fix. Once done we can get your patch landed.

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py
@@ -83,5 @@
>  
>      if not binary:
>          # The expected binary has not been found. Make sure we clean the
>          # install folder to remove any traces
> -        mozfile.remove(path)

The removal looks fine, but also get rid of the comment which does not apply anymore.

Comment 16

[parthbakshi]

Attachment: Updated comments when binary not found.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8603731 [details] [diff] [review]
Updated comments when binary not found.

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

Thanks for the update. FYI r+ can only be set by a reviewer. You want to ask for review, so it will be the same as what I do now. I will have a look at it later.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8603731 [details] [diff] [review]
Updated comments when binary not found.

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

This looks good. Parth, have you run the local unit tests for mozbase to ensure that your change in mozinstall didn't cause any other test to fail? You can find the runner in mozbase/test.py.

Comment 19

[parthbakshi]

I have run all the tests, I did not see any errors.

python mozbase/test.py 
SUITE-START | Running 74 tests
...
TEST-PASS | test_apk.ApkTest.test_with_package_name | took 2ms
SUITE-END | took 111s

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6800629ce1b1

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That's great. Thanks Parth! Given that the tests are passing I'm confident that we can land this patch without a try run. Thanks for your contribution and please let me know if there is something else you want to work on.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6800629ce1b1