Closed Bug 759469 Opened 7 years ago Closed 6 years ago

Add new updater instruction to add a file if it doesn't exist in the destination.

Categories

(Toolkit :: Application Update, defect)

15 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: rstrong, Assigned: rstrong)

References

Details

Attachments

(3 files, 8 obsolete files)

3.73 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
124.46 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
725.16 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
It would be handy to be able to add a file if it doesn't exist in the destination. In the case of Bug 756325 the channel-prefs.js file was accidentally moved to a new location and since we don't update that file so release build mar file updates can be applied to beta builds without changing the channel installations before the change have the channel-prefs.js file in one location (defaults/pref/) and installations performed after the change have the channel-prefs.js file in a different location (defaults/preferences). This would at least allow us to add the file to the location we decide upon if it doesn't exist in that location. This will require adding a new manifest as well.
Blocks: 657789
No longer blocks: 657789
Attached patch patch in progress (obsolete) — Splinter Review
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attached patch patch rev1 (obsolete) — Splinter Review
I still need to finish up the tests which I'll do in this bug and the packaging scripts which I'll do in another bug.
Attachment #628179 - Attachment is obsolete: true
Attachment #784063 - Attachment description: bug759469 → patch rev1
Attachment #784063 - Flags: review?(netzen)
Filed bug 900251 for updating the mar generation scripts.
Depends on: 900251
Comment on attachment 784063 [details] [diff] [review]
patch rev1

Messed up a couple of lines and fixed locally

>   // extract the manifest
>-  int rv = gArchiveReader.ExtractFile("updatev2.manifest", manifest);
>+  int rv = gArchiveReader.ExtractFile("updatev3.manifest", manifest);
>   if (rv) {
>-    rv = gArchiveReader.ExtractFile("update.manifest", manifest);
>+    rv = gArchiveReader.ExtractFile("updatev2.manifest", manifest);
>     if (rv) {
>-      LOG(("DoUpdate: error extracting manifest file"));
>-      return rv;
>-    }
>+      rv = gArchiveReader.ExtractFile("update.manifest", manifest);
>+      if (rv) {
>+        LOG(("DoUpdate: error extracting manifest file\n"));
>+        return rv;
>+     }
fixed locally
-     }
+      }
+    }

>   }
> 

>@@ -3717,16 +3787,19 @@ int DoUpdate()
>       action = new AddFile();
>     }
>     else if (NS_tstrcmp(token, NS_T("patch")) == 0) {
>       action = new PatchFile();
>     }
>     else if (NS_tstrcmp(token, NS_T("add-if")) == 0) { // Add if exists
>       action = new AddIfFile();
>     }
>+    else if (NS_tstrcmp(token, NS_T("add-if-not")) == 0) { // Add if not exists
>+      action = new AddNotFile();
fixed locally
-      action = new AddNotFile();
+      action = new AddIfNotFile();

>+    }
>     else if (NS_tstrcmp(token, NS_T("patch-if")) == 0) { // Patch if exists
Comment on attachment 784063 [details] [diff] [review]
patch rev1

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

::: toolkit/mozapps/update/updater/updater.cpp
@@ +1661,5 @@
> +
> +int
> +AddIfNotFile::Execute()
> +{
> +  if (mTestFile)

This should be if (!mTestFile)

@@ +1670,5 @@
> +
> +void
> +AddIfNotFile::Finish(int status)
> +{
> +  if (mTestFile)

This should be if (!mTestFile)

@@ +3717,2 @@
>      if (rv) {
> +      rv = gArchiveReader.ExtractFile("update.manifest", manifest);

I update.manifest even needed anymore if we are required to upgrade to an intermediate update?

@@ +3792,5 @@
>      else if (NS_tstrcmp(token, NS_T("add-if")) == 0) { // Add if exists
>        action = new AddIfFile();
>      }
> +    else if (NS_tstrcmp(token, NS_T("add-if-not")) == 0) { // Add if not exists
> +      action = new AddNotFile();

seen comment thx.
Attachment #784063 - Flags: review?(netzen)
(In reply to Brian R. Bondy [:bbondy] from comment #5)
> Comment on attachment 784063 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 784063 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/updater/updater.cpp
> @@ +1661,5 @@
> > +
> > +int
> > +AddIfNotFile::Execute()
> > +{
> > +  if (mTestFile)
> 
> This should be if (!mTestFile)
> 
> @@ +1670,5 @@
> > +
> > +void
> > +AddIfNotFile::Finish(int status)
> > +{
> > +  if (mTestFile)
> 
> This should be if (!mTestFile)
> 
> @@ +3717,2 @@
> >      if (rv) {
> > +      rv = gArchiveReader.ExtractFile("update.manifest", manifest);
> 
> I update.manifest even needed anymore if we are required to upgrade to an
> intermediate update?
I thought I removed this in bug 896224... I'll do it here.
Attached patch patch rev2 (obsolete) — Splinter Review
Comments addressed.

I still have to finish the python script for mar packaging and the tests which will be next.
Attachment #784063 - Attachment is obsolete: true
Attachment #784436 - Flags: review?(netzen)
Comment on attachment 784436 [details] [diff] [review]
patch rev2

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

::: toolkit/mozapps/update/updater/updater.cpp
@@ +3717,2 @@
>      if (rv) {
> +      LOG(("DoUpdate: error extracting manifest file\n"));

nit: \n is included already
Attachment #784436 - Flags: review?(netzen) → review+
carrying forward r+
Attachment #784436 - Attachment is obsolete: true
Attachment #784439 - Flags: review+
No longer blocks: 657789
Attached patch 3. updated test data files (obsolete) — Splinter Review
Attachment #8384538 - Flags: review?(netzen)
Attached patch 5. new add-if-not tests (obsolete) — Splinter Review
I have a tad more cleanup to do but wanted to get what I have so far reviewed. I'll submit the rest which is mainly marAppApply* test cleanup in another patch.

Try push
https://tbpl.mozilla.org/?tree=Try&rev=73c4aaa87094
Attachment #8384540 - Flags: review?(netzen)
Attachment #784439 - Attachment description: patch rev3 comment addressed → 1. main patch rev3 comment addressed
Attached patch 6. more test cleanup rev2 (obsolete) — Splinter Review
fixed up more of the comments
Attachment #8385199 - Attachment is obsolete: true
Attachment #8385199 - Flags: review?(netzen)
Attachment #8385413 - Flags: review?(netzen)
Attachment #8384537 - Flags: review?(netzen) → review+
Attachment #8384538 - Flags: review?(netzen) → review+
Attachment #8384540 - Flags: review?(netzen) → review+
Attachment #8385413 - Flags: review?(netzen) → review+
Comment on attachment 8384539 [details] [diff] [review]
4. updated tests and cleanup

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

8| as far as I can tell this looks good :)
Attachment #8384539 - Flags: review?(netzen) → review+
Combined patch for test changes in patches 3 - 6.
Attachment #8384538 - Attachment is obsolete: true
Attachment #8384539 - Attachment is obsolete: true
Attachment #8384540 - Attachment is obsolete: true
Attachment #8385413 - Attachment is obsolete: true
Attachment #8386282 - Flags: review+
Pushed to mozilla-central
https://hg.mozilla.org/mozilla-central/rev/75aef72bf1be
https://hg.mozilla.org/mozilla-central/rev/5265254ba734
https://hg.mozilla.org/mozilla-central/rev/c1c3aa2b09d8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment on attachment 784439 [details] [diff] [review]
1. main patch rev3 comment addressed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: Releng will need to perform additional work to create update mar files for additional cycles to update beta users to release bits.
Testing completed (on m-c, etc.): This has been on m-c for several days, I've manually verified thoroughly, tests in bug 759469
Risk to taking this patch (and alternatives if risky): Minimal
String or IDL/UUID changes made by this patch: None
Attachment #784439 - Flags: approval-mozilla-aurora?
Attachment #784439 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Moving milestone to 29 to reflect uplift to aurora.
Target Milestone: mozilla30 → mozilla29
You need to log in before you can comment on or make changes to this bug.