Bug 825854 - Review Request: zita-alsa-pcmi - alsa pcm libraries
Review Request: zita-alsa-pcmi - alsa pcm libraries
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
17
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FedoraAudio
  Show dependency treegraph
 
Reported: 2012-05-28 14:02 EDT by Jørn Lomax
Modified: 2012-11-26 06:26 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-11-26 06:26:37 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
oget.fedora: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
Patch for sed line, apps version (1.23 KB, patch)
2012-06-21 05:51 EDT, Jørn Lomax
no flags Details | Diff
patch for sed line, libs version (834 bytes, patch)
2012-06-21 05:53 EDT, Jørn Lomax
no flags Details | Diff

  None (edit)
Description Jørn Lomax 2012-05-28 14:02:16 EDT
Spec URL: http://dl.dropbox.com/u/14369138/RPM/zita-alsa-pcmi.spec
SRPM URL: http://dl.dropbox.com/u/14369138/RPM/zita-alsa-pcmi-0.2.0-1.fc17.src.rpm
Description: It provides easy access
to ALSA PCM devices, taking care of the many functions required to
open, initialise and use a hw: device in mmap mode, and providing
floating point audio data.
This is my first package and I need a sponsor 

Fedora Account System Username: jvlomax
Comment 1 Jørn Lomax 2012-05-28 14:06:00 EDT
I'm a google summer of code student working on the fedora audio spin
Comment 2 Brendan Jones 2012-05-29 06:40:39 EDT
Hi Jørn,

on first looks, this is looking pretty good. Its a good idea to inlcude the rpmlint output against all the packages in the review request.
Comment 3 Michael Schwendt 2012-05-30 06:02:18 EDT
There's an eyebrow-rasier in the spec file:

> sed -i -e 's|-O2 -Wall|%{optflags} -I../libs|' \
>     -e 's|ldconfig||' libs/Makefile
> # We don't want to run ldconfig in the build environment. 

Why?
And later:

> # install missing symlink (was giving no-ldconfig-symlink rpmlint errors)
> ln -sf %{_libdir}/lib%{name}.so.%{version} %{buildroot}%{_libdir}/lib%{name}.so.0

Running ldconfig would create that missing symlink. You could even explicitly run

   ldconfig -n %{buildroot}%{_libdir}

to create the needed symlink instead of creating it manually with a hardcoded version.
Comment 4 Jørn Lomax 2012-06-07 05:46:49 EDT
I based the spec file on the zita-resample .spec file, as they are both similar in how they work (and I'm still learning to use .spec files.). As to why the author of that package doesn't want to use ldconfig in the build enviroment is do not know. Should I just replace:

> sed -i -e 's|-O2 -Wall|%{optflags} -I../libs|' \
>     -e 's|ldconfig||' libs/Makefile
> # We don't want to run ldconfig in the build environment. 

with ldconfig instead ?

(In reply to comment #3)
> There's an eyebrow-rasier in the spec file:
> 
> > sed -i -e 's|-O2 -Wall|%{optflags} -I../libs|' \
> >     -e 's|ldconfig||' libs/Makefile
> > # We don't want to run ldconfig in the build environment. 
> 
> Why?
> And later:
> 
> > # install missing symlink (was giving no-ldconfig-symlink rpmlint errors)
> > ln -sf %{_libdir}/lib%{name}.so.%{version} %{buildroot}%{_libdir}/lib%{name}.so.0
> 
> Running ldconfig would create that missing symlink. You could even
> explicitly run
> 
>    ldconfig -n %{buildroot}%{_libdir}
> 
> to create the needed symlink instead of creating it manually with a
> hardcoded version.
Comment 5 Brendan Jones 2012-06-07 05:59:46 EDT
As Michael suggested you could run ldconfig in the buildroot. Just make it part of your sed command


sed -i -e 's|-O2 -Wall|%{optflags}|' \
    -e 's|ldconfig|ldconfig -n %{buildroot}/%{_libdir}|' libs/Makefile
Comment 6 Jørn Lomax 2012-06-07 06:04:55 EDT
Fixed
Comment 7 Brendan Jones 2012-06-07 07:20:54 EDT
You can drop -I../libs from the sed command on libs/Makefile

Its also customary to bump the release number and add an entry in the change log, even in review. If you have a fedorapeople.org/ a/c now, you can host them there.

Repost the updated links here.

I notice noone has stepped up as yet to sponsor you - how about you send an email to the devel list (and the music list) requesting a sponsor? The pending review requests line can get pretty long.
Comment 8 Jørn Lomax 2012-06-08 07:03:54 EDT
I have fixed it as requested and updated the change log, but now rpmlint gives me this warning (I'm guessing for running ld config in the build enviroment):

>rpm-buildroot-usage %prep -e 's|ldconfig|ldconfig -n %{buildroot}/%{_libdir}|' >libs/Makefile
>$RPM_BUILD_ROOT should not be touched during %build or %prep stage, as it may
>break short circuit builds.

Should i just go back to the original, or should i ignore the warning?

I haven't set my fedorapeople.org/ a/c up properly yet, but i'll fix that next week.
Comment 9 Brendan Jones 2012-06-08 07:21:54 EDT
Ah - ok. It is kind of a false positive but one way is to remove the warning is to change it back the way it was and explicitly call 

ldconfig -n %{buildroot}%{_libdir} 

in your %build section
Comment 10 Jørn Lomax 2012-06-14 08:18:48 EDT
I don't see why i should have to call ldconfig in %build when rpmlint gives a warning and

> sed -i -e 's|-O2 -Wall|%{optflags} -I../libs|' \
>     -e 's|ldconfig||' libs/Makefile
> # We don't want to run ldconfig in the build environment. 
does it without warning. It might not be as streamlined, but it does the trick (and i have seen several packages doing it this way)

The files have now been updated and moved to http://jvlomax.fedorapeople.org/packeging/

The rpmlint reports no errors/warnings
Comment 11 Brendan Jones 2012-06-14 08:42:22 EDT
(In reply to comment #9)
> Ah - ok. It is kind of a false positive but one way is to remove the warning
> is to change it back the way it was and explicitly call 
> 
> ldconfig -n %{buildroot}%{_libdir} 
> 
> in your %build section

That should read '%install section'.

Leave the sed lines as they are and replace the ln -sf lines with  ldconfig -n %{buildroot}%{_libdir} in the %install section

You won't get any rpmlint warnings
Comment 12 Jørn Lomax 2012-06-14 08:55:20 EDT
That makes more sense :D Fixed it now, and no rpmlint warning anymore. Thanks
Comment 13 Jørn Lomax 2012-06-14 20:46:54 EDT
(ongoing) informal reviews can be found at https://bugzilla.redhat.com/show_bug.cgi?id=829971 
https://bugzilla.redhat.com/show_bug.cgi?id=829970
Comment 14 Jørn Lomax 2012-06-15 06:41:01 EDT
I have gone though the checklist, and can't find any issued. I built and tested it on fedora 17 i686 and it's working fine
Comment 15 Orcan Ogetbil 2012-06-15 23:01:46 EDT
I'll do the review, but package does not build
   http://koji.fedoraproject.org/koji/taskinfo?taskID=4169293
Comment 16 Orcan Ogetbil 2012-06-17 14:32:33 EDT
Sorry, I accidentally downloaded the SRPM from the original post. Now, as a starter, I want to point a few things:

* Changelog format does not fit the requirements:
   http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs

! -I../libs is not required in libs/Makefile. It is only required in apps.Makefile.

* In the buildlog I see lines such as 
   g++ -L/usr//usr/lib -o alsa_loopback ...
Probably because of the line
   LDFLAGS += -L$(PREFIX)/$(LIBDIR)
It would be nice to fix this.

Well, since this is your first package, let us do the things the proper way. Instead of hacking in the Makefiles with sed, it would be good to make patches and send them upstream. This will require a different approach at a few places. For instance, since we want to be able to override the CXXFLAGS, we might want to change
   CXXFLAGS += -O2 -Wall ...
with something like
   CXXFLAGS += -O2 -Wall ... -I../libs $(OPTFLAGS)
Then specify OPTFLAGS=%{optflags} in your make line. Of course there are other methods, but this one is simple and upstreamable.
Comment 17 Jørn Lomax 2012-06-21 05:51:29 EDT
Created attachment 593402 [details]
Patch for sed line, apps version

This is to remove the sed lines from the .spec. This only takes care of the /apps sed line
Comment 18 Jørn Lomax 2012-06-21 05:53:00 EDT
Created attachment 593403 [details]
patch for sed line, libs version

This is to remove the sed lines from the .spec file. This removes the sed lines from the /libs version
Comment 19 Jørn Lomax 2012-06-21 05:54:36 EDT
I created the patches by running the sed lines on the Makefiles on the commandline, and modifying it so that it would take OPTFLAGS as an arguemnt. It doesn't build though, i get the following error

> g++: error: unrecognized command line option '-fasynchronous-unwind-tables -j4'
I can't for the life of me see what is causing this error
Comment 21 Brendan Jones 2012-06-21 07:09:54 EDT
Try this:

%build
export OPTFLAGS="%{optflags}"
make PREFIX=%{_prefix} LIBDIR=%{_libdir} %{?_smp_mflags} -C libs
make PREFIX=%{_prefix} LIBDIR=%{_libdir} %{?_smp_mflags} -C apps
Comment 22 Jørn Lomax 2012-06-21 07:29:51 EDT
Here is (hopefully) the final update
spec: http://jvlomax.fedorapeople.org/packeging/zita-alsa-pcmi.spec
srpm: http://jvlomax.fedorapeople.org/packeging/zita-alsa-pcmi-0.2.0-6.fc17.src.rpm


rpmlint->.spec:
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

rpmlint->SRPM:
zita-alsa-pcmi.src: W: spelling-error %description -l en_US clalsadrv -> cloistral, clustered, clerestory
zita-alsa-pcmi.src: W: spelling-error %description -l en_US initialise -> initialize, initials, initial's
zita-alsa-pcmi.src: W: spelling-error %description -l en_US hw -> Haw, He, haw
zita-alsa-pcmi.src: W: spelling-error %description -l en_US mmap -> map, amp, mam
1 packages and 0 specfiles checked; 0 errors, 4 warnings.
Comment 23 Orcan Ogetbil 2012-06-21 20:46:22 EDT
Thank you for the update. I did a full review on this:

! rpmlint says
   zita-alsa-pcmi.x86_64: W: spelling-error %description -l en_US clalsadrv -> clausal
   zita-alsa-pcmi.x86_64: W: spelling-error %description -l en_US initialise -> initialize, initial, inessential
   zita-alsa-pcmi.x86_64: W: spelling-error %description -l en_US hw -> he, h, w
   zita-alsa-pcmi.x86_64: W: spelling-error %description -l en_US mmap -> map, m map, mamma
   zita-alsa-pcmi.src: W: spelling-error %description -l en_US clalsadrv -> clausal
   zita-alsa-pcmi.src: W: spelling-error %description -l en_US initialise -> initialize, initial, inessential
   zita-alsa-pcmi.src: W: spelling-error %description -l en_US hw -> he, h, w
   zita-alsa-pcmi.src: W: spelling-error %description -l en_US mmap -> map, m map, mamma
Let us ignore the above.
   zita-alsa-pcmi-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/zita-alsa-pcmi-0.2.0/apps/mtdm.cc
   zita-alsa-pcmi-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/zita-alsa-pcmi-0.2.0/apps/alsa_loopback.cc
   zita-alsa-pcmi-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/zita-alsa-pcmi-0.2.0/apps/alsa_delay.cc
   zita-alsa-pcmi-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/zita-alsa-pcmi-0.2.0/apps/mtdm.h
   zita-alsa-pcmi-utils.x86_64: W: no-documentation
   zita-alsa-pcmi-utils.x86_64: W: no-manual-page-for-binary alsa_loopback
   zita-alsa-pcmi-utils.x86_64: W: no-manual-page-for-binary alsa_delay
   zita-alsa-pcmi-devel.x86_64: W: no-documentation
It would be good to contact upstream to do something for the above.

! For both of the patches you can do something like
-CXXFLAGS += -O2 -Wall -MMD -MP
+CXXFLAGS += -O2 -Wall -MMD -MP -I../libs $(OPTFLAGS)
so that you don't remove the upstream optimization flags when no OPTFLAGS was specified. Your OPTFLAGS will override whatever there was initially.

Please submit your patches upstream and leave a comment in your specfile about the patch status. A link to upstream bugtracker or mailing list archive would be nice.


* The utils subpackge should have license GPLv2+ and GPLv3+. Please take a look at the source code under apps/*. Also please indicate this in the specfile as a comment (which files are GPLv2+, which are GPLv3+ etc.).

* The devel package MUST require alsa-lib-devel. See libs/zita-alsa-pcmi.h

* Please replace %{?smp_mflags} with %{?_smp_mflags}
Comment 24 Jørn Lomax 2012-06-22 08:04:10 EDT
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.

Doesn't that mean that it's possible to release it under GPLv3 at our option?

There is no bugtracker or mailinglist for upstream as far as i know. I have contacted upstream about the patches and documentation.

I will submit the updated package once i receive an answer from upstream
Comment 25 Orcan Ogetbil 2012-06-23 14:43:43 EDT
(In reply to comment #24)
> 
> Doesn't that mean that it's possible to release it under GPLv3 at our option?

Yes, there is no licensing conflict as GPLv2+ software is a superclass of GPLv3+ software by definition. In the case that a software package contains code from multiple (and compatible) licenses, these licenses need to be listed in the License tag. Please see [1] for the relevnt guideline and an example, and [2] for a real-life example. Note that the examples give detailed explanation about the licenses of individual source files.

> 
> There is no bugtracker or mailinglist for upstream as far as i know. I have
> contacted upstream about the patches and documentation.
> 

Thanks, please indicate this in the specfile as a comment. See [3] for the relevant guideline.


[1] http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios

[2] http://pkgs.fedoraproject.org/gitweb/?p=dpkg.git;a=blob;f=dpkg.spec;h=70e29fc42ad

[3] http://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment
Comment 26 Jørn Lomax 2012-06-25 07:50:32 EDT
SPEC: http://jvlomax.fedorapeople.org/packaging/zita-alsa-pcmi.spec
SRPM: http://jvlomax.fedorapeople.org/packaging/zita-alsa-pcmi-0.2.0-7.fc17.src.rpm

[makerpm@Fafnir SPECS]$ rpmlint zita-alsa-pcmi
zita-alsa-pcmi.i686: W: spelling-error %description -l en_US clalsadrv -> cloistral, clustered, clerestory
zita-alsa-pcmi.i686: W: spelling-error %description -l en_US initialise -> initialize, initials, initial's
zita-alsa-pcmi.i686: W: spelling-error %description -l en_US hw -> Haw, He, haw
zita-alsa-pcmi.i686: W: spelling-error %description -l en_US mmap -> map, amp, mam
zita-alsa-pcmi.i686: W: unused-direct-shlib-dependency /usr/lib/libzita-alsa-pcmi.so.0.2.0 /lib/libstdc++.so.6
zita-alsa-pcmi.i686: W: unused-direct-shlib-dependency /usr/lib/libzita-alsa-pcmi.so.0.2.0 /lib/libm.so.6
zita-alsa-pcmi.i686: W: unused-direct-shlib-dependency /usr/lib/libzita-alsa-pcmi.so.0.2.0 /lib/libgcc_s.so.1
1 packages and 0 specfiles checked; 0 errors, 7 warnings.

I'm guessing the above is from inlcuding alsa-devel as a requirement for the devel pacakge?

[makerpm@Fafnir SPECS]$ rpmlint ../SRPMS/zita-alsa-pcmi-0.2.0-7.fc17.src.rpm 
zita-alsa-pcmi.src: W: spelling-error %description -l en_US clalsadrv -> cloistral, clustered, clerestory
zita-alsa-pcmi.src: W: spelling-error %description -l en_US hw -> Haw, He, haw
zita-alsa-pcmi.src: W: spelling-error %description -l en_US mmap -> map, amp, mam
1 packages and 0 specfiles checked; 0 errors, 3 warnings.
Comment 27 Jørn Lomax 2012-06-25 08:17:20 EDT
Another informal review created: https://bugzilla.redhat.com/show_bug.cgi?id=835028
Comment 28 Orcan Ogetbil 2012-06-25 20:40:33 EDT
(In reply to comment #26)
> 
> I'm guessing the above is from inlcuding alsa-devel as a requirement for the
> devel pacakge?
> 

I don't think it is for that reason. Those are standard libraries and their linkage is automatic, e.g. you don't need to specify a -lstdc++ during linking. Possibly a bug with the linker? Anyhow this is a problem with the package.

We are almost there. The License: GPLv2+ and GPLv3+ tag shall be for the utils subpackage. Since no GPLv2+ code goes to rest, the other packages are pure GPLv3+.

I will take a look at your other packages and informal reviews next. Do you use the same email address at FAS?
Comment 29 Jørn Lomax 2012-06-26 06:42:38 EDT
SPEC:http://jvlomax.fedorapeople.org/packaging/zita-alsa-pcmi-0.2.0-8.fc17.src.rpm
SRPM: http://jvlomax.fedorapeople.org/packaging/zita-alsa-pcmi-0.2.0-8.fc17.src.rpm

rpmlint output from specfile:
zita-alsa-pcmi.i686: W: spelling-error %description -l en_US clalsadrv -> cloistral, clustered, clerestory
zita-alsa-pcmi.i686: W: spelling-error %description -l en_US initialise -> initialize, initials, initial's
zita-alsa-pcmi.i686: W: spelling-error %description -l en_US hw -> Haw, He, haw
zita-alsa-pcmi.i686: W: spelling-error %description -l en_US mmap -> map, amp, mam
zita-alsa-pcmi.i686: W: unused-direct-shlib-dependency /usr/lib/libzita-alsa-pcmi.so.0.2.0 /lib/libstdc++.so.6
zita-alsa-pcmi.i686: W: unused-direct-shlib-dependency /usr/lib/libzita-alsa-pcmi.so.0.2.0 /lib/libm.so.6
zita-alsa-pcmi.i686: W: unused-direct-shlib-dependency /usr/lib/libzita-alsa-pcmi.so.0.2.0 /lib/libgcc_s.so.1
1 packages and 0 specfiles checked; 0 errors, 7 warnings.

rpmlint output from SRPM:zita-alsa-pcmi.src: W: spelling-error %description -l en_US clalsadrv -> cloistral, clustered, clerestory
zita-alsa-pcmi.src: W: spelling-error %description -l en_US hw -> Haw, He, haw
zita-alsa-pcmi.src: W: spelling-error %description -l en_US mmap -> map, amp, mam
Comment 30 Jørn Lomax 2012-06-26 06:47:15 EDT
and yes, i use the same email address everywhere :)
Comment 31 Orcan Ogetbil 2012-06-29 01:42:15 EDT
I think this package is good to go. You have shown us so far that you can follow the reviewer's feedback and the packaging guidelines. Besides this package, you also submitted another one in bug #834239 (a mono package that has a different structure than this one) which is in good shape. You made a few informal package rewiews, but trust me, more won't hurt. Actually, following the review guidelines [1] bullet to bullet teaches a lot.

I am sponsoring you now. Welcome to Fedora. Next, you can file an SCM request [2] and then import your files and build the official package by following [3].

Furthermore, I encourage you to finish the informal reviews you started, unless they were taken over by someone else. As a packager, you are allowed to maintain and co-maintain Fedora packages. 


If you have any questions at any point, please do not hesitate to contact me.


-------------------------------------------------
This package (zita-alsa-pcmi) is APPROVED by oget
-------------------------------------------------


[1] http://fedoraproject.org/wiki/Packaging:ReviewGuidelines
[2] http://fedoraproject.org/wiki/Package_SCM_admin_requests
[3]http://fedoraproject.org/wiki/PackageMaintainers/Join#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner
Comment 32 Jørn Lomax 2012-06-30 08:06:01 EDT
New Package SCM Request
=======================
Package Name: zita-alsa-pcmi
Short Description: alsa pcm libraries
Owners: jvlomax
Branches: f16 f17
InitialCC:
Comment 33 Jon Ciesla 2012-07-01 18:43:02 EDT
Git done (by process-git-requests).

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