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

RESOLVED FIXED in Firefox 27

Status

()

defect
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

Trunk
Firefox 27
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

58.17 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
Posted patch sample patch (obsolete) — Splinter Review
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/
Component: General → Installer
QA Contact: general → installer
Posted patch patch rev1 (obsolete) — Splinter Review
Assignee: nobody → robert.bugzilla
Attachment #525657 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #527966 - Flags: review?(khuey)
Posted patch patch rev2 (obsolete) — Splinter Review
bah... had a merge conflict in the patch
Attachment #527966 - Attachment is obsolete: true
Attachment #527966 - Flags: review?(khuey)
Attachment #527967 - Flags: review?(khuey)
Comment on attachment 527967 [details] [diff] [review]
patch rev2

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

r=me
Attachment #527967 - Flags: review?(khuey) → review+
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. :(
Attachment #527967 - Attachment is obsolete: true
Attachment #527967 - Flags: review+
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!
Depends on: 515492
Summary: Update removed-files.in to remove directories → Update removed-files.in after requiring update to the version with bug 515492
s/bug 515492/bug 563318/
Summary: Update removed-files.in after requiring update to the version with bug 515492 → Update removed-files.in after requiring update to the version with bug 563318
Posted patch 1. Main Patch rev1 (obsolete) — Splinter Review
Brian, if you are ok with these changes could you push the patch to oak?
Attachment #784023 - Flags: review?(netzen)
(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?
Only this bug. There are dependencies in the other ones that need to happen at the same time.
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.
Attachment #784023 - Flags: review?(netzen) → review+
They are sorted in the mar script so the order in removed-files.in doesn't matter except when it comes to readability.
(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.
Yes, they are reverse sorted (sort -r). Implemented in bug 386760
great, thanks for checking.
(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.
Pushed to oak and started a nightly build, I'll kick a second one off shortly.
https://tbpl.mozilla.org/?tree=Oak&rev=eeef87df536f
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.
Attachment #785233 - Flags: review?(netzen)
cc'ing some releng folks to give them a heads up regarding this landing soon.
Attachment #784023 - Attachment description: Patch rev1 → 1. Main Patch rev1
Attachment #785233 - Flags: review?(netzen) → review+
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.
Flags: in-testsuite-
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25
Posted patch Combined patch as pushed (obsolete) — Splinter Review
Attachment #784023 - Attachment is obsolete: true
Attachment #785233 - Attachment is obsolete: true
Attachment #785302 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f87e93861239
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Depends on: 901921
Backed out for causing bug 901921:
remote:   https://hg.mozilla.org/mozilla-central/rev/1e381c91885d

Nightly respun.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Posted patch Patch rev2 (obsolete) — Splinter Review
Attachment #785302 - Attachment is obsolete: true
Attachment #793737 - Flags: review?(netzen)
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.
Attachment #793737 - Flags: review?(netzen) → review+
I'm going to push the patch to elm again
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
https://hg.mozilla.org/mozilla-central/rev/790dfb757a70
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED

Comment 31

6 years ago
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.

Updated

6 years ago
Assignee: robert.bugzilla → gijskruitbosch+bugs

Comment 32

6 years ago
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. :-(
Attachment #806558 - Attachment is obsolete: true

Updated

6 years ago
Assignee: gijskruitbosch+bugs → robert.bugzilla
You need to log in before you can comment on or make changes to this bug.