Bug 1239067

Summary: Review Request: libaudclient - audacious D-Bus remote control library
Product: [Fedora] Fedora Reporter: Sergio Basto <sergio>
Component: Package ReviewAssignee: Michael Schwendt <bugs.michael>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: rawhideCC: gwync, package-review
Target Milestone: ---Flags: bugs.michael: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: libaudclient-3.5-0.2.rc2.fc21 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2015-07-23 08:58:57 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Sergio Basto 2015-07-03 12:14:03 UTC
At least 3 packages: conky (bug #1090655), audtty (bug #1090654) and mp3splt-gtk [1] needs this libaudclient , so I think we should have it, If it is an argument Debian also have libaudclient :) 


SPEC: https://sergiomb.fedorapeople.org/libaudclient.spec
SRPM: https://sergiomb.fedorapeople.org/libaudclient-3.5-0.1.rc2.fc21.src.rpm


[1] https://sourceforge.net/p/mp3splt/bugs/180/

Comment 1 Michael Schwendt 2015-07-03 16:29:45 UTC
> Name:           libaudclient
> Group:          Development/Libraries

The Group tag for runtime libraries has been "System Environment/Libraries" for many years. Nowadays the Group tag is obsolete:
https://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag


> BuildRequires:  autoconf
> BuildRequires:  automake

Not needed.


> %description
> audacious D-Bus remote control library

The player is called "Audacious" with upper-case 'A' - except for the logo and file names.


> %package    -n  libaudclient-devel
> Summary:        Development files for libaudclient
> Group:          Development/Libraries/C and C++

Red Hat and Fedora have used group "Development/Libraries" for -devel packages for many years.


> Requires:       %{name} = %{version}-%{release}

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> %description -n libaudclient-devel
> Development files for libaudclient (to communicate with audacious)

As above.


> ### corrects version mismatch
> sed '/^version/cversion=%{version}' -i audclient.pc

If the comment explained *why* it does that, it would be a better comment. Now there's a version mismatch in the file, because there are two different versions and two different ways to retrieve those versions:

  $ grep ^version /usr/lib64/pkgconfig/audclient.pc 
  version=3.5
  $ pkg-config --variable=version audclient
  3.5

  $ grep ^Version /usr/lib64/pkgconfig/audclient.pc 
  Version: 3.5-rc2
  $ pkg-config --atleast-version=3.5 audclient && echo "yes"
  yes
  $ pkg-config --atleast-version=3.5-rc2 audclient && echo "yes"
  yes
  $ pkg-config --atleast-version=3.5-rc3 audclient && echo "yes"
  $


> %build
> %configure
> make %{?_smp_mflags}

Build output is non-verbose. One cannot see which compiler/linker flags are used. You can insert

  sed -i '\,^.SILENT:,d' buildsys.mk.in

before %configure to fix that. Audacious does that for years, too. ;-)


> %files -n libaudclient-devel
> %dir %{_includedir}/audacious/

Directory ownership is okay, since package audacious-devel is not needed for installing libaudclient-devel.

[...]

As for the runtime, last audtty in Fedora rebuilds and seems to run with this.

Comment 2 Sergio Basto 2015-07-03 18:44:38 UTC
(In reply to Michael Schwendt (Fedora Packager Sponsors Group) from comment #1)
> > Name:           libaudclient
> > Group:          Development/Libraries
> 
> The Group tag for runtime libraries has been "System Environment/Libraries"
> for many years. Nowadays the Group tag is obsolete:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag

I read somewhere rpmlint validates groups from /usr/share/doc/rpm/GROUPS 
So I choose  : Development/Libraries 
 
> > BuildRequires:  autoconf
> > BuildRequires:  automake
> 
> Not needed.

OK , fixed 
 
> 
> > %description
> > audacious D-Bus remote control library
> 
> The player is called "Audacious" with upper-case 'A' - except for the logo
> and file names.

OK, fixed  

> > %package    -n  libaudclient-devel
> > Summary:        Development files for libaudclient
> > Group:          Development/Libraries/C and C++
> 
> Red Hat and Fedora have used group "Development/Libraries" for -devel
> packages for many years.

OK, so where is right use 
Group:          Development/Libraries
like in main package ? or should I remove Group on main package ?

 
> > Requires:       %{name} = %{version}-%{release}
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

OK, fixed
 
> > %description -n libaudclient-devel
> > Development files for libaudclient (to communicate with audacious)
> 
> As above.

OK, fixed  

> > ### corrects version mismatch
> > sed '/^version/cversion=%{version}' -i audclient.pc
> 
> If the comment explained *why* it does that, it would be a better comment.
> Now there's a version mismatch in the file, because there are two different
> versions and two different ways to retrieve those versions:
> 
>   $ grep ^version /usr/lib64/pkgconfig/audclient.pc 
>   version=3.5
>   $ pkg-config --variable=version audclient
>   3.5
> 
>   $ grep ^Version /usr/lib64/pkgconfig/audclient.pc 
>   Version: 3.5-rc2
>   $ pkg-config --atleast-version=3.5 audclient && echo "yes"
>   yes
>   $ pkg-config --atleast-version=3.5-rc2 audclient && echo "yes"
>   yes
>   $ pkg-config --atleast-version=3.5-rc3 audclient && echo "yes"
>   $

OK , I removed sed script 
This came in SUSE src.rpm, is not my script. I see now that SUSE 
Version was 3.5~rc2 , now is just 3.5 so it wasn't right at all . 

> > %build
> > %configure
> > make %{?_smp_mflags}
> 
> Build output is non-verbose. One cannot see which compiler/linker flags are
> used. You can insert
> 
>   sed -i '\,^.SILENT:,d' buildsys.mk.in
> 
> before %configure to fix that. Audacious does that for years, too. ;-)

DONE

> > %files -n libaudclient-devel
> > %dir %{_includedir}/audacious/
> 
> Directory ownership is okay, since package audacious-devel is not needed for
> installing libaudclient-devel.

So it is correct ? no modifications ? 

> [...]
> 
> As for the runtime, last audtty in Fedora rebuilds and seems to run with
> this.

Cool! 

Thanks.

SPEC: https://sergiomb.fedorapeople.org/libaudclient.spec
SRPM: https://sergiomb.fedorapeople.org/libaudclient-3.5-0.2.rc2.fc21.src.rpm

Comment 3 Michael Schwendt 2015-07-04 09:56:46 UTC
> I read somewhere rpmlint validates groups from /usr/share/doc/rpm/GROUPS 
> So I choose  : Development/Libraries 
 
$ grep Lib /usr/share/doc/rpm/GROUPS 
Development/Libraries
System Environment/Libraries

As you can see, "System Environment/Libraries" is in there, too.


> OK, so where is right use 
> Group:          Development/Libraries
> like in main package ? or should I remove Group on main package ?

The Group tag can be set for each [sub-]package. Unless you want to drop it everywhere: https://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag


> Requires:       %{name}}%{?_isa} = %{version}-%{release}

The extra '}' in there causes the package to be not installable.


>> Directory ownership is okay, since package audacious-devel
>> is not needed for installing libaudclient-devel.
>
> So it is correct ? no modifications ? 

Yes, that's what "is okay" means. Reviewing is not only about pointing out mistakes, it's also an opportunity to acknowledge things that are done right.
Owning /usr/include/audacious is acceptable according to this:
https://fedoraproject.org/wiki/Packaging:Guidelines#The_directory_is_owned_by_a_package_which_is_not_required_for_your_package_to_function


>  %install
> -%make_install
> +make install DESTDIR=%{buildroot}

A completely unnecessary change. Using %make_install is entirely acceptable.
See:  rpm -E %make_install

  https://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_.25makeinstall_macro_should_not_be_used


[...]

If you fix the Group tag in dist git and the accidental '}', you can get an APPROVED here.

Comment 4 Sergio Basto 2015-07-04 16:25:22 UTC
(In reply to Michael Schwendt (Fedora Packager Sponsors Group) from comment #3)
> > I read somewhere rpmlint validates groups from /usr/share/doc/rpm/GROUPS 
> > So I choose  : Development/Libraries 
>  
> $ grep Lib /usr/share/doc/rpm/GROUPS 
> Development/Libraries
> System Environment/Libraries
> 
> As you can see, "System Environment/Libraries" is in there, too.
> 
> 
> > OK, so where is right use 
> > Group:          Development/Libraries
> > like in main package ? or should I remove Group on main package ?
> 
> The Group tag can be set for each [sub-]package. Unless you want to drop it
> everywhere: https://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag

Sorry, I misunderstood at first time , I'd like keep Group , so : 

Name:   libaudclient
Group:  System Environment/Libraries
(...)
%package    -n  libaudclient-devel
Summary:        Development files for libaudclient
Group:          Development/Libraries

is correct ? or what remove second group tag or second group tag should be: 
Group: System Environment/Libraries ? 


> > Requires:       %{name}}%{?_isa} = %{version}-%{release}
> 
> The extra '}' in there causes the package to be not installable.
> 
> 
> >> Directory ownership is okay, since package audacious-devel
> >> is not needed for installing libaudclient-devel.
> >
> > So it is correct ? no modifications ? 
> 
> Yes, that's what "is okay" means. Reviewing is not only about pointing out
> mistakes, it's also an opportunity to acknowledge things that are done right.
> Owning /usr/include/audacious is acceptable according to this:
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#The_directory_is_owned_by_a_package_which_is_not_required_for_your
> _package_to_function
> 
> 
> >  %install
> > -%make_install
> > +make install DESTDIR=%{buildroot}
> 
> A completely unnecessary change. Using %make_install is entirely acceptable.
> See:  rpm -E %make_install
> 
>  
> https://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_.
> 25makeinstall_macro_should_not_be_used

Fedora's RPM includes a %makeinstall macro but it must NOT be used when make install DESTDIR=%{buildroot} works.
So I change it , is correct ?
 

> [...]
> 
> If you fix the Group tag in dist git and the accidental '}', you can get an
> APPROVED here.

OK thanks,

Comment 5 Michael Schwendt 2015-07-04 16:54:38 UTC
> Fedora's RPM includes a %makeinstall macro but it must NOT be used
> when make install DESTDIR=%{buildroot} works.

There are _two_ macros:

  1. %makeinstall
  2. %make_install

The %makeinstall macro should be avoided, because it redefines many path variables that can lead to ugly side-effects.

The %make_install macro does exactly what you want. Really do take a look at "rpm -E %make_install" to see what the macro does. It's there to make packaging easier. ;-)


> or what remove second group tag or second group tag should be: 
> Group: System Environment/Libraries ? 

No, "Development/Libraries" is correct for library -devel packages.

Comment 6 Sergio Basto 2015-07-05 21:46:27 UTC
Reloaded with last fixes : 
SPEC: https://sergiomb.fedorapeople.org/libaudclient.spec
SRPM: https://sergiomb.fedorapeople.org/libaudclient-3.5-0.2.rc2.fc21.src.rpm


Package Change Request
======================
Package Name: libaudclient
Owners: sergiomb
New Branches: f21 f22

Comment 7 Gwyn Ciesla 2015-07-08 12:19:41 UTC
WARNING: Package does not appear to exist in pkgdb currently. 

Use a New Package Request.

Comment 8 Sergio Basto 2015-07-08 13:10:40 UTC
New Package SCM Request
======================
Package Name: libaudclient
Short Description: Client library for audacious
Upstream URL: http://audacious-media-player.org/
Owners: sergiomb
New Branches: f21 f22

Comment 9 Gwyn Ciesla 2015-07-08 15:45:42 UTC
Git done (by process-git-requests).

Comment 10 Sergio Basto 2015-07-09 03:26:10 UTC
I just have master branch ! , went to  pkgdb [1]

I requested branch f21 and f22 :

    Branch f21 created for user sergiomb
    Branch f22 created for user sergiomb

but after that still not have: branch f22 and f21:


    fedpkg clone libaudclient
    cd libaudclient/
    [snip]
    fedpkg push; fedpkg build
    fedpkg switch-branch f22

Unknown remote branch origin/f22

[1] https://admin.fedoraproject.org/pkgdb/package/libaudclient/

Comment 11 Michael Schwendt 2015-07-09 07:17:17 UTC
$ fedpkg clone libaudclient
Cloning into 'libaudclient'...
remote: Counting objects: 8, done.
remote: Compressing objects: 100% (6/6), done.
remote: Total 8 (delta 0), reused 0 (delta 0)
Receiving objects: 100% (8/8), done.
Checking connectivity... done.
$ cd libaudclient
$ fedpkg switch-branch f22
Branch f22 set up to track remote branch f22 from origin.
$ fedpkg switch-branch f21
Branch f21 set up to track remote branch f21 from origin.
$ fedpkg switch-branch master
Switched to branch 'master'
$ fedpkg switch-branch f22
Switched to branch 'f22'
$

Try again?

Comment 12 Sergio Basto 2015-07-09 16:12:40 UTC
Today git pull did: 

From ssh://pkgs.fedoraproject.org/libaudclient
 * [new branch]      f21        -> origin/f21
 * [new branch]      f22        -> origin/f22

Thanks,

Comment 13 Fedora Update System 2015-07-09 16:26:23 UTC
libaudclient-3.5-0.2.rc2.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/libaudclient-3.5-0.2.rc2.fc22

Comment 14 Fedora Update System 2015-07-09 16:28:31 UTC
libaudclient-3.5-0.2.rc2.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/libaudclient-3.5-0.2.rc2.fc21

Comment 15 Sergio Basto 2015-07-09 16:37:53 UTC
Michael Schwendt, 
Would you like join on Main Contact and Package Administrator of this package ? 
if yes you may request it in [1]. I will approve it . 

Thanks, 

[1] https://admin.fedoraproject.org/pkgdb/package/libaudclient/

Comment 16 Fedora Update System 2015-07-13 19:10:16 UTC
libaudclient-3.5-0.2.rc2.fc22 has been pushed to the Fedora 22 testing repository.

Comment 17 Fedora Update System 2015-07-23 08:58:57 UTC
libaudclient-3.5-0.2.rc2.fc22 has been pushed to the Fedora 22 stable repository.

Comment 18 Fedora Update System 2015-07-23 08:59:15 UTC
libaudclient-3.5-0.2.rc2.fc21 has been pushed to the Fedora 21 stable repository.