Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1239067 - Review Request: libaudclient - audacious D-Bus remote control library
Summary: Review Request: libaudclient - audacious D-Bus remote control library
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Michael Schwendt
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-07-03 12:14 UTC by Sergio Basto
Modified: 2015-07-23 08:59 UTC (History)
2 users (show)

Fixed In Version: libaudclient-3.5-0.2.rc2.fc21
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-07-23 08:58:57 UTC
Type: Bug
Embargoed:
bugs.michael: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

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.


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