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

RESOLVED FIXED in Firefox 34

Status

()

Toolkit
Application Update
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

Trunk
mozilla35
x86
Mac OS X
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox34 fixed, firefox35 fixed)

Details

Attachments

(1 attachment, 8 obsolete attachments)

On Mac OS the precomplete file will need to be moved to Contents/Resources
Created attachment 8480075 [details] [diff] [review]
Mar packaging code and precomplete generation (wip)
Created attachment 8480244 [details] [diff] [review]
Mar packaging code and precomplete generation (wip)

removed-files change had snuck into the first patch
Attachment #8480075 - Attachment is obsolete: true
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 ?
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
Attachment #8480244 - Flags: review?(nthomas)
Created attachment 8481061 [details] [diff] [review]
updater patch (wip)

I need to modify the test mar files before I can change this properly but this suffices for now.
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
Attachment #8480244 - Flags: review?(nthomas) → review+
(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.
Created attachment 8486658 [details] [diff] [review]
Mar packaging code and precomplete generation
Attachment #8480244 - Attachment is obsolete: true
Attachment #8486658 - Flags: review+
Created attachment 8486977 [details] [diff] [review]
updater patch

The test changes to support v2 signing are massive and will be done in bug bug1058182.
Attachment #8481061 - Attachment is obsolete: true
Attachment #8486977 - Flags: review?(netzen)
Created attachment 8487752 [details] [diff] [review]
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.
Attachment #8486977 - Attachment is obsolete: true
Attachment #8486977 - Flags: review?(netzen)
Attachment #8487752 - Flags: review?(netzen)
Attachment #8487752 - Flags: review?(netzen) → review+
Created attachment 8489185 [details] [diff] [review]
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.
Attachment #8489185 - Flags: review?(nthomas)
Created attachment 8489186 [details] [diff] [review]
Create precomplete before enumerating files

stray tab snuck in
Attachment #8489185 - Attachment is obsolete: true
Attachment #8489185 - Flags: review?(nthomas)
Attachment #8489186 - Flags: review?(nthomas)
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.
Attachment #8489186 - Flags: review?(nthomas) → review+
It is
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;
Attachment #8489879 - Flags: review?(netzen)
Attachment #8486658 - Attachment is obsolete: true
Attachment #8487752 - Attachment is obsolete: true
Attachment #8489186 - Attachment is obsolete: true
Attachment #8489879 - Flags: review?(netzen) → review+
(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 :)
https://hg.mozilla.org/projects/oak/rev/2d142498cdea
Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/d3025e55dfc3
https://hg.mozilla.org/mozilla-central/rev/d3025e55dfc3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Landed on aurora in the Mac V2 signing combined patch in bug 1047584
status-firefox34: --- → fixed
status-firefox35: --- → fixed
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.