649607 - Update removed-files.in after requiring update to the version with bug 563318

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

[Robert Strong (they/them - no direct email)]

Attachment: sample patch

Now that bug 386760 has landed it is possible to remove directories on application update.

To remove a directory only if it is empty a path should be added as follows:
parent_dir/sub_dir/

To remove a directory and all of its contents including subdirectories a path should be added as follows:
parent_dir/sub_dir/*

For the recursive removal if a file can't be removed the update will fail.

For both cases if a directory can't be removed the update will succeed.

I'm attaching a patch that cleans up some of Firefox's removed-files.in

Though the removed-files.in file is not used by the installer anymore I am filing in the installer since it is similar to app update and the removed-files.in file lives under browser/installer/

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

bah... had a merge conflict in the patch

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 527967 [details] [diff] [review]
patch rev2

Shame there's no XP_LINUX to clean up that chrome/ removal.

r=me

Comment 4

[Robert Strong (they/them - no direct email)]

Comment on attachment 527967 [details] [diff] [review]
patch rev2

There is a bit of a chicken and egg situation with this in that we need these updates to be available for older clients without the fix so the cleanup can't happen yet. :(

Comment 5

[Robert Strong (they/them - no direct email)]

This will be much easier when we fix bug 515492 since it will require updating to either an earlier Firefox which supports directory removal prior to updating to a build that has bug 515492 fixed. We will also be able to remove most of the old entries from removed-files.in at the same time. woohoo!

Comment 6

[Robert Strong (they/them - no direct email)]

s/bug 515492/bug 563318/

Comment 7

[Robert Strong (they/them - no direct email)]

Attachment: 1. Main Patch rev1

Brian, if you are ok with these changes could you push the patch to oak?

Comment 8

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #7)
> Created attachment 784023 [details] [diff] [review]
> Patch rev1
> 
> Brian, if you are ok with these changes could you push the patch to oak?

Only this bug? or this + any of the other recent ones you requested review for?

Comment 9

[Robert Strong (they/them - no direct email)]

Only this bug. There are dependencies in the other ones that need to happen at the same time.

Comment 10

[Brian R. Bondy [:bbondy]]

Comment on attachment 784023 [details] [diff] [review]
1. Main Patch rev1

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

Based on Comment 5, this looks good.
Reviewing other patches at the same time.

::: browser/installer/removed-files.in
@@ +68,5 @@
> +components/
> +defaults/profile/
> +defaults/profile/autoconfig/
> +defaults/profile/chrome/
> +defaults/profile/US/*

Shouldn't defaults/profile/ come last?
Seems like the first one will fail because it may contain defaults/profile/autoconfig/ and defaults/profile/chrome/ and defaults/profile/US/*

Ditto /extensions below? maybe the order doesn't matter.

Comment 11

[Robert Strong (they/them - no direct email)]

They are sorted in the mar script so the order in removed-files.in doesn't matter except when it comes to readability.

Comment 12

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #11)
> They are sorted in the mar script so the order in removed-files.in doesn't
> matter except when it comes to readability.
Actually, let me double check... better safe than sorry.

Comment 13

[Robert Strong (they/them - no direct email)]

Yes, they are reverse sorted (sort -r). Implemented in bug 386760

Comment 14

[Brian R. Bondy [:bbondy]]

great, thanks for checking.

Comment 15

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #10)
> Comment on attachment 784023 [details] [diff] [review]
> Patch rev1
> 
> Review of attachment 784023 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Based on Comment 5, this looks good.
> Reviewing other patches at the same time.
Note: the other patches only depend on each other mainly so I only have to generate the test mars once.

Comment 16

[Brian R. Bondy [:bbondy]]

Pushed to oak and started a nightly build, I'll kick a second one off shortly.
https://tbpl.mozilla.org/?tree=Oak&rev=eeef87df536f

Comment 17

[Robert Strong (they/them - no direct email)]

Attachment: 2. patch fix path to autoconfig

Tested functionality by updating from
3.6.28 to 12 to latest release and then copied over oak from a couple of days ago to verify complete updates.
3.6.28 to 12 to latest release and then copied over oak from yesterday to verify partial updates.

The only problem I found is the path to the autoconfig directory was incorrect.

Comment 18

[Robert Strong (they/them - no direct email)]

cc'ing some releng folks to give them a heads up regarding this landing soon.

Comment 19

[Robert Strong (they/them - no direct email)]

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/f87e93861239

setting in-testsuite-
We do have tests for the functionality provided by removed-files.in but there are no tests for the files specified in removed-files.in.

Comment 20

[Robert Strong (they/them - no direct email)]

Attachment: Combined patch as pushed

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/f87e93861239

Comment 22

[Ed Morley [:emorley]]

Backed out for causing bug 901921:
remote:   https://hg.mozilla.org/mozilla-central/rev/1e381c91885d

Nightly respun.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

And Aurora.
https://hg.mozilla.org/releases/mozilla-aurora/rev/42bd5e547c30

Comment 24

[Robert Strong (they/them - no direct email)]

Attachment: Patch rev2

Comment 25

[Robert Strong (they/them - no direct email)]

The only difference is the removal of res/*. I verified that the removals in the res/ directory were present in Firefox 12 so the removals for this directory are no longer needed.

Comment 26

[Robert Strong (they/them - no direct email)]

I'm going to push the patch to elm again

Comment 27

[Brian R. Bondy [:bbondy]]

you want oak, have at it.
After pushing you can issue a nightly build by pasting the latest changeset id to https://secure.pub.build.mozilla.org/buildapi/self-serve/oak

Comment 28

[Robert Strong (they/them - no direct email)]

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/790dfb757a70

Comment 29

[Robert Strong (they/them - no direct email)]

Attachment: Patch as checked in - updated to tip and comment changed

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/790dfb757a70

Comment 31

[:Gijs (he/him)]

Attachment: Most entries in removed-files.in are no longer needed - - Update removed-files.in after requiring update to the version with bug 563318.

This patch works, but it breaks mochitest-browser for me locally. It hangs waiting to start browser_cmd_settings.js, but only if browser_cmd_screenshot.js has run before it. I have poked at this for about an hour now, but I don't understand why this patch would affect that. I've tried sticking all the code I added in try catch blocks with Cu.reportErrors, and no errors are reported, so I'm a bit lost as to what the issue is.

Comment 32

[:Gijs (he/him)]

Comment on attachment 806558 [details] [diff] [review]
Most entries in removed-files.in are no longer needed -  - Update removed-files.in after requiring update to the version with bug 563318.

Apologies. It would be nice if bzexport warned before dumping tip into an existing bug instead of the patch you just qpopped into the bug you really meant. :-(