Closed Bug 1255245 Opened 4 years ago Closed 4 years ago

ESR45 generates mar files with the wrong channel ID

Categories

(Firefox Build System :: General, defect)

45 Branch
defect
Not set

Tracking

(firefox45 unaffected, firefox-esr38 unaffected, firefox-esr4545+ fixed)

RESOLVED FIXED
Tracking Status
firefox45 --- unaffected
firefox-esr38 --- unaffected
firefox-esr45 45+ fixed

People

(Reporter: nthomas, Assigned: nthomas)

References

Details

Attachments

(3 files)

[Tracking Requested - why for this release]:

It comes from http://hg.mozilla.org/releases/mozilla-esr45/file/default/browser/confvars.sh#l52

ACCEPTED_MAR_CHANNEL_IDS=firefox-mozilla-release
# The MAR_CHANNEL_ID must not contain the following 3 characters: ",\t "
MAR_CHANNEL_ID=firefox-mozilla-release

We shipped 45.0 like this, which is going to impact updates to 45.0.1/45.1.0.
(In reply to Nick Thomas [:nthomas] from comment #0)
> We shipped 45.0 like this, which is going to impact updates to 45.0.1/45.1.0.

I misspoke - we shipped 45.0esr like this, which is going to impact updates to 45.0.1esr/45.1.0esr.
Should we make an announcement on the ESR list advising people not to install ESR45 and follow up with 45.0.1 to fix the issue?
That would make sense for anyone using the builtin updater, which will only accept release mar files. It would also simplify life for RelEng across the all the ESR branches if we could ignore 45.0esr and not offer any updates for it.

Not sure how this happened. The merge docs say to use the migration script
 https://wiki.mozilla.org/ReleaseEngineering/How_To/Create_new_ESR_branch#Pull_from_mozilla-release
and that bumps browser/confvars.sh
 http://hg.mozilla.org/mozilla-central/file/af7c0cb0798f/testing/mozharness/configs/merge_day/release_to_esr.py

Patch coming up.
Assignee: nobody → nthomas
Status: NEW → ASSIGNED
Attachment #8728755 - Flags: review?(robert.strong.bugs)
Attachment #8728755 - Flags: review?(robert.strong.bugs) → review+
Comment on attachment 8728755 [details] [diff] [review]
[gecko] Fix channel IDs

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Won't be able to update ESR38 and older to ESR45
Fix Landed on Version: Only applicable here
Risk to taking this patch (and alternatives if risky): Very small, same values as ESR38
String or UUID changes made by this patch:  None
Attachment #8728755 - Flags: approval-mozilla-esr45?
Duplicate of this bug: 1255790
(In reply to Nick Thomas [:nthomas] from comment #4)
> Created attachment 8728755 [details] [diff] [review]
> [gecko] Fix channel IDs

I think we might need a two stage update for this...if we build the next ESR with a mar channel id of "firefox-mozilla-esr", 45.0 won't be able to apply it. I think we need to change ACCEPTED_MAR_CHANNEL_IDS, then MAR_CHANNEL_ID, and watershed through the ACCEPTED_MAR_CHANNEL_IDS change.
(In reply to Ben Hearsum (:bhearsum) from comment #7)
> (In reply to Nick Thomas [:nthomas] from comment #4)
> > Created attachment 8728755 [details] [diff] [review]
> > [gecko] Fix channel IDs
> 
> I think we might need a two stage update for this...if we build the next ESR
> with a mar channel id of "firefox-mozilla-esr", 45.0 won't be able to apply
> it. I think we need to change ACCEPTED_MAR_CHANNEL_IDS, then MAR_CHANNEL_ID,
> and watershed through the ACCEPTED_MAR_CHANNEL_IDS change.

Sorry, just reread comment #3 and I see that you're assuming we don't care about 45.0esr -> 45.1.0/45.0.1 updates. Works for me!
The main reason for this is the fact that releng didn't run our merge scripts on this repo. We prepared the repo/automation, but we missed the most important piece, maybe because there was no formal "please uplift/merge" go?
> it would also simplify life for RelEng across the all the ESR branches if we could ignore 45.0esr 
> and not offer any updates for it.
Can we avoid that?
Flags: needinfo?(sledru)
Attachment #8728755 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Sylvestre, we have a few options I think. First of all, this applies just to those users actually using our updater, rather than organisation-wide deploy methods. I can't find any ADI from blocklist or update pings for 45.0esr, so it's hard to say how many people are affected. We are defaulting to ESR45 on the d/l page though, so getting something out the door soon would help minimize the problem.

Options:
1, Abandon 45.0esr and never offer it any updates
How:  We land attachment 8728755 [details] [diff] [review] for 45.0.1, and that's the first supported release.
Pros: Fastest possible fix, simpler to offer updates to previous and future versions (ie releng isn't carrying a complexity cost for a long time)
Cons: Some unknown number of users left in the lurch. Bad optics.

2, Land the full fix now, and do some manual work to give 45.0.esr users an update
How:  Land attachment 8728755 [details] [diff] [review] in full, so that 45.0.1esr generates and accepts mar files for esr only. Block updates to 45.0esr users initially. Repackage the 45.0.1 mar files so that they are tagged with the channel id 'firefox-mozilla-release' and can be applied to 45.0esr. Very carefully ensure these are only offered to 45.0esr users on the esr channel, because they can also be applied to 45.0 release.
Pros: All users get updated. Only one esr release in the wild which accepts release channel updates. 
Cons: We'd have to write a script to repackage & sign the updates, possibly slower if we need to ship a sec-fix. Risk of offering release builds with version <= 45.0 the 45.0.1esr update if Balrog misconfigured.

3, Ben's proposal in comment #7 - split attachment 8728755 [details] [diff] [review] in two and land in separate releases
How:  Land the ACCEPTED_MAR_CHANNEL_IDS change in 45.0.1esr, and the MAR_CHANNEL_ID part in 45.0.2esr or 45.1.0esr. Users which already have 45.0esr would be offered 45.0.1esr first (aka watershed). Anyone on ESR38 or older would skip right over the problem to the 2nd release.
Pros: All users get updated. Mostly automated, just need to handle the watershed at 45.0.1esr
Cons: Two releases out there which could accept updates from the release channel

What do you prefer ? I'll hold off landing the patch for now.
Flags: needinfo?(sledru)
I don't think we should go with #1.
I don't know how to choice between #2 & #3.
Whatever is the easier & safest for you.

Could you land a fix today? Thanks. I would like to start the build Tuesday
Flags: needinfo?(sledru)
I realized that I mis-thought about option #3 in comment #11 - we'd only have 45.0esr out there that accepts release mar files, and only generate mar files like that for 45.0.1esr. So a watershed is the only 'cost to bear' from the split patch approach, which is marginal and avoids a lot of manual work compared to #2.

--> Landed the ACCEPTED_MAR_CHANNEL_IDS change in https://hg.mozilla.org/releases/mozilla-esr45/rev/3fdd789fe744


TODO for 45.0.1esr:
* verify mar files are still firefox-mozilla-release
* verify updates from 45.0 apply

TODO after 45.0.1esr:
* land second half of patch
* set up watershed rule in Balrog
* adjust http://hg.mozilla.org/build/tools/file/default/release/patcher-configs/mozEsr45-branch-patcher2.cfg for watershed
Verified with 45.0.1esr build1:
* update-settings.ini in the installer (win32 en-US) contains:
    ACCEPTED_MAR_CHANNEL_IDS=firefox-mozilla-esr
* mar files are have this 'mar -T' output:
    Signature block found with 1 signature
    1 additional block found:
      - Product Information Block:
        - MAR channel name: firefox-mozilla-release
        - Product version: 45.0.1

While the update applies OK, there's still a problem in the update verify logs. Updating from 45.0esr yields this diff:
  diff -r source/bin/update-settings.ini target/bin/update-settings.ini
  5c5
  < ACCEPTED_MAR_CHANNEL_IDS=firefox-mozilla-release
  ---
  > ACCEPTED_MAR_CHANNEL_IDS=firefox-mozilla-esr
  WARN: non-binary files found in diff

ie update-settings.ini hasn't been changed by the update. This is because the updatev3.manifest says
  add-if-not "update-settings.ini" "update-settings.ini"
rather than just 'add'.
This would land on the GECKO4501esr_2016031518_RELBRANCH relbranch only, and we'd do a build2.

I've created test complete and partial updates (on mac), and the diff against the build1 updates look like this:

diff -rU2 build1-complete/updatev3.manifest test-complete/updatev3.manifest
--- complete/updatev3.manifest	2016-03-16 17:07:34.000000000 +1300
+++ test-complete/updatev3.manifest	2016-03-16 17:49:47.000000000 +1300
@@ -9,5 +9,5 @@
 add "updater.ini"
 add "updater.exe"
-add-if-not "update-settings.ini" "update-settings.ini"
+add "update-settings.ini"
 add "uninstall/helper.exe"
 add "softokn3.dll"

Only in build1-partial/: update-settings.ini
Only in test-partial: update-settings.ini.patch
diff -rU2 partial/updatev2.manifest test-partial/updatev2.manifest
--- build1-partial/updatev2.manifest	2016-03-16 17:06:05.000000000 +1300
+++ test-partial/updatev2.manifest	2016-03-16 17:36:34.000000000 +1300
@@ -7,4 +7,5 @@
 patch "voucher.bin.patch" "voucher.bin"
 patch "updater.exe.patch" "updater.exe"
+patch "update-settings.ini.patch" "update-settings.ini"
 patch "uninstall/helper.exe.patch" "uninstall/helper.exe"
 patch "softokn3.dll.patch" "softokn3.dll"
diff -rU2 partial/updatev3.manifest test-partial/updatev3.manifest
--- build1-partial/updatev3.manifest	2016-03-16 17:06:05.000000000 +1300
+++ test-partial/updatev3.manifest	2016-03-16 17:36:34.000000000 +1300
@@ -7,5 +7,5 @@
 patch "voucher.bin.patch" "voucher.bin"
 patch "updater.exe.patch" "updater.exe"
-add-if-not "update-settings.ini" "update-settings.ini"
+patch "update-settings.ini.patch" "update-settings.ini"
 patch "uninstall/helper.exe.patch" "uninstall/helper.exe"
 patch "softokn3.dll.patch" "softokn3.dll"
Attachment #8731048 - Flags: review?(bhearsum)
Comment on attachment 8731048 [details] [diff] [review]
[gecko] Always modify update-settings.ini

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Firefox will need updates tagged for release instead of esr
Fix Landed on Version: N/A, this would only land on the relbranch for 45.0.1esr
Risk to taking this patch (and alternatives if risky): We could repack build1 updates, but this is less risky
String or UUID changes made by this patch: None
Attachment #8731048 - Flags: approval-mozilla-esr45?
Comment on attachment 8731048 [details] [diff] [review]
[gecko] Always modify update-settings.ini

Review of attachment 8731048 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. I can help get this landed if necessary.
Attachment #8731048 - Flags: review?(bhearsum) → review+
Comment on attachment 8731048 [details] [diff] [review]
[gecko] Always modify update-settings.ini

We need this too
Attachment #8731048 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Comment on attachment 8731048 [details] [diff] [review]
[gecko] Always modify update-settings.ini

Landed on the relbranch: https://hg.mozilla.org/releases/mozilla-esr45/rev/cab241354c10
Attachment #8731048 - Flags: checkin+
Verified that 45.0esr --> 45.0.1esr updates are applied cleanly, and set update-settings to the right content. So 45.0.1esr build2 looks good to release.

TODO after 45.0.1esr ships:
* land second half of patch
* set up watershed rule in Balrog
* adjust http://hg.mozilla.org/build/tools/file/default/release/patcher-configs/mozEsr45-branch-patcher2.cfg for watershed
Keywords: leave-open
Added rule 323, which maps requests from Firefox on channel=esr*, with version=45.0 and buildID=20160304113541 to Firefox-45.0.1esr-build2.

This patch means we'll get to http://hg.mozilla.org/build/tools/file/default/release/patcher-config-bump.pl#l202 and not execute the block, avoiding adding a past-update line for 45.0 (which in turn will result in update failures on the next release). I've left the <45.0esr> block in releases because I use these files as references for buildIDs.
Attachment #8732691 - Flags: review?(bhearsum)
Comment on attachment 8732691 [details] [diff] [review]
[tools] setup patcher config for watershed

Review of attachment 8732691 [details] [diff] [review]:
-----------------------------------------------------------------

Huh, I didn't know this was supported. Great!
Attachment #8732691 - Flags: review?(bhearsum) → review+
Comment on attachment 8732691 [details] [diff] [review]
[tools] setup patcher config for watershed

https://hg.mozilla.org/build/tools/rev/a5246c464c99
Attachment #8732691 - Flags: checkin+
(In reply to Nick Thomas [:nthomas] from comment #20)
> TODO after 45.0.1esr ships:
> * land second half of patch
> * set up watershed rule in Balrog
> * adjust
> http://hg.mozilla.org/build/tools/file/default/release/patcher-configs/
> mozEsr45-branch-patcher2.cfg for watershed

All done, closing.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
(In reply to Nick Thomas [:nthomas] from comment #21)
> Second part of patch landing:
> http://hg.mozilla.org/releases/mozilla-esr45/rev/0937306e704a

Transplanted to GECKO4501esr_2016031618_RELBRANCH for 45.0.2esr at
https://hg.mozilla.org/releases/mozilla-esr45/rev/6bdef7c9bc62
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Component: Build Config → General
Product: Firefox → Firefox Build System
You need to log in before you can comment on or make changes to this bug.