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

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

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

This way we can add back channel-prefs.js if it gets removed as happened in bug 756325.

Comment 1

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

Attachment: patch in progress

This passes the tests for the scripts... still have some cleanup to do.

Comment 2

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

Attachment: patch rev1

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.

Comment 3

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

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/common.sh b/tools/update-packaging/common.sh
>   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/make_incremental_update.sh b/tools/update-packaging/make_incremental_update.sh
+      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 make_incremental_updates.py.

>+    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/make_incremental_updates.py b/tools/update-packaging/make_incremental_updates.py
>     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/test_make_incremental_updates.py b/tools/update-packaging/test_make_incremental_updates.py
>     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 ?

Comment 4

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

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.

Comment 5

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

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?

Comment 6

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

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

Comment 7

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

Do you think we can still get this patch this beta cycle ? Is there anything I can do to help ?

Comment 8

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

Attachment: patch rev2

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.

Comment 9

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

Attachment: patch rev3

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.

Comment 10

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

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/common.sh
@@ +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/make_full_update.sh
@@ +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/make_full_update.sh copy. Looks like tests/ is checking make_incremental_updates.py, so it doesn't cause a failure to have the lines in or out. Lets keep the two copies of make_full_update.sh copy in sync for consistency though.

::: tools/update-packaging/make_incremental_updates.py
@@ +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.

Comment 11

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

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.

Comment 12

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

Attachment: patch rev4

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.

Comment 13

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

Attachment: 2. new and renamed binaries

Comment 14

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

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

oops... wrong bug

Comment 15

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

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.

Comment 16

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

Hey Nick, Bug 759469 is ready to land. Do you have a preferred landing time tomorrow?

Comment 17

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

^^

Comment 18

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

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.

Comment 19

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

Pushed to mozilla-central
https://hg.mozilla.org/mozilla-central/rev/3b71d879000c

Comment 20

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

Hey Nick, everything has landed on mozilla-central.
https://tbpl.mozilla.org/?rev=c1c3aa2b09d8

Can you freeze updates and generate new nightlies? Thanks!

Comment 21

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

Done!

Comment 22

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

Attachment: Followup patch

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.

Comment 23

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

Thanks!

Followup pushed to mozilla-central
https://hg.mozilla.org/mozilla-central/rev/21740ba59e60

Comment 24

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

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!

Comment 25

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

Great! Updates on the nightly channel are now unfrozen.

Comment 26

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

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

Comment 27

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

Pushed combined patches to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/fabe14145cd7

Comment 28

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

Everything looks good on Aurora as well.