Skip to content
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

Closed
wants to merge 1 commit into from
Closed

bluez: 5.50 -> 5.51 #69460

wants to merge 1 commit into from

Conversation

bkchr
Copy link
Contributor

@bkchr bkchr commented Sep 26, 2019

Motivation for this change

New version with a lot of fixes.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@bkchr
Copy link
Contributor Author

bkchr commented Oct 2, 2019

@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";
Copy link
Contributor

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 }:
Copy link
Contributor

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.

@FRidh FRidh changed the base branch from master to staging October 22, 2019 07:55
@bkchr bkchr force-pushed the bkchr-bluez-5-51 branch 2 times, most recently from b196b67 to 317813d Compare October 25, 2019 07:35
@bkchr
Copy link
Contributor Author

bkchr commented Oct 25, 2019

@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" ]);
Copy link
Contributor

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

@Moredread
Copy link
Contributor

Moredread commented Oct 26, 2019

Enabling the flags and updating to 5.51 increases the size of bluezFull from 188MB to 189 MB. IMO this is negligible.

@bkchr
Copy link
Contributor Author

bkchr commented Oct 28, 2019

@Moredread ty, I applied your patch :)

@bkchr bkchr force-pushed the bkchr-bluez-5-51 branch 2 times, most recently from f61f98a to ac95603 Compare October 28, 2019 08:32
Copy link
Contributor

@Moredread Moredread left a 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.
@mweinelt
Copy link
Member

mweinelt commented Oct 30, 2019

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.

@bkchr
Copy link
Contributor Author

bkchr commented Oct 30, 2019

And the best, finally reconnecting of the headset works flawlessly!

Comment on lines +15622 to +15628
enableHealth = true;
enableMesh = true;
enableMidi = true;
enableNfc = true;
enableSap = true;
enableSixaxis = true;
enableWiimote = true;
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

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}";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
name = "bluez-${version}";
pname = "bluez";


src = fetchurl {
url = "mirror://kernel/linux/bluetooth/${name}.tar.xz";
sha256 = "048r91vx9gs5nwwbah2s0xig04nwk14c5s0vb7qmaqdvighsmz2z";
sha256 = "1fpbsl9kkfq6mn6n0dg4h0il4c7fzhwhn79gh907k5b2kwszpvgb";
};

pythonPath = with python3.pkgs; [
Copy link
Contributor

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)

@FRidh FRidh added this to Needs review in Staging Nov 12, 2019
@FRidh FRidh moved this from Needs review to WIP in Staging Nov 12, 2019
@FRidh FRidh added this to the 20.03 milestone Nov 12, 2019
@Moredread
Copy link
Contributor

I have updated my branch with the proposed changes in this PR at https://github.com/Moredread/nixpkgs/tree/bluez

It updates bluez to 5.52 as well.

As this PR fixes issues for headphone usage, I think it would be good if we can get it merged asap. :)

@mweinelt
Copy link
Member

mweinelt commented Dec 3, 2019

I've tested these changes for over a month now and I am quite happy with the headphone changes as well.

@mweinelt
Copy link
Member

mweinelt commented Dec 3, 2019

@Moredread Actually with your latest commit the bluez testsuite seems to fail.

==================================
   bluez 5.52: ./test-suite.log
==================================

# TOTAL: 27
# PASS:  26
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: unit/test-midi
====================

**
ERROR:unit/test-midi.c:288:compare_events: assertion failed (ev1->data.ext.len == ev2->data.ext.len): (5 == 4)

Raw BLE packets read - init
Raw BLE packets read - setup
Raw BLE packets read - setup complete
Raw BLE packets read - run
Raw BLE packets read - test passed
Raw BLE packets read - teardown
Raw BLE packets read - teardown complete
Raw BLE packets read - done

Raw BLE packets SysEx read - init
Raw BLE packets SysEx read - setup
Raw BLE packets SysEx read - setup complete
Raw BLE packets SysEx read - run
Raw BLE packets SysEx read - test passed
Raw BLE packets SysEx read - teardown
Raw BLE packets SysEx read - teardown complete
Raw BLE packets SysEx read - done

ALSA Seq events to Raw BLE packets - init
ALSA Seq events to Raw BLE packets - setup
ALSA Seq events to Raw BLE packets - setup complete
ALSA Seq events to Raw BLE packets - run
ALSA Seq events to Raw BLE packets - test passed
ALSA Seq events to Raw BLE packets - teardown
ALSA Seq events to Raw BLE packets - teardown complete
ALSA Seq events to Raw BLE packets - done

ALSA SysEx events to Raw BLE packets - init
ALSA SysEx events to Raw BLE packets - setup
ALSA SysEx events to Raw BLE packets - setup complete
ALSA SysEx events to Raw BLE packets - run
Bail out! ERROR:unit/test-midi.c:288:compare_events: assertion failed (ev1->data.ext.len == ev2->data.ext.len): (5 == 4)
FAIL unit/test-midi (exit status: 134)

============================================================================
Testsuite summary for bluez 5.52
============================================================================
# TOTAL: 27
# PASS:  26
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0
============================================================================
See ./test-suite.log
============================================================================
make[3]: *** [Makefile:10068: test-suite.log] Error 1
make[2]: *** [Makefile:10176: check-TESTS] Error 2
make[1]: *** [Makefile:10570: check-am] Error 2
make: *** [Makefile:10572: check] Error 2
builder for '/nix/store/saiyncn4rvx1f8mi13awvr3z0bqzkvph-bluez-5.52.drv' failed with exit code 2

@bkchr
Copy link
Contributor Author

bkchr commented Dec 3, 2019

Yeah sorry for my delay, too much todo. @Moredread will you open a pr with your changes? :)

@bkchr bkchr closed this Dec 3, 2019
Staging automation moved this from WIP to Done Dec 3, 2019
@Moredread
Copy link
Contributor

@bkchr np, I'll try to do this tomorrow.
@mweinelt strange, I don't think it happend on my side... I'll post my results in the new PR asap.

@Moredread Moredread mentioned this pull request Dec 4, 2019
10 tasks
timokau added a commit to timokau/dotfiles that referenced this pull request Sep 27, 2022
bluezFull has been aliased to bluez for a while now. See

- NixOS/nixpkgs#69460 (comment)
- NixOS/nixpkgs#74995
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants