I'm a google summer of code student working on the fedora audio spin

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.

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.

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.

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

Fixed

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.

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.

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

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

(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

That makes more sense :D Fixed it now, and no rpmlint warning anymore. Thanks

(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

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

I'll do the review, but package does not build
   http://koji.fedoraproject.org/koji/taskinfo?taskID=4169293

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.

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

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

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

Updated spec:http://jvlomax.fedorapeople.org/packeging/zita-alsa-pcmi.spec
updated SRPM: http://jvlomax.fedorapeople.org/packeging/zita-alsa-pcmi-0.2.0-5.fc17.src.rpm

Try this:

%build
export OPTFLAGS="%{optflags}"
make PREFIX=%{_prefix} LIBDIR=%{_libdir} %{?_smp_mflags} -C libs
make PREFIX=%{_prefix} LIBDIR=%{_libdir} %{?_smp_mflags} -C apps

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.

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}

> 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

(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

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.

Another informal review created: https://bugzilla.redhat.com/show_bug.cgi?id=835028

(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?

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

and yes, i use the same email address everywhere :)

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

New Package SCM Request
=======================
Package Name: zita-alsa-pcmi
Short Description: alsa pcm libraries
Owners: jvlomax
Branches: f16 f17
InitialCC:

Git done (by process-git-requests).