Closed Bug 900251 Opened 6 years ago Closed 6 years ago

Add support for add-if-not instruction added by bug 759469 to the mar generation scripts


(Release Engineering :: General, defect)

Windows 7
Not set


(firefox29 fixed, firefox30 fixed)

Tracking Status
firefox29 --- fixed
firefox30 --- fixed


(Reporter: rstrong, Assigned: rstrong)




(2 files, 5 obsolete files)

This way we can add back channel-prefs.js if it gets removed as happened in bug 756325.
Attached patch patch in progress (obsolete) — Splinter Review
This passes the tests for the scripts... still have some cleanup to do.
Product: → Release Engineering
Attached patch patch rev1 (obsolete) — Splinter Review
Hey Nick, these changes add the ability to add a file if it doesn't exist and exclude update-settings.ini from the precomplete manifest. I added readded the distribution/extensions directory code (e.g. add-if and patch-if). The patch requires the patch from bug 896223. I just need to verify a couple of more test changes in bug 759469 to land the client side changes.
Attachment #785446 - Attachment is obsolete: true
Attachment #8377492 - Flags: review?(nthomas)
Comment on attachment 8377492 [details] [diff] [review]
patch rev1

r- for issues around deployment and inadvertent argument change for $MAR call. The rest is nits or python style suggestions.

>diff --git a/tools/update-packaging/ b/tools/update-packaging/
>   find . -type f \
>-    ! -name "channel-prefs.js" \

Will this bug land a few days after bug 759469, so that most users have an updater that speaks v3 manifest ? If not, the first update generated with this chunk will remove update-settings.ini (via the precomplete), and the next update should put it back (via add-if-not and v3 manifest). Could hold this chunk back.

>diff --git a/tools/update-packaging/ b/tools/update-packaging/
+      notice "    diffing \"$f\""

I like there's some output when diffing large files like the XUL library, but this appears amongst lines with genuine actions. Adding something to indicate this is informational only would be good, maybe just a leading ## or not center justifying. Same in

>+    if [ `basename $f` = "channel-prefs.js" ]; then
>+      notice "new channel-prefs.js file found: $f"
>+      exit 1
>+    fi
>+    if [ `basename $f` = "update-settings.ini" ]; then
>+      notice "new update-settings.ini file found: $f"
>+      exit 1
>+    fi

In testing with this patch, I found that this block breaks generating a partial the first build after it lands. These files are in the 'to' complete mar for the first time, but it's not an error case. The nightly builds will stop without uploading installer or complete mar.

>+    make_add_if_not_instruction "$f" "$updatemanifestv3" "$contents"

Nit, the $contents arg isn't used by the function.

>-eval "$MAR -C \"$workdir\" -c output.mar $archivefiles"
>+eval "$MAR -C \"$workdir\" -H xpcshell-test -V \"*\" -c output.mar $archivefiles"

Looks like test only change in non-test file.

>diff --git a/tools/update-packaging/ b/tools/update-packaging/
>     def create_manifest_files(self):
>         """ Createst the v2 manifest file in the root of the work_dir """

Nit while we're here - Createst typo

>     # Files which exist in both sets need to be patched
>     patch_filenames = list(from_file_set.intersection(to_file_set))
>     patch_filenames.sort(reverse=True)
>     for filename in patch_filenames:
>         from_marfile_entry = from_dir_hash[filename]
>         to_marfile_entry = to_dir_hash[filename]
>-        if filename in forced_list:
>-            print "Forcing "+ filename
>+        is_add_if_file = False
>+        for add_if_file in add_if_not_list:
>+            if filename.endswith(add_if_file):
>+                is_add_if_file = True
>+        if is_add_if_file:
>+            # This filename is in the add if not list, explicitly add-if-not
>+            create_add_if_not_patch_for_file(to_dir_hash[filename], patch_info)

You could express this as
          if os.path.basename(filename) in add_if_not_list:
and avoid the boolean and for loop.

>+        elif filename in forced_list:
>+            print "Forcing "+filename

Nit, missed the quoting around filename you've used elsewhere.

>-        create_add_patch_for_file(to_dir_hash[filename], patch_info)
>+        is_add_if_file = False
>+        for add_if_file in add_if_not_list:
>+            if filename.endswith(add_if_file):

For loop and boolean can be avoided by using basename again.

>diff --git a/tools/update-packaging/ b/tools/update-packaging/
>     def test_append_remove_instruction(self):
>         self.patch_info.append_remove_instruction('file.test')
>         self.assertEquals(['remove "file.test"'], self.patch_info.manifestv2)

For consistency, should check manifestv3 too (as you do other instructions). For another time, no tests on rmdir or rmrfdir ?
Attachment #8377492 - Flags: review?(nthomas) → review-
I said a few days there because I can't think of a way to make nightly/aurora users update to a specific build then onwards. Hence waiting until the majority have already updated beyond a patch landing.
I was thinking about how to handle the update-settings.ini file for older clients. How about leaving it as an add in the v2 manifest so clients that don't understand the v3 manifest still add it and clients that understand the v3 manifest only add-if-not?
That sounds like it would work great for nightly/aurora/release. For beta we'll also need a watershed after landing, because the 28.0 mar's will have the release version of update-settings.ini (pre-landing beta users would have trouble).
Do you think we can still get this patch this beta cycle ? Is there anything I can do to help ?
Attached patch patch rev2 (obsolete) — Splinter Review
Fingers crossed that we can.

Separating the file out so it is added to the version 2 manifest is a tad ugly but I think this is decent enough. I think I've addressed your other comments as well.
Attachment #8377492 - Attachment is obsolete: true
Attachment #8383593 - Flags: review?(nthomas)
Attachment #8383593 - Attachment description: bug900251 → patch rev2
Attached patch patch rev3 (obsolete) — Splinter Review
I think this is a better approach since the only time the add instruction needs to be present in the v2 manifest is when it is a complete update.
Attachment #8384246 - Flags: review?(nthomas)
Comment on attachment 8384246 [details] [diff] [review]
patch rev3

Review of attachment 8384246 [details] [diff] [review]:

This looks good after looking at the interdiff, and running a few tests. Minor nits to follow ....

::: tools/update-packaging/
@@ +43,5 @@
> +  # The third param will be an empty string when a file has an add-if-not
> +  # instruction in the version 3 manifest and needs an add instruction for the
> +  # version 2 manifest due to the precomplete file prior to the version 3
> +  # manifest having a remove instruction for the file before applying a complete
> +  # update.

Nit, more punctuation please, for clarity.

::: tools/update-packaging/
@@ +86,5 @@
> +  if check_for_add_if_not_update "$f"; then
> +    make_add_if_not_instruction "$f" "$updatemanifestv3"
> +    if check_for_add_to_manifestv2 "$f"; then
> +      make_add_instruction "$f" "$updatemanifestv2" "" 1
> +    fi

The three lines aren't in the tests/ copy. Looks like tests/ is checking, so it doesn't cause a failure to have the lines in or out. Lets keep the two copies of copy in sync for consistency though.

::: tools/update-packaging/
@@ +348,5 @@
>      patch_filenames.sort(reverse=True)
>      for filename in patch_filenames:
>          from_marfile_entry = from_dir_hash[filename]
>          to_marfile_entry = to_dir_hash[filename]
> +        is_add_if_file = False

Nit, boolean no longer used.
Attachment #8384246 - Flags: review?(nthomas) → review+
For landing on central - if you would like, we can freeze updates at a given build, get a new round of nightlies, and do some spot checks before unleashing on all users.
Attached patch patch rev4Splinter Review
Carrying forward r+

As soon as I get review on the tests in bug 759469 I'll land this. I'm finishing up a couple of things which is why the test patch hasn't been submitted. The tests use the same packaging for the test mars so it should be good to go but I'll take you up on new nightlies just to be safe.
Attachment #8383593 - Attachment is obsolete: true
Attachment #8384246 - Attachment is obsolete: true
Attachment #8384515 - Flags: review+
Comment on attachment 8384536 [details] [diff] [review]
2. new and renamed binaries

oops... wrong bug
Comment on attachment 8384536 [details] [diff] [review]
2. new and renamed binaries

Obsoleting in favor of the copy at attachment 8384537 [details] [diff] [review] on bug 759469.
Attachment #8384536 - Attachment is obsolete: true
Attachment #8384536 - Flags: review?(netzen)
Hey Nick, Bug 759469 is ready to land. Do you have a preferred landing time tomorrow?
Not for landing on central. Whenever you like I can lock the updates to the current build on the nightly channel, and leave nightlytest pointing to the latest.
Flags: needinfo?(nthomas)
Pushed to mozilla-central
Closed: 6 years ago
Resolution: --- → FIXED
Hey Nick, everything has landed on mozilla-central.

Can you freeze updates and generate new nightlies? Thanks!
Flags: needinfo?(nthomas)
Flags: needinfo?(nthomas)
Attached patch Followup patchSplinter Review
What is going on is that the from source doesn't contain the channel-prefs.js file so it falls through to an add instruction and then bails due to the check for a new channel-prefs.js file.
Attachment #8386479 - Flags: review?(nthomas)
Attachment #8386479 - Flags: review?(nthomas) → review+
I manually verified everything works as expected on Mac and Win when
* upgrading from a version without the new add-if-not instruction including the update-settings.ini file being removed by the precomplete operations and added by the update manifest instructions.

* upgrading with the new add-if-not instruction including the adding of the channel-prefs.js and update-settings.ini files when they don't exist.

I also visually inspected both the version 2 and version 3 update manifests and the instructions were correct.

So, all looks good!
Great! Updates on the nightly channel are now unfrozen.
Comment on attachment 8384515 [details] [diff] [review]
patch rev4

Also for followup patch

[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 #8384515 - Flags: approval-mozilla-aurora?
Attachment #8384515 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Everything looks good on Aurora as well.
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.