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

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

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

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.

Comment 1

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

Attachment: patch in progress

Comment 2

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

Attachment: patch rev1

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.

Comment 3

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

Filed bug 900251 for updating the mar generation scripts.

Comment 4

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

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 5

[Brian R. Bondy [:bbondy]]

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.

Comment 6

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

(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.

Comment 7

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

Attachment: patch rev2

Comments addressed.

I still have to finish the python script for mar packaging and the tests which will be next.

Comment 8

[Brian R. Bondy [:bbondy]]

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

Comment 9

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

Attachment: 1. main patch rev3 comment addressed

carrying forward r+

Comment 10

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

Attachment: 2. new and renamed binaries

Comment 11

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

Attachment: 3. updated test data files

Comment 12

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

Attachment: 4. updated tests and cleanup

Comment 13

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

Attachment: 5. new add-if-not tests

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

Comment 14

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

Attachment: 6. more test cleanup

All patched pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=5453ea12fdf9

Comment 15

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

Attachment: 6. more test cleanup rev2

fixed up more of the comments

Comment 16

[Brian R. Bondy [:bbondy]]

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 :)

Comment 17

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

Attachment: 3_bug759469_tests

Combined patch for test changes in patches 3 - 6.

Comment 18

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

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

Comment 19

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

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

Comment 20

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

Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/a34be11408a6
https://hg.mozilla.org/releases/mozilla-aurora/rev/52238086bd70
https://hg.mozilla.org/releases/mozilla-aurora/rev/ac69d3de936c

Comment 21

[Nick Thomas [:nthomas] (UTC+12)]

Moving milestone to 29 to reflect uplift to aurora.