1059467 - Move precomplete file from the root of the Mac bundle to Contents/Resources

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

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

On Mac OS the precomplete file will need to be moved to Contents/Resources

Comment 1

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

Attachment: Mar packaging code and precomplete generation (wip)

Comment 2

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

Attachment: Mar packaging code and precomplete generation (wip)

removed-files change had snuck into the first patch

Comment 3

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

IIRC we put precomplete at the top of the .app to force OSX to refresh something after an update (icon, or version in the Get Info maybe). Do we have to accept a regression as part of the packaging changes ?

Comment 4

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

Already handled. There is code to perform essentially a touch to the app bundle after a successful update. I had hope that this one-off code could one day be removed with the addition of the precomplete file that I added as part of Firefox 5 since that does the same thing but with it moving the code that performs the touch will suffice.

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#2315

Comment 5

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

Attachment: updater patch (wip)

I need to modify the test mar files before I can change this properly but this suffices for now.

Comment 6

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

Comment on attachment 8480244 [details] [diff] [review]
Mar packaging code and precomplete generation (wip)

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

::: tools/update-packaging/make_incremental_updates.py
@@ +335,5 @@
>      to_dir_path = os.path.abspath(to_dir_path)
>      # Create a hashtable of the from  and to directories
>      from_dir_hash,from_file_set,from_dir_set = patch_info.build_marfile_entry_hash(from_dir_path)
>      to_dir_hash,to_file_set,to_dir_set = patch_info.build_marfile_entry_hash(to_dir_path)
> +    # Create a list of the forced updates 

Nit, trailing whitespace.

@@ +348,5 @@
> +                forced_list.append("Contents/Resources/precomplete")
> +            else:
> +                forced_list.append("Contents\Resources\precomplete")
> +    else:
> +        forced_list.append("precomplete")

Alternatively something like:
for location in ('precomplete', 'Contents/Resources/precomplete', 'Contents\Resources\precomplete'):
    if location in to_file_set:
         forced_list.append(location)
         break
else:
    raise Exception, "missing precomplete file in: "+to_dir_path

Comment 7

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

(In reply to Nick Thomas [:nthomas] from comment #6)
>...
> @@ +348,5 @@
> > +                forced_list.append("Contents/Resources/precomplete")
> > +            else:
> > +                forced_list.append("Contents\Resources\precomplete")
> > +    else:
> > +        forced_list.append("precomplete")
> 
> Alternatively something like:
> for location in ('precomplete', 'Contents/Resources/precomplete',
> 'Contents\Resources\precomplete'):
>     if location in to_file_set:
>          forced_list.append(location)
>          break
> else:
>     raise Exception, "missing precomplete file in: "+to_dir_path

That will raise an exception except when the first item is found so I went with what I originally had.

Comment 8

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

Attachment: Mar packaging code and precomplete generation

Comment 9

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

Attachment: updater patch

The test changes to support v2 signing are massive and will be done in bug bug1058182.

Comment 10

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

Attachment: updater patch

I reverted one change so the tests continue to pass on Mac without creating the new mar files. I'll create those as soon as I am able.

Comment 11

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

Attachment: Create precomplete before enumerating files

This way the precomplete file is always listed in the precomplete file to lessen the chance that the precomplete file will be different than the one present when generating the bundle's signature so the signature won't be invalid.

Comment 12

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

Attachment: Create precomplete before enumerating files

stray tab snuck in

Comment 13

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

Comment on attachment 8489186 [details] [diff] [review]
Create precomplete before enumerating files

r+, assuming the updater has read and closed precomplete before trying to remove it.

Comment 14

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

It is

Comment 15

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

Attachment: Combined patch

Brian, can I get an r+ for the following change to the original updater.cpp patch which is now possible with the updated mar files in bug 1058182.


diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
--- a/toolkit/mozapps/update/updater/updater.cpp
+++ b/toolkit/mozapps/update/updater/updater.cpp
@@ -3646,27 +3619,27 @@ GetManifestContents(const NS_tchar *mani
 }
 
 int AddPreCompleteActions(ActionList *list)
 {
   if (sIsOSUpdate) {
     return OK;
   }
 
+#ifdef XP_MACOSX
+  NS_tchar *rb = GetManifestContents(NS_T("Contents/Resources/precomplete"));
+#else
   NS_tchar *rb = GetManifestContents(NS_T("precomplete"));
+#endif
   if (rb == nullptr) {
-    //XXX Temporary since requiring this location on Mac OS X will require making new test mar files
-    rb = GetManifestContents(NS_T("Contents/Resources/precomplete"));
-    if (rb == nullptr) {
-      LOG(("AddPreCompleteActions: error getting contents of precomplete " \
-           "manifest"));
-      // Applications aren't required to have a precomplete manifest. The mar
-      // generation scripts enforce the presence of a precomplete manifest.
-      return OK;
-    }
+    LOG(("AddPreCompleteActions: error getting contents of precomplete " \
+         "manifest"));
+    // Applications aren't required to have a precomplete manifest. The mar
+    // generation scripts enforce the presence of a precomplete manifest.
+    return OK;
   }
 
   int rv;
   NS_tchar *line;
   while((line = mstrtok(kNL, &rb)) != 0) {
     // skip comments
     if (*line == NS_T('#'))
       continue;

Comment 16

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #15)
> Created attachment 8489879 [details] [diff] [review]
> Combined patch
> 
> Brian, can I get an r+ for the following change to the original updater.cpp
> patch which is now possible with the updated mar files in bug 1058182.
> 
> 
> diff --git a/toolkit/mozapps/update/updater/updater.cpp
> b/toolkit/mozapps/update/updater/updater.cpp
> --- a/toolkit/mozapps/update/updater/updater.cpp
> +++ b/toolkit/mozapps/update/updater/updater.cpp
> @@ -3646,27 +3619,27 @@ GetManifestContents(const NS_tchar *mani
>  }
>  
>  int AddPreCompleteActions(ActionList *list)
>  {
>    if (sIsOSUpdate) {
>      return OK;
>    }
>  
> +#ifdef XP_MACOSX
> +  NS_tchar *rb =
> GetManifestContents(NS_T("Contents/Resources/precomplete"));
> +#else
>    NS_tchar *rb = GetManifestContents(NS_T("precomplete"));
> +#endif
>    if (rb == nullptr) {
> -    //XXX Temporary since requiring this location on Mac OS X will require
> making new test mar files
> -    rb = GetManifestContents(NS_T("Contents/Resources/precomplete"));
> -    if (rb == nullptr) {
> -      LOG(("AddPreCompleteActions: error getting contents of precomplete " \
> -           "manifest"));
> -      // Applications aren't required to have a precomplete manifest. The
> mar
> -      // generation scripts enforce the presence of a precomplete manifest.
> -      return OK;
> -    }
> +    LOG(("AddPreCompleteActions: error getting contents of precomplete " \
> +         "manifest"));
> +    // Applications aren't required to have a precomplete manifest. The mar
> +    // generation scripts enforce the presence of a precomplete manifest.
> +    return OK;
>    }
>  
>    int rv;
>    NS_tchar *line;
>    while((line = mstrtok(kNL, &rb)) != 0) {
>      // skip comments
>      if (*line == NS_T('#'))
>        continue;

yep :)

Comment 17

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/2d142498cdea

Comment 18

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

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

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

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

Landed on aurora in the Mac V2 signing combined patch in bug 1047584