Closed
Bug 900251
Opened 11 years ago
Closed 11 years ago
Add support for add-if-not instruction added by bug 759469 to the mar generation scripts
Categories
(Release Engineering :: General, defect)
Tracking
(firefox29 fixed, firefox30 fixed)
RESOLVED
FIXED
People
(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)
References
Details
Attachments
(2 files, 5 obsolete files)
(deleted),
patch
|
robert.strong.bugs
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nthomas
:
review+
|
Details | Diff | Splinter Review |
This way we can add back channel-prefs.js if it gets removed as happened in bug 756325.
Assignee | ||
Comment 1•11 years ago
|
||
This passes the tests for the scripts... still have some cleanup to do.
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
Assignee | ||
Comment 2•11 years ago
|
||
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 3•11 years ago
|
||
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 ?
Attachment #8377492 -
Flags: review?(nthomas) → review-
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
Do you think we can still get this patch this beta cycle ? Is there anything I can do to help ?
Assignee | ||
Comment 8•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8383593 -
Attachment description: bug900251 → patch rev2
Assignee | ||
Comment 9•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8383593 -
Flags: review?(nthomas)
Comment 10•11 years ago
|
||
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.
Attachment #8384246 -
Flags: review?(nthomas) → review+
Comment 11•11 years ago
|
||
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.
Assignee | ||
Comment 12•11 years ago
|
||
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+
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8384536 -
Flags: review?(netzen)
Assignee | ||
Comment 14•11 years ago
|
||
Comment on attachment 8384536 [details] [diff] [review]
2. new and renamed binaries
oops... wrong bug
Comment 15•11 years ago
|
||
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)
Assignee | ||
Comment 16•11 years ago
|
||
Hey Nick, Bug 759469 is ready to land. Do you have a preferred landing time tomorrow?
Comment 18•11 years ago
|
||
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)
Assignee | ||
Comment 19•11 years ago
|
||
Pushed to mozilla-central
https://hg.mozilla.org/mozilla-central/rev/3b71d879000c
Assignee | ||
Comment 20•11 years ago
|
||
Hey Nick, everything has landed on mozilla-central.
https://tbpl.mozilla.org/?rev=c1c3aa2b09d8
Can you freeze updates and generate new nightlies? Thanks!
Flags: needinfo?(nthomas)
Assignee | ||
Comment 22•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8386479 -
Flags: review?(nthomas) → review+
Assignee | ||
Comment 23•11 years ago
|
||
Thanks!
Followup pushed to mozilla-central
https://hg.mozilla.org/mozilla-central/rev/21740ba59e60
Assignee | ||
Comment 24•11 years ago
|
||
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•11 years ago
|
||
Great! Updates on the nightly channel are now unfrozen.
Assignee | ||
Comment 26•11 years ago
|
||
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?
Updated•11 years ago
|
status-firefox29:
--- → affected
Updated•11 years ago
|
Attachment #8384515 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 27•11 years ago
|
||
Pushed combined patches to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/fabe14145cd7
Assignee | ||
Comment 28•11 years ago
|
||
Everything looks good on Aurora as well.
Updated•7 years ago
|
Component: General Automation → General
You need to log in
before you can comment on or make changes to this bug.
Description
•