New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bluez: 5.50 -> 5.51 #69460
bluez: 5.50 -> 5.51 #69460
Conversation
@peterhoeg maybe? :) This fixes some annoying bugs of headsets not reconnecting correctly :) |
enableWiimote ? false, enableMidi ? false, enableSixaxis ? false }: | ||
|
||
stdenv.mkDerivation rec { | ||
name = "bluez-5.50"; | ||
name = "bluez-5.51"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to switch to pname
and version
? I think it is useful to have a separate version field.
Doing that while we update to a new version might prevent rebuilding a lot of packages for a trivial change.
@@ -1,13 +1,13 @@ | |||
{ stdenv, fetchurl, pkgconfig, dbus, glib, alsaLib, | |||
python3, readline, udev, libical, systemd, fetchpatch, | |||
python3, readline, udev, libical, systemd, | |||
enableWiimote ? false, enableMidi ? false, enableSixaxis ? false }: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newer bluez versions have support for additional optional features, at least
--enable-nfc
--enable-mesh
--enable-sap
It might be out of the scope of this PR though.
b196b67
to
317813d
Compare
@mweinelt @Moredread sorry for the delay, but I have updated the pr |
++ optional enableSixaxis [ "--enable-sixaxis" ] | ||
++ optional enableNfc [ "--enable-nfc" ] | ||
++ optional enableMesh [ "--enable-mesh" ] | ||
++ optional enableSap [ "--enable-sap" ]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sap needs json_c as an additional dependency.
I've pushed a commit to https://github.com/Moredread/nixpkgs/tree/bluez which also adds another flag (--enable-health
) and enables all optional flags for bluezFull
.
I've sorted the flags, so it might fail to merge to the master branch though.
Feel free to use the commit as basis for your PR or squash it into yours. :)
Enabling the flags and updating to 5.51 increases the size of bluezFull from 188MB to 189 MB. IMO this is negligible. |
9298d75
to
c96af21
Compare
@Moredread ty, I applied your patch :) |
f61f98a
to
ac95603
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've cherry-picked the PR locally to the 19.09 release channel and switched my NixOS install over to it.
Had no problems so far. :)
For consistency I would propose squashing the second commit and adding a short description like
Also add new configuration flag options and enable them for the bluezFull package by default.
Also add new configuration flag options and enable them for the bluezFull package by default.
ac95603
to
1ec1fc2
Compare
I've tested this change for a few days now as well on my laptop and was pretty happy about my headset being consistently connected as A2DP over HSP/HFP. So no issues here as well. |
And the best, finally reconnecting of the headset works flawlessly! |
enableHealth = true; | ||
enableMesh = true; | ||
enableMidi = true; | ||
enableNfc = true; | ||
enableSap = true; | ||
enableSixaxis = true; | ||
enableWiimote = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't you just default all the values to true? and get rid of passing the arguments explicitly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are two different packages. bluez
and bluezFull
. I'm not sure why there were two in the first place, but for one of them the arguments need to be passed. :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the xxxFull pattern is usually to reduce closure size, either the nonFull version is bloated, or there's an insignificant difference:
[09:06:37] jon@jon-desktop /home/jon/projects/nixpkgs (master)
$ nix-build -A bluez
/nix/store/qhiqw4g1nalyxmzbr2skc14d1dcghgbw-bluez-5.50
[09:06:45] jon@jon-desktop /home/jon/projects/nixpkgs (master)
$ nix path-info -Sh ./result
/nix/store/qhiqw4g1nalyxmzbr2skc14d1dcghgbw-bluez-5.50 183.1M
[08:52:58] jon@jon-desktop /home/jon/projects/nixpkgs (master)
$ nix-build -A bluezFull
these paths will be fetched (1.28 MiB download, 7.71 MiB unpacked):
/nix/store/l85pyxn643sd6n768s0rla4aczhsc64d-bluez-5.50
copying path '/nix/store/l85pyxn643sd6n768s0rla4aczhsc64d-bluez-5.50' from 'https://cache.nixos.org'...
/nix/store/l85pyxn643sd6n768s0rla4aczhsc64d-bluez-5.50
[09:07:00] jon@jon-desktop /home/jon/projects/nixpkgs (master)
$ nix path-info -Sh ./result
/nix/store/l85pyxn643sd6n768s0rla4aczhsc64d-bluez-5.50 184.9M
1% difference doesn't seem like much to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the systemd package seems to be the majority of the closure, but it looks like this package is very dependent on systemd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be in favor of just aliasing bluezFull to bluez, and just having 1 derivation
|
||
stdenv.mkDerivation rec { | ||
name = "bluez-5.50"; | ||
version = "5.51"; | ||
name = "bluez-${version}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name = "bluez-${version}"; | |
pname = "bluez"; |
|
||
src = fetchurl { | ||
url = "mirror://kernel/linux/bluetooth/${name}.tar.xz"; | ||
sha256 = "048r91vx9gs5nwwbah2s0xig04nwk14c5s0vb7qmaqdvighsmz2z"; | ||
sha256 = "1fpbsl9kkfq6mn6n0dg4h0il4c7fzhwhn79gh907k5b2kwszpvgb"; | ||
}; | ||
|
||
pythonPath = with python3.pkgs; [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pygobject2 should not be needed since 2014 (RadiusNetworks/bluez@7adf8d0)
I have updated my branch with the proposed changes in this PR at https://github.com/Moredread/nixpkgs/tree/bluez It updates As this PR fixes issues for headphone usage, I think it would be good if we can get it merged asap. :) |
I've tested these changes for over a month now and I am quite happy with the headphone changes as well. |
@Moredread Actually with your latest commit the bluez testsuite seems to fail.
|
Yeah sorry for my delay, too much todo. @Moredread will you open a pr with your changes? :) |
bluezFull has been aliased to bluez for a while now. See - NixOS/nixpkgs#69460 (comment) - NixOS/nixpkgs#74995
Motivation for this change
New version with a lot of fixes.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @