Closed Bug 1123990 Opened 5 years ago Closed 5 years ago

EME plugin-container voucher not in Nightly dir

Categories

(Release Engineering :: General, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(firefox37- verified, firefox38+ verified, firefox39 verified, firefox40 verified)

VERIFIED FIXED
Tracking Status
firefox37 - verified
firefox38 + verified
firefox39 --- verified
firefox40 --- verified

People

(Reporter: cpearce, Assigned: mshal)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

The EME plugin-container.exe voucher has disappeared from the Nightly install dir. It needs to be there. Can we get it back please?

It would also be very nice if we can write a unit test to ensure it doesn't disappear again. I don't know how to do that on my end, as any unit test I write won't work locally for me.
From IRC chris tells me: "the voucher appears in the Nightly zips I downloaded, so there must be some problem with packaging the voucher into the installer."  When I asked if it ever worked in installer: " I'm pretty sure it worked with the installer... I recall testing it."
Somehow I was mistaken; I tested the stand-alone Nightly installer all the way back to the landing of bug 1091668, and voucher.bin was never installed. I must have been looking in the nightly zip archives, not in the actual installed Nightly dir...
Mike, can you own this?
Flags: needinfo?(mshal)
[Tracking Requested - why for this release]: This corresponded to the landing of EME support, but with this bug will fail to install the necessary voucher on new installs (unsure about updates at this point)
Assignee: nobody → mshal
Ahh, this worked in the original patches in bug 1091668 (rev 25800dd79661), but there were complaints with how it was implemented, and the followup patch (rev 0db3ba4b70ab) ended up breaking the installer. Originally the voucher.bin was created in stage-package, which is used by both the installer and packager, but it was moved to MAKE_PACKAGE, which is apparently only used by the packager and not the installer. I'm trying to find the right place to put it...
Flags: needinfo?(mshal)
Does this patch make sense? It seems we are running stage-package for both 'make package' and 'make installer', and as far as I can tell it's unnecessary. This moves that knowledge from packager.mk to moz-automation.mk, so we can avoid running stage-package twice. The installer then picks up the stage-package dir that includes the voucher.bin file.

I also got rid of the pymake-specific code and removed the prepare-package target, which seem to be useless.

For comparison, here's a try build before and after:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a6ea5593cda
https://treeherder.mozilla.org/#/jobs?repo=try&revision=712691610a40

And a diff of the directories using the installer:

$ diff old.txt new.txt
77a78
> ./voucher.bin
Attachment #8552866 - Flags: review?(mh+mozilla)
Comment on attachment 8552866 [details] [diff] [review]
0001-Bug-1123990-installer-should-use-the-full-package.patch

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

::: toolkit/mozapps/installer/packager.mk
@@ +13,5 @@
>  ifndef PACKAGER_NO_LIBS
>  libs:: make-package
>  endif
>  
> +installer-stage:

The problem with this is that it will break people doing make installer without doing make package first...

@@ +23,5 @@
>  	@$(NSINSTALL) -D $(DEPTH)/installer-stage/core
>  	@cp -av $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/. $(DEPTH)/installer-stage/core
>  	@(cd $(DEPTH)/installer-stage/core && $(CREATE_PRECOMPLETE_CMD))
>  ifdef MOZ_SIGN_PREPARED_PACKAGE_CMD
> +	$(MOZ_SIGN_PREPARED_PACKAGE_CMD) $(DEPTH)/installer-stage

This may of may not be a problem. The comment mentioned pymake, but that makes a difference in mozmake too.
Attachment #8552866 - Flags: review?(mh+mozilla) → feedback-
While we're here, is there a way we can write a unit test to ensure the voucher remains packaged?
We're going to defer EME support to 38. As such, no need to track for 37.
(In reply to Chris Pearce (:cpearce) from comment #8)
> While we're here, is there a way we can write a unit test to ensure the
> voucher remains packaged?

Anthony: Would it be possible to add a moztrap case to check that a "voucher.bin" file appears in the install dir after installing the Firefox installer? We will break EME if the voucher.bin file does not end up there. Or perhaps you know a better way to verify the installer (I don't).
Flags: needinfo?(anthony.s.hughes)
(In reply to Chris Pearce (:cpearce) from comment #10)
> (In reply to Chris Pearce (:cpearce) from comment #8)
> > While we're here, is there a way we can write a unit test to ensure the
> > voucher remains packaged?
> 
> Anthony: Would it be possible to add a moztrap case to check that a
> "voucher.bin" file appears in the install dir after installing the Firefox
> installer? We will break EME if the voucher.bin file does not end up there.
> Or perhaps you know a better way to verify the installer (I don't).

I'm not directly involved with QAing releases anymore. Robert, can you please review this request?
Flags: needinfo?(anthony.s.hughes) → needinfo?(kairo)
I don't look at MozTrap at all, I completely rely on Florin and the team in Romania to run those tests and report any issues to me.

Florin, can you look into comment #10 here?
Flags: needinfo?(kairo) → needinfo?(florin.mezei)
We can definitely add a test in MozTrap (it's noted and will add the test when time allows), however we do not always run all MozTrap tests. So if we want to make sure this does not disappear at some point again, then we would need to cover this through Automation.
Flags: needinfo?(florin.mezei)
I'm fairly certain there is a better way to do this, but I could use some help figuring out what that is. The prior patch also didn't add "voucher.bin" to the precomplete file, which means when you use the uninstall helper, voucher.bin stays in the install directory. However, I don't know the best place to generate the voucher such that we are generating it on the signed plugin-container.exe, but also generate precomplete after voucher.bin. It seems to me we need to do:

1) run packager.py before we sign anything
2) sign the package (in particular, plugin-container.exe) before voucher.bin generation
3) generate voucher.bin before generating precomplete
4) generate precomplete before zipping up the package or creating the installer.

For 'make package', the precomplete file is just generated by packager.py, but for the installer it is generated by packager.py, and then overwritten later by an explicit call to CREATE_PRECOMPLETE_CMD. So far the only way I've found to meet the above constraints is to move the installer's CREATE_PRECOMPLETE_CMD to the end of installer-stage, and then pull precomplete logic out of packager.py and call it directly from MAKE_PACKAGE.

I hope there's a simpler solution that I'm not seeing.

Also, I've given up on trying to share a single stage-package call between the installer & packager for now, though it seems to be redundant.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=538a68b24dfb
Attachment #8552866 - Attachment is obsolete: true
Attachment #8556796 - Flags: review?(mh+mozilla)
Comment on attachment 8556796 [details] [diff] [review]
0001-Bug-1123990-installer-needs-voucher.bin.patch

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

That seems reasonable. Ideally, I think the whole thing should just be integrated in the packager code, whereby the packager would be doing the package, the installer, and the signing. But that's a lot of work, and something to keep for the future.
Attachment #8556796 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8556796 [details] [diff] [review]
0001-Bug-1123990-installer-needs-voucher.bin.patch

cpearce, can you double-check the builds from https://treeherder.mozilla.org/#/jobs?repo=try&revision=538a68b24dfb to make sure I'm not missing anything?
Attachment #8556796 - Flags: review?(cpearce)
(In reply to Mike Hommey [:glandium] from comment #15)
> That seems reasonable. Ideally, I think the whole thing should just be
> integrated in the packager code, whereby the packager would be doing the
> package, the installer, and the signing. But that's a lot of work, and
> something to keep for the future.

That sounds like a good idea - filed bug 1127840. Feel free to add any details I may have missed.
Comment on attachment 8556796 [details] [diff] [review]
0001-Bug-1123990-installer-needs-voucher.bin.patch

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

I see the voucher in both the zip archive, and in the install dir after I run the installer. Looks good.
Attachment #8556796 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/2742651b0cd9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
sorry had to back this out from m-c since this might have busted the nightly builds like https://treeherder.mozilla.org/logviewer.html#?job_id=982094&repo=mozilla-central and also caused Bug 1128826
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
So I'm pretty sure this is because I somehow managed to call CREATE_PRECOMPLETE_CMD *after* INNER_MAKE_PACKAGE, in one of the 3 cases where MAKE_PACKAGE is defined. So 2/3 of the cases were correct, but the else case (anything without a signed package) wasn't. I'm trying to find a way to verify this.
I'm pretty sure this is the right fix, though I'm still trying to figure out how to actually verify it. interdiff:

< +MAKE_PACKAGE    = $(INNER_MAKE_PACKAGE) && (cd $(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH) && $(CREATE_PRECOMPLETE_CMD))
---
> +MAKE_PACKAGE    = (cd $(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH) && $(CREATE_PRECOMPLETE_CMD)) && $(INNER_MAKE_PACKAGE)
Attachment #8556796 - Attachment is obsolete: true
Attachment #8558768 - Flags: review?(mh+mozilla)
FWIW I verified this somewhat by building on my dev-master and running './build.sh gecko-update-full' manually. I don't think it's possible in our infra to actually test a nightly build though, so I'm not completely confident that it doesn't break something else.
Attachment #8558768 - Flags: review?(mh+mozilla) → review+
Hopefully it sticks this time...

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f543e83304e9
https://hg.mozilla.org/mozilla-central/rev/f543e83304e9
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Backed out in https://hg.mozilla.org/mozilla-central/rev/3094601af679. You'll need to find another way to land this, because this way isn't working.

Apparently someone retriggered nightlies on the push that merged you from mozilla-inbound, for some reason, and then killed most of them but left the desktop ones running. Then you busted Mac, the same way as before, https://treeherder.mozilla.org/logviewer.html#?job_id=1023103&repo=mozilla-central, and I presume it was also you breaking Windows, https://treeherder.mozilla.org/logviewer.html#?job_id=1023169&repo=mozilla-central, though I don't actually see the error anywhere in the log, and then because if a rattlesnake was biting us on the neck we would just click retrigger, we just merrily retriggered them. That didn't turn out to improve things.

I think your best bet is going to be to arrange with a sheriff who is just starting out his day to land directly on mozilla-central, and then trigger nightlies on that push, so that the sheriff will still know that they are your nightlies, and that bustage in them is bustage, when they finish busted again.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1132503
Mike, what's the status here? This is blocking Adobe shipping us a test release, we need it fixed ASAP.
Flags: needinfo?(mshal)
I believe the OSX failure is because the path where "precomplete" needs to go changed from Contents/MacOS to Contents/Resources in bug 1047584. However, when I reintroduced the old precomplete logic, it was still using $(_BINPATH), which points to Contents/MacOS. So the precomplete file ends up in the wrong directory, and make-full-update.sh can no longer find it.

I think the correct fix here is to just introduce a new variable, say $(_RESPATH), and set it to Contents/Resources. As far as I can tell, _BINPATH was only set for OSX anyway, but since it is used elsewhere we can't just change the value of _BINPATH directly.

For Windows I think the problem was when I added the 'automation/installer: automation/package' dependency, the installer was now being generated later in the build process, in particular after automation/update-packaging runs. It turns out that update-packaging needs the installer exe, though for some reason I only included this dependency for the "pretty" versions of the steps. So we should just need a dependency for 'automation/update-packaging: automation/installer'.

I am trying to test these changes on a project branch that is configured for nightlies. I expect this will take the bulk of the time.
Flags: needinfo?(mshal)
I still need to finish testing this on ash, but I figure I'll ask for review now in case things happen to go well: https://tbpl.mozilla.org/?tree=Ash&rev=4b72126c9637

The red OSX N's are for l10n, which I think has to do with the l10n repack work that mgerva is doing on ash as well, but I need to double-check with him. As of now, Windows nightlies haven't finished, so they obviously need to pass and have a working voucher.bin

This patch includes $(_RESPATH) to point to the correct location for the precomplete file on OSX, as well as the additional dependency for Windows.
Attachment #8558768 - Attachment is obsolete: true
Attachment #8565820 - Flags: review?(mh+mozilla)
And I forgot to mention, we need to reconfigure ash to do B2G nightlies and test them as well.
Attachment #8565820 - Flags: review?(mh+mozilla) → review+
ash has been configured to do B2G nightlies, and they are running now. We can ignore the l10n nightly bustage on ash, since it is configured to do mozharness l10n repacks, while mozilla-central isn't.
Well, as far as I can tell, it looks like the nightlies ran successfully on ash. I think now we can try to land on m-c directly per #c27.
Landed on m-c:

https://hg.mozilla.org/mozilla-central/rev/986e840a2979

Fingers crossed...
I'm guessing this didn't get backed out?
Correct :)
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 1137060
We'll need uplift to Firefox 37.
Flags: needinfo?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #37)
> We'll need uplift to Firefox 37.

But not until bug 1138535 is verified as fixed.
Flags: needinfo?(cpearce)
This can be closed now as the voucher.bin file is in the installation folder for Firefox 37 Release, 38 Beta, 39 Aurora and 40 Nightly.
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.