Closed Bug 1245745 Opened 8 years ago Closed 8 years ago

Include libpulse version in update URL

Categories

(Toolkit :: Application Update, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox45 + fixed
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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
May want to put a comma in there.
Attachment #8715645 - Flags: review?(mh+mozilla)
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?
Attachment #8715645 - Flags: feedback?(robert.strong.bugs)
[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.
Depends on: 1239516
(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!
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 ?
(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.
(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.
Flags: needinfo?(bhearsum)
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())
Attachment #8715645 - Flags: review?(mh+mozilla) → review+
(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.
Flags: needinfo?(bhearsum) → needinfo?(karlt)
Comment on attachment 8715645 [details] [diff] [review]
include libpulse version in update URL

LGTM with bhearsum's modifications
Attachment #8715645 - Flags: feedback?(robert.strong.bugs) → feedback+
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?
Is there a bug filed for requiring Pulse Audio? If not, could someone file one? Thanks!
Flags: needinfo?(ajones)
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.
Attachment #8717314 - Flags: review?(mh+mozilla)
(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.
Flags: needinfo?(karlt)
Attachment #8715645 - Attachment is obsolete: true
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?
Attachment #8717314 - Flags: review?(mh+mozilla)
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.
Attachment #8717314 - Flags: review+
(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.
Flags: needinfo?(ajones)
(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.
https://hg.mozilla.org/mozilla-central/rev/bbd61f93a164
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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]:
Attachment #8717314 - Flags: approval-mozilla-beta?
Attachment #8717314 - Flags: approval-mozilla-aurora?
Comment on attachment 8717314 [details] [diff] [review]
include libpulse version in update URL

OK, let's try that.
Should be in 45 beta 6.
Attachment #8717314 - Flags: approval-mozilla-beta?
Attachment #8717314 - Flags: approval-mozilla-beta+
Attachment #8717314 - Flags: approval-mozilla-aurora?
Attachment #8717314 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: