1245745 - Include libpulse version in update URL

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Karl Tomlinson (:karlt)]

There is a plan to make libpulse.so.0 a runtime dependency, so that
distribution packaging will automatically notice the dependency and alsa
support can be deprecated.

To provide smooth update messaging for those without libpulse installed, we
need to indicate from the client whether this library is installed.

Comment 1

[Karl Tomlinson (:karlt)]

Currently, the osVersion part of the update URL has the format
"Linux 3.19.0-33-generic (GTK 2.24.23)".

A logical extension would be to give this the format
"Linux 3.19.0-33-generic (GTK 2.24.23 libpulse 7.1.0)"

[1] indicates that update rules use partial matches on this string, so
extending the string with a unique substring seems suitable, but I'm not able
to see the existing rules [2] to confirm.

We don't yet have a need for the version so much as the knowledge that the
library is installed, but the version may come in useful and it is easy to
include.

This can be implemented by merely extending the secondaryLibrary property.

[1] https://wiki.mozilla.org/Balrog#What.27s_in_a_rule.3F 
[2] https://wiki.mozilla.org/Balrog#Admin_UI_Use_Cases

Comment 2

[Mike Hommey [:glandium]]

May want to put a comma in there.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: include libpulse version in update URL

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8715645 [details] [diff] [review]
include libpulse version in update URL

With app.update.log set, I see in the error console:

AUS:SVC Checker:getUpdateURL - update URL: https://aus5.mozilla.org/update/3/Firefox/47.0a1/20160130214854/Linux_x86_64-gcc3/en-US/default/Linux%203.12.53-gentoo%20(GTK%203.16.7%2Clibpulse%207.1.0)/default/default/update.xml

Are you OK with that format, Robert?

Comment 5

[Karl Tomlinson (:karlt)]

[Tracking Requested - why for this release]:
Updates will have to go through one version for bug 1227023 changes before the GTK3 dependency change.  Would be good to get this in before the GTK3 change so that users don't need to update through another version too.

Comment 6

[[e-mail] (:bhearsum)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #1)
> Currently, the osVersion part of the update URL has the format
> "Linux 3.19.0-33-generic (GTK 2.24.23)".
> 
> A logical extension would be to give this the format
> "Linux 3.19.0-33-generic (GTK 2.24.23 libpulse 7.1.0)"
> 
> [1] indicates that update rules use partial matches on this string, so
> extending the string with a unique substring seems suitable, but I'm not able
> to see the existing rules [2] to confirm.

I think this will be OK. The only scenario I can think of where it would be an issue is if we're trying to match a GTK version and a Pulse version in the same rule...it would be very difficult to construct a single string that matches both sets. This doesn't seem like a terribly likely scenario, and we could probably work around it with extra rules anyways.

> We don't yet have a need for the version so much as the knowledge that the
> library is installed, but the version may come in useful and it is easy to
> include.

It's *awesome* to do this ahead of time to avoid the need for a special update to merely alter the update URL later. Thanks!

Comment 7

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

Ben, I'm wondering how we would structure the rules around deprecation and ongoing updates. When libpulse is not installed we could have libpulse-not-available (or similar) in the query, making the deprecation rule easy. That should also avoid needing a linux-only rule which whitelists 'libpulse' and points to the latest release, plus the existing rule for the other platforms. Some defensive coding might be necessary to distinguish between the 'genuinely no libpulse' and 'problem looking for libpulse' cases.

What do you think ?

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to Nick Thomas [:nthomas] from comment #7)
> Ben, I'm wondering how we would structure the rules around deprecation and
> ongoing updates. When libpulse is not installed we could have
> libpulse-not-available (or similar) in the query, making the deprecation
> rule easy. That should also avoid needing a linux-only rule which whitelists
> 'libpulse' and points to the latest release, plus the existing rule for the
> other platforms. Some defensive coding might be necessary to distinguish
> between the 'genuinely no libpulse' and 'problem looking for libpulse' cases.

I don't think I can distinguish between 'genuinely no libpulse' and 'problem looking for libpulse', but I think they would be treated the same.

It sounds like you are implying that a "version == 45 and does not contain libpulse" rule is not possible?

I can include 'libpulse none' or whatever if that helps.

Please ni? me when the appropriate direction is clear, and I can update the patch.

Comment 9

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

(In reply to Karl Tomlinson (ni?:karlt) from comment #8)
> It sounds like you are implying that a "version == 45 and does not contain
> libpulse" rule is not possible?

Not directly - the code can look for matches but not absences. Lets see what Ben has to say.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8715645 [details] [diff] [review]
include libpulse version in update URL

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

::: xpcom/base/nsSystemInfo.cpp
@@ +714,5 @@
> +      (dlsym(libpulse, "pa_get_library_version"));
> +
> +    if (pa_get_library_version) {
> +      secondaryLibrary.AppendLiteral(",libpulse ");
> +      secondaryLibrary.Append(pa_get_library_version());

You also could do AppendPrintf(",libpulse %s", pa_get_library_version())

Comment 11

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #7)
> Ben, I'm wondering how we would structure the rules around deprecation and
> ongoing updates. When libpulse is not installed we could have
> libpulse-not-available (or similar) in the query, making the deprecation
> rule easy. That should also avoid needing a linux-only rule which whitelists
> 'libpulse' and points to the latest release, plus the existing rule for the
> other platforms. Some defensive coding might be necessary to distinguish
> between the 'genuinely no libpulse' and 'problem looking for libpulse' cases.

I think I might be a confused...so let me restate things to make sure I understand correctly:
pulseaudio is going to be a required runtime dependency. We're adding it to the update URL so that we can avoid updating people who do not have it installed or have an incompatible version (thus breaking them). This is very similar to what we already do with GTK, except that because some users may not have it *at all*, we need some special logic to indicate that (unlike the GTK case, where we're just passing along version information).

If that's all correct, I think a special string in the update query (such as "libpulse-not-available) makes a lot of sense. I don't much care what it is as long as it's unique enough to do a substring match without using regexes. Karl, I'm NI'ing you as requested to update the patch to this effect.

(In reply to Nick Thomas [:nthomas] from comment #9)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #8)
> > It sounds like you are implying that a "version == 45 and does not contain
> > libpulse" rule is not possible?
> 
> Not directly - the code can look for matches but not absences. Lets see what
> Ben has to say.

Yeah, exactly. This really boils down to the fact that our update query is mostly just the URI of a GET request, and each part of the URL needs to have *something* in it. So testing for absence becomes "is this field the default value". In some glorious future where we're redesigning the update request this might be something to consider improving.

Comment 12

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

Comment on attachment 8715645 [details] [diff] [review]
include libpulse version in update URL

LGTM with bhearsum's modifications

Comment 13

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

I'm concerned with adding a runtime dependency for audio, which is less than a 100% use case for the Web. (I can reasonably check email, read news, etc without audio.) Do we need to have pulseaudio as a runtime dependency? Can we gracefully degrade on the client and inform the user that Firefox requires pulseaudio for sound rather than have this as a hard runtime requirement?

Comment 14

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

Is there a bug filed for requiring Pulse Audio? If not, could someone file one? Thanks!

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: include libpulse version in update URL

This version adds the ",libpulse not-available" string when not available
(and keeps the version when available), so we have either of the following
kinds of responses.

https://aus5.mozilla.org/update/3/Firefox/47.0a1/20160205121449/Linux_x86_64-gcc3/en-US/default/Linux%203.12.53-gentoo%20(GTK%203.16.7%2Clibpulse%20not-available)/default/default/update.xml
https://aus5.mozilla.org/update/3/Firefox/47.0a1/20160205121449/Linux_x86_64-gcc3/en-US/default/Linux%203.12.53-gentoo%20(GTK%203.16.7%2Clibpulse%207.1.0)/default/default/update.xml

The consistency of the space after "libpulse" makes the code a little simpler
than changing to a hyphen when not available.

Comment 16

[Karl Tomlinson (:karlt)]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #13)
The questions you raise are worth discussing, but this bug just tracks update url support to make the new dependency possible in the future, while rushing to get this into the same update node as the one for GTK3 info.  If the conclusion turns out to be that a hard dependency is not what we want, then we can do something different.

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8717314 [details] [diff] [review]
include libpulse version in update URL

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

::: xpcom/base/nsSystemInfo.cpp
@@ +708,5 @@
> +    secondaryLibrary.Append(nsDependentCSubstring(gtkver, gtkver_len));
> +  }
> +
> +  void* libpulse = dlopen("libpulse.so.0", RTLD_LAZY);
> +  const char* libpulseVersion = "not-available";

"missing" would be shorter. That said, I'm not sure what adding a string when it's missing is doing. We already know what version we're being upgraded from, so no version of libpulse + upgrading from >= 46 should be enough?

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8717314 [details] [diff] [review]
include libpulse version in update URL

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

Comment 11 has the answer to my question.

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #14)
> Is there a bug filed for requiring Pulse Audio? If not, could someone file
> one? Thanks!

I created bug 1247056.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbd61f93a164

Comment 21

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #13)
> I'm concerned with adding a runtime dependency for audio, which is less than
> a 100% use case for the Web. (I can reasonably check email, read news, etc
> without audio.) Do we need to have pulseaudio as a runtime dependency? Can
> we gracefully degrade on the client and inform the user that Firefox
> requires pulseaudio for sound rather than have this as a hard runtime
> requirement?

The dependency will be on having an audio subsystem installed. That is independent of using an audio device. An audio device is not required unless you want to play audio. The library itself will be required. Linux distros aren't good at managing package dependencies at runtime.

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 8717314 [details] [diff] [review]
include libpulse version in update URL

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
This is about getting the pulse audio info into the version prior to the version when GTK3 is shipped so the we don't need to force users with old versions to downloading another additional version when they update.
[Describe test coverage new/current, TreeHerder]:
Tested locally with and without libpulse.
[Risks and why]:
The cost of errors in changes involving updates is higher, but the patch is simple and not too different from existing code.
[String/UUID change made/needed]:

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8717314 [details] [diff] [review]
include libpulse version in update URL

OK, let's try that.
Should be in 45 beta 6.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a3573b42275e

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1ce5ee8b5c55