1123990 - EME plugin-container voucher not in Nightly dir

Status: VERIFIED FIXED

(Release Engineering :: General, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 1

[Justin Wood (:Callek)]

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

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 3

[Justin Wood (:Callek)]

Mike, can you own this?

Comment 4

[Justin Wood (:Callek)]

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

Comment 5

[Michael Shal [: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...

Comment 6

[Michael Shal [:mshal]]

Attachment: 0001-Bug-1123990-installer-should-use-the-full-package.patch

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

Comment 7

[Mike Hommey [:glandium]]

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.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

While we're here, is there a way we can write a unit test to ensure the voucher remains packaged?

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

We're going to defer EME support to 38. As such, no need to track for 37.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 11

[u279076]

(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?

Comment 12

[Robert Kaiser]

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?

Comment 13

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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.

Comment 14

[Michael Shal [:mshal]]

Attachment: 0001-Bug-1123990-installer-needs-voucher.bin.patch

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

Comment 15

[Mike Hommey [:glandium]]

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.

Comment 16

[Michael Shal [:mshal]]

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?

Comment 17

[Michael Shal [:mshal]]

(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 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 19

[Michael Shal [:mshal]]

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/2742651b0cd9

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2742651b0cd9

Comment 21

[Carsten Book [:Tomcat]]

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

Comment 22

[Michael Shal [:mshal]]

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.

Comment 23

[Michael Shal [:mshal]]

Attachment: 0001-Bug-1123990-installer-needs-voucher.bin-r-glandium-r.patch

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)

Comment 24

[Michael Shal [:mshal]]

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.

Comment 25

[Michael Shal [:mshal]]

Hopefully it sticks this time...

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f543e83304e9

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f543e83304e9

Comment 27

[Phil Ringnalda (:philor)]

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.

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

Mike, what's the status here? This is blocking Adobe shipping us a test release, we need it fixed ASAP.

Comment 29

[Michael Shal [: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.

Comment 30

[Michael Shal [:mshal]]

Attachment: bug-1123990.patch

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.

Comment 31

[Michael Shal [:mshal]]

And I forgot to mention, we need to reconfigure ash to do B2G nightlies and test them as well.

Comment 32

[Michael Shal [:mshal]]

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.

Comment 33

[Michael Shal [:mshal]]

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.

Comment 34

[Michael Shal [:mshal]]

Landed on m-c:

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

Fingers crossed...

Comment 35

[Chris Pearce [:cpearce (Not reading bugmail)]]

I'm guessing this didn't get backed out?

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Correct :)

Comment 37

[Chris Pearce [:cpearce (Not reading bugmail)]]

We'll need uplift to Firefox 37.

Comment 38

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 39

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b377c6a7858e

Comment 40

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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.