Bug 1239067
Summary: | Review Request: libaudclient - audacious D-Bus remote control library | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Sergio Basto <sergio> |
Component: | Package Review | Assignee: | Michael Schwendt <bugs.michael> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | unspecified | Docs Contact: | |
Priority: | unspecified | ||
Version: | rawhide | CC: | 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
> 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. (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 > 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. (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, > 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. 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 WARNING: Package does not appear to exist in pkgdb currently. Use a New Package Request. 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 Git done (by process-git-requests). 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/ $ 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? Today git pull did: From ssh://pkgs.fedoraproject.org/libaudclient * [new branch] f21 -> origin/f21 * [new branch] f22 -> origin/f22 Thanks, 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 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 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/ libaudclient-3.5-0.2.rc2.fc22 has been pushed to the Fedora 22 testing repository. libaudclient-3.5-0.2.rc2.fc22 has been pushed to the Fedora 22 stable repository. libaudclient-3.5-0.2.rc2.fc21 has been pushed to the Fedora 21 stable repository. |