Bug 974145 - (bluez5) Review Request: bluez5 - Bluetooth v5 utilities
Review Request: bluez5 - Bluetooth v5 utilities
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Björn "besser82" Esser
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-13 09:59 EDT by Don Zickus
Modified: 2013-08-14 15:59 EDT (History)
9 users (show)

See Also:
Fixed In Version: bluez-5.8-1.fc20
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-08-14 15:59:48 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
improvements for spec-file (9.33 KB, patch)
2013-06-17 04:51 EDT, Björn "besser82" Esser
no flags Details | Diff
improvements for spec-file with proper upgrade-path (9.91 KB, patch)
2013-06-17 07:26 EDT, Björn "besser82" Esser
no flags Details | Diff

  None (edit)
Description Don Zickus 2013-06-13 09:59:07 EDT
Spec URL: http://dzickus.fedorapeople.org/bluez5.spec
SRPM URL: http://dzickus.fedorapeople.org/bluez5-5.5-3.fc19.src.rpm
Description: 
Utilities for use in Bluetooth applications:
        - hcitool
        - hciattach
        - hciconfig
        - bluetoothd
        - l2ping
        - rfcomm
        - sdptool
        - bccmd
        - bluetoothctl
        - btmon
        - hcidump
        - l2test
        - rctest
        - start scripts (Red Hat)
        - pcmcia configuration files

The BLUETOOTH trademarks are owned by Bluetooth SIG, Inc., U.S.A.


Fedora Account System Username: dzickus
Comment 1 Björn "besser82" Esser 2013-06-14 10:58:42 EDT
Since your package ships a daemon (bluetoothd), I think it should build fully hardened. [1]

/usr/libexec/bluetooth/bluetoothd:
 Position Independent Executable: no, normal executable!
 Stack protected: yes
 Fortify Source functions: yes (some protected functions found)
	unprotected: poll
	unprotected: strncpy
	unprotected: memset
	unprotected: snprintf
	unprotected: strcat
	unprotected: memmove
	unprotected: read
	unprotected: recv
	unprotected: strcpy
	unprotected: memcpy
	unprotected: sprintf
	unprotected: fread
	protected: fdelt
	protected: strncpy
	protected: vsnprintf
	protected: strncat
	protected: snprintf
	protected: strcat
	protected: vfprintf
	protected: vsyslog
	protected: strcpy
	protected: memcpy
	protected: printf
	protected: sprintf
	protected: syslog
 Read-only relocations: yes
 Immediate binding: no, not found!

consider adding `%global _hardened_build 1` on top of spec enabling PIE and fully functional RELRO.

[1] https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#PIE
Comment 2 Don Zickus 2013-06-14 15:45:59 EDT
Hi Bjorn,

I have made the requested changes and uploaded them to the same spot in comment 0.

Please let me know if there is anything else I need to do.

Cheers,
Don
Comment 3 Björn "besser82" Esser 2013-06-17 04:51:21 EDT
Created attachment 761959 [details]
improvements for spec-file

Hi Don!

Just took a deeper look inside spec-file:

  * arch-conditionals on BRs:

    %ifnarch s390 s390x
    BuildRequires: libusbx-devel
    %endif

    ---> There's a discussion around on f-packaging ml. [1]
         You should possibly track where it'll lead to and
         act on this accordingly.

Some changes I'd propose (as in attached patch):

-Source: http://www.kernel.org/pub/linux/bluetooth/bluez-%{version}.tar.xz
+Source0: http://www.kernel.org/pub/linux/bluetooth/bluez-%{version}.tar.xz

Just cosmetic...


-BuildRequires: git
-BuildRequires: flex
-BuildRequires: dbus-devel >= 0.90
-BuildRequires: libusb-devel, glib2-devel
+BuildRequires: git flex dbus-devel
+BuildRequires: libusb-devel glib2-devel

reduced lines, dbus-devel has more recent ver in F17+, so no need for explicit min-version.


-BuildRequires: libtool autoconf automake
+BuildRequires: libtool

autotools are pulled on libtool requires.


-Requires: bluez5-libs = %{version}-%{release}
+Requires: bluez5-libs%{?_isa} = %{version}-%{release}

requires should be arched, when package is arched as well.


-Requires: dbus >= 0.60

dbus is added from autorequires


-Requires: hwdata >= 0.215
+Requires: hwdata

hwdata has more recent ver in F17+, so no need for explicit min-version.


-Obsoletes: bluez
 ...
+Obsoletes: bluez <= 4.94-4
 ...

reduce rpmlint complains...


-Requires: hwdata >= 0.215
-Requires(preun): /bin/systemctl
-Requires(post): /bin/systemctl
+# conditional and %%else and be safely removed, if not intended for < F18
+%if 0%{?fedora} >= 18 || 0%{?rhel} >= 7
+%{systemd_requires}
+%else
+Requires(post): systemd-units
+Requires(preun): systemd-units
+Requires(postun): systemd-units
+%endif

+# conditional and %%else and be safely removed, if not intended for < F18
+%if 0%{?fedora} >= 18 || 0%{?rhel} >= 7
+%post
+%systemd_post bluetooth.service
+
+%preun
+%systemd_preun bluetooth.service
+
+%postun
+%systemd_postun_with_restart bluetooth.service
+%else

There are special systemd-macros avail on F18+


-	- hcitool
 ...
+        - hcitool
 ...

mixed use of spaces and tabs...


-make V=1
+make V=1 %{?_smp_mflags}

enable parallel build


-make install DESTDIR=$RPM_BUILD_ROOT
+%make_install

exchanged with macro, not mandatory


 /sbin/ldconfig -n %{buildroot}%{_libdir}

What is this used for?


-%triggerun -- bluez < 4.94-4
-/bin/systemctl --no-reload enable bluetooth.service >/dev/null 2>&1 || :

+%triggerun -- bluez <= 4.94-4
+if /sbin/chkconfig --level 5 bluetooth ; then
+        /bin/systemctl --no-reload enable bluetooth.service >/dev/null 2>&1 || :
+fi
+
+%post libs -p /sbin/ldconfig

respect admin's sysconfig on upgrade-path. only enable service by default, if was so before.


generally I made some simplifications in spec, too, but don't want to comment here, since will spam the list with not really useful comments...


[1] https://lists.fedoraproject.org/pipermail/packaging/2013-June/009206.html

If you want, I can take full review.

Cheers,
  Björn
Comment 4 Björn "besser82" Esser 2013-06-17 07:26:32 EDT
Created attachment 762006 [details]
improvements for spec-file with proper upgrade-path

Forgot to encounter a proper upgrade-path from old bluez, but here comes now. :)
Comment 5 Don Zickus 2013-06-18 13:42:25 EDT
(In reply to Björn Esser from comment #4)
> Created attachment 762006 [details]
> improvements for spec-file with proper upgrade-path
> 
> Forgot to encounter a proper upgrade-path from old bluez, but here comes
> now. :)

Ok. Thanks Bjorn!

Let me run this by Bastien.  I need to find a devel box that supports systemd to regenerate the srpm (my RHEL-6 boxes are not good enough :-( ).

Cheers,
Don
Comment 6 Bastien Nocera 2013-06-18 15:11:02 EDT
The upgrade path bits are completely broken.

The 2 packages aren't compatible, nor should they be installed together. The obexd and obex-data-server versions are completely different from the bluez versions. It's completely expected that bluez5 would not be installable on a stock install yet (gnome-shell and gnome-bluetooth have been ported, NetworkManager hasn't yet).

As you've noted though, there's no need to support anything older than F20.
Comment 7 Kalev Lember 2013-06-21 05:55:28 EDT
I am not sure a new separate bluez5 package makes sense here. As Bastien notes, bluez 4.x and 5.x aren't meant to be parallel installable.

Why can't we just update the existing bluez package to version 5.x?
Comment 8 Don Zickus 2013-06-21 08:41:25 EDT
Hi Kalev,

I think the API is so different, that Bastien wanted to seperate the packages.  I'll leave it to him to comment.  The seperation is really his work.  

Cheers,
Don
Comment 9 Bastien Nocera 2013-07-01 02:35:08 EDT
Because we wanted to drop the legacy bits in the bluez4 packaging, and that we wanted packages to conflict when we couldn't upgrade a particular version.

If I have bluez4, and bluez4 versions of gnome-bluetooth, and the KDE Bluetooth manager, what happens when gnome-bluetooth requires bluez5? I won't be able to upgrade KDE's BlueZ manager as it still requires bluez4. That stops us from breaking people's installations, especially during the migration to BlueZ 5.
Comment 10 Kevin Kofler 2013-07-09 13:04:25 EDT
I agree with Kalev that we should just upgrade the existing bluez package. Having 2 Conflicting (at RPM level) versions of BlueZ around is a very bad idea. (The same idea had been proposed and discarded for NetworkManager. Having different desktop environments require conflicting packages prevents installing them on the same machine, which is very broken for machines with more than one user.)

KDE's BlueDevil has already been ported to BlueZ 5 in a branch:
https://git.reviewboard.kde.org/r/108912/
https://projects.kde.org/projects/playground/libs/libbluedevil/repository/show?rev=bluez5
https://projects.kde.org/projects/extragear/base/bluedevil/repository/show?rev=bluez5
Until the release (2.0?) from that branch, we can ship branch snapshots.

(I'm CCing jreznik, who's done most of the BlueDevil packaging lately, but I think he'll tell you the same thing.)
Comment 11 Kevin Kofler 2013-07-17 18:10:17 EDT
CCing ltinkl and dvratil, who have been asking about this feature on IRC earlier today. Please read the comment trail on this bug, and also the log of yesterday's KDE SIG meeting you were supposed to attend:
http://meetbot.fedoraproject.org/fedora-meeting/2013-07-16/kde-sig.2013-07-16-15.08.log.html
You will find all your questions answered here. (The most important answer: There's a git branch of BlueDevil which should support BlueZ 5 already, see my comment #10 above.)
Comment 12 Björn "besser82" Esser 2013-07-19 10:09:51 EDT
So is a review still needed here or will you upgrade the existing bluez-pkg, then?
Comment 13 Bastien Nocera 2013-07-22 04:00:12 EDT
How does that solve the concerns in comment 9 about dropping legacy packages?
Comment 14 Kevin Kofler 2013-07-22 16:42:40 EDT
Use Obsoletes for that.
Comment 15 Don Zickus 2013-07-24 14:41:06 EDT
(In reply to Kevin Kofler from comment #14)
> Use Obsoletes for that.

How would you obsolete packages that use bluez4?  Isn't that a rough guess at best?

My understanding is that the API change will break all the bluez4 apps.  How do you manage that from a packaging perspective?  You need to block the upgrade if any package depends on bluez4 and does _not_ have a bluez5 component to migrate too.

Having a separate bluez5 package makes that simpler to handle.  I am not sure what rpm magic we can add to keep it inside one package.  But I defer to those who have been here before.  Perhaps obsoletes can work, I just can't see how.

Cheers,
Don
Comment 16 Kevin Kofler 2013-07-24 21:32:52 EDT
Well, first you find out what packages are affected. It's your job as the people driving the change to do that. repoquery can find at least those where RPM Requires has been used correctly. I don't know if there are packages where that is missing. (If so, arguably, the package is already broken.)

And then you file tracking bugs here to try and get those packages ported. They'll just be broken in Rawhide until that happens. If the package cannot be ported, a fallback plan (either introducing a bluez4 compatibility package or just dropping the offending BlueZ-using package, having it be Obsoleted by some other package) needs to be established. But really, the goal should be to get everything affected ported. Conflicts are evil, especially in cases such as this where they prevent installing multiple desktop environments.
Comment 17 Kalev Lember 2013-08-10 17:40:48 EDT
I have talked to Bastien and promised to help this along. He'll be on a vacation for a month which complicates things, but hopefully we can sort things out.

This is my plan:

 1) Submit a late Change for F20. I have talked to jreznik and it should still be possible to get this in. (https://fedoraproject.org/wiki/Changes/Bluez5 if anyone wants to help clean this up.)

 2) Coordinate with the KDE team (Kevin Kofler) for bluedevil git snapshot packaging.

 3) I'll handle the GNOME side

 4) Figure out what to do with pulseaudio, either backport BlueZ 5 support to pulseaudio 4 or update to a git snapshot.

 5) The Dans are going to handle the NetworkManager side

 6) Go on with the existing bluez package and drop the separate bluez5

I have ported the spec file changes here here to the current bluez packaging and pushed this to https://github.com/kalev/bluez/commits/master . It is only a temporary location while I handle the distro wide coordination, but it should be mostly ready to push to fedora git proper. Can you guys give it a quick look, please, to see if the changes there make sense?

Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5804536

Don, can you request the ACLs for bluez at https://admin.fedoraproject.org/pkgdb/acls/name/bluez please?
Comment 18 Kalev Lember 2013-08-14 15:59:48 EDT
FESCo discussed BlueZ 5 today at the meeting and gave a green light to landing it in F20. The plan is to land the changes now, to give time to integrate everything together. One week before the Beta Freeze, there's going to be another FESCo meeting to decide if there's a need to enact the contingency plan (reverting back to BlueZ 4) in case there release blocking desktops have regressions.

I'm closing this ticket since we're going with updating the 'bluez' package instead and won't have a separate bluez5.

Note You need to log in before you can comment on or make changes to this bug.