Closed Bug 1059467 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: Application Update, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: rstrong, Assigned: rstrong)

References

Details

Attachments

(1 file, 8 obsolete files)

14.91 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
On Mac OS the precomplete file will need to be moved to Contents/Resources
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
Attached patch updater patch (wip) (obsolete) — Splinter Review
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.
Attached patch updater patch (obsolete) — Splinter Review
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)
Attached patch updater patch (obsolete) — Splinter Review
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+
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)
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+
Attached patch Combined patchSplinter Review
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 #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/mozilla-central/rev/d3025e55dfc3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Landed on aurora in the Mac V2 signing combined patch in bug 1047584
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.