The flite 1.3 patch here...

http://homepage.hispeed.ch/loehrer/flite_alsa.html

...adds ALSA support.  It seems to apply and build.  Should you add it?

(In reply to comment #1)
> The flite 1.3 patch here...
> 
> http://homepage.hispeed.ch/loehrer/flite_alsa.html
> 
> ...adds ALSA support.  It seems to apply and build.  Should you add it?
> 
> 
Thank you for the ALSA patch! I will apply it, build, and update the 
spec/SRPM.
By the way, you mention on your site that you were unable to build shared 
libraries from Flite 1.3 - the SRPM above includes a patch that fixes this 
issue.
You can also download the patch here:
http://dialogpalette.sourceforge.net/extras/makefile_shared_objects.patch
Cheers,
-Francois

Ok, rebuilt package with the ALSA patch included:
Spec URL: http://dialogpalette.sourceforge.net/extras/fedora/flite.spec
SRPM URL: http://dialogpalette.sourceforge.net/extras/fedora/flite-1.3-2.src.rpm

Package built clean in mock on i386, FC5. No rpmlint output.

> By the way, you mention on your site that you were unable to build shared 
> libraries from Flite 1.3 - the SRPM above includes a patch that fixes this 
> issue.
Oops, didn't realize it's not your site... ;-)

Please don't package static libraries.  See...
http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7

Also, since the ALSA patch is non-trivial, I recommend creating and installing a
README-ALSA.txt file or something like that to credit the author and point to
the patch's web page.

I'll do a review once these are taken care of, but since I'm not a sponsor we'll
need somebody else to jump in.  Thanks for submitting this, BTW.

(In reply to comment #4)
> Please don't package static libraries.  See...
> 
I had been wondering about this, actually :-). 

Since Flite is largely aimed (by its original developers, anyhow) at embedded 
systems such as augmentative devices, it is not uncommon to link to the static 
flite libs (this is also probably why the "vanilla" upstream flite 1.3 does 
not create shared libraries)

Personally I agree that static libs are usually not too useful (hence the 
separate flite-devel-static subpackage). The Fedora Packaging Guidelines makes 
provision for exceptions to the static libs-rule; does this package qualify?

Anyhow, will implement suggestions, update and rebuild. Thanks for the 
feedback!

(In reply to comment #5)
> Personally I agree that static libs are usually not too useful (hence the 
> separate flite-devel-static subpackage). The Fedora Packaging Guidelines makes 
> provision for exceptions to the static libs-rule; does this package qualify?

I don't think so.  Shared libraries have the advantage that bug/security fix
updates are automatically made available to users of those shared libraries. 
Static linking hides these dependencies.  I can't think of any exceptional
reason to override this thinking for flite.

New build:
Spec URL: http://dialogpalette.sourceforge.net/extras/fedora/flite.spec
SRPM URL: 
http://dialogpalette.sourceforge.net/extras/fedora/flite-1.3-3.src.rpm

Changes:
- Added README-ALSA.txt
- Removed subpackage: flite-devel-static
- Modified shared libraries patch to prevent building static libraries 

You need a sponsor.  If it were up to me, I would approve this. 

* source files match upstream:
ae0aca1cb7b4801f4372f3a75a9e52b5  flite-1.3-release.tar.gz
* package meets naming and packaging guidelines
* specfile is properly named, is cleanly written and uses macros consistently
* dist tag is present
* build root is correct
* license field matches the actual license
* license is open source-compatible, license text included in package
* latest version is being packaged
* BuildRequires are proper
* compiler flags are appropriate
* %clean is present
* package builds in mock (FC-5, i386)
* package installs properly
* rpmlint is silent
* final provides and requires are sane
* package is not relocatable
* owns the directories it creates
* doesn't own any directories it shouldn't
* no duplicates in %files
* file permissions are appropriate
* scriptlets are good
* code, not content
* documentation is small, so no -docs subpackage is necessary
* %docs are not necessary for the proper functioning of the package
* no libtool .la droppings

(In reply to comment #8)
> * package builds in mock (FC-5, i386)

It doesn't build on x86_64 and this is a blocker.
But it can be simple fixed by removing %{?_smp_mflags} from `make`.

(In reply to comment #9)
> It doesn't build on x86_64 and this is a blocker.
> But it can be simple fixed by removing %{?_smp_mflags} from `make`.
Done. :-)

New package:
Spec URL: http://dialogpalette.sourceforge.net/extras/fedora/flite.spec
SRPM URL: 
http://dialogpalette.sourceforge.net/extras/fedora/flite-1.3-4.src.rpm

Changes:
- Removed "_smp_flags" macro from "build" for x86_64 arch

I've successfully rebuilt it on an FC5, x86_64 and FC5, i386.

I am not sure if you should remove _smp_flags only for x86_64 arch or remove
it completely. We don't if it causes error on e.g. ppc.
But it's only my suggestion, you don't have to comply with it.

(In reply to comment #11)
> I am not sure if you should remove _smp_flags only for x86_64 arch or remove
> it completely. We don't if it causes error on e.g. ppc.
> But it's only my suggestion, you don't have to comply with it.

I agree. However, before I do this, could anyone please test this package on a
ppc arch? (sadly I don't have any lying around :-( ) 
Thanks for the comments!

Ok, no-one's tested this on ppc yet, so I'm opting for the safer build route.

New package:
Spec URL: http://dialogpalette.sourceforge.net/extras/fedora/flite.spec
SRPM URL: 
http://dialogpalette.sourceforge.net/extras/fedora/flite-1.3-5.src.rpm

Changes:
- Modified "build" so that "_smp_flags" is only used for i386 arch

Francois, I see you reported two speech syntesis applications and you're
still looking for a sponsor. I don't read every review and I don't what about
your activity on another requests.
Do as many reviews as you can, then you'll find your sponsor faster :) Also,
you may make another high-quality packages and post request for them. I hope
you'll be success :)

Assigning to me.

Created attachment 140079 [details]
Patch to try to make parallel

Well, as far as I noticed by now...

1. 
parallel make is broken in i386, also. I sometimes fails
parallel make -j3 on i386.
This is because config/common_make_rules is broken. This file
does not check if needed object files are already built before
trying to make static ar file.
Please check if my patch attached fixes the problem.

2.
Well, you try to provide shared libraries which upstream doesn't
by attaching a patch, do you? I have not yet checked the patch, however
please make the following sure:

2-1. 
Please explain why the	soname version '1.3' is proper. Are there
any assurance that the API of the libraries won't change during 1.X version?
2-2
The shared libraries are 'partially' broken. 'Partially' means that
libraries in flite includes undefined non-weak symbols.
--------------------------------------------------------------------------
W: flite undefined-non-weak-symbol /usr/lib/libflite_cmu_time_awb.so.1.3
clunits_synth
W: flite undefined-non-weak-symbol /usr/lib/libflite_cmu_time_awb.so.1.3
cmu_syl_boundary
W: flite undefined-non-weak-symbol /usr/lib/libflite_cmu_time_awb.so.1.3
lexicon_val
W: flite undefined-non-weak-symbol /usr/lib/libflite_cmu_time_awb.so.1.3
feat_set_int
W: flite undefined-non-weak-symbol /usr/lib/libflite_cmu_time_awb.so.1.3
feat_set
W: flite undefined-non-weak-symbol /usr/lib/libflite_cmu_time_awb.so.1.3
usenglish_init
..........
(too many, cannot write all of them)
--------------------------------------------------------------------------------

You can check these by 'ldd -r /usr/lib/libflite.so.1.3', for example.

If these libraries are used only by binaries in flite rpm, this may
be ignored for a moment (some reviewers disagree).
However, it seems that you want to provide -devel package, for which
case, this problem should be fixed because undefined non-weak symbols
disables correct linkage.

3.
I suggest that when applying patches the suffixes should be changed
for different patches.

Another issues:

4. 
The binaries /usr/bin/flite and /usr/bin/flite_time do not use
the shared libraries included in flite main rpm (You can check
it by:
[tasaka1@localhost i386]$ ldd -r /usr/bin/flite
        linux-gate.so.1 =>  (0x003d5000)
        libm.so.6 => /lib/libm.so.6 (0x002c8000)
        libasound.so.2 => /lib/libasound.so.2 (0x07d7b000)
        libc.so.6 => /lib/libc.so.6 (0x00189000)
        /lib/ld-linux.so.2 (0x0016c000)
        libdl.so.2 => /lib/libdl.so.2 (0x002f1000)
) I wonder if shared libraries and -devel packages are really required......    

Removing NEEDSPONSOR blocker, as I am sponsoring (bug 209311).

I have started re-writing the shared libraries patch to fix the "undefined
symbol"-issues, and to make the flite binary link to its own shared libraries
(thanks for pointing this out). The idea here is to make the patch as clear as
possible (unlike the current sharedlibs patch), for easier maintainability.

IMHO, the shared libraries and -devel packages are very valuable, as flite
actually has quite a nice API for embedding text-to-speech in other
applications. :-)

Will probably publish a new build later today.

New build:
Spec URL: http://dialogpalette.sourceforge.net/extras/fedora/flite.spec
SRPM URL: 
http://dialogpalette.sourceforge.net/extras/fedora/flite-1.3-6.src.rpm

Changes:
- Recreated patch to allow shared libraries to build correctly (sharedlibs.patch)
- "flite" and "flite_time" binaries now link to flite shared libraries
(sharedlibs.patch)
- Simplified the documentation patch filename
- Modified patch steps in %prep to create backup files with different suffixes
- Removed "_smp_flags" macro from %build for all archs

The new shared libraries patch also adds pretty much the same functionality as
the suggested parallel build patch (attachment id=140079); sadly, parallel
builds still fails sometimes :-(, so I have removed "_smp_flags" from %build
completely.

I have tested the shared libraries with ldd -r and they're all fine now. 
rpmlint is silent, except for a "no documentation" warning for flite-devel.

Well, almost okay.

1. From http://fedoraproject.org/wiki/Packaging/Guidelines :
* Filesystem Layout
  - I think all header files in -devel package should be
    moved to %{_includedir}/%{name} (then %{_includedir}/%{name}
    should be owned).

* Timestamps
  cp %{SOURCE1} .
  Use 'cp -p' to keep timestamps.

2. From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines :
   = Nothing.

3. Other things I have noticed:
* Patches:
  - Well, the name of the patch and the suffix when applying 
    it are inconsistent.....

  - I don't think patching to configure (not configure.in) is
    preferable. Would you rewrite sharedlibs.patch and spec
    file so that the patch is applied for configure.in and 
    then 'autoconf' is called (this requires 'autoconf' for
    BuildRequires)?

  - Note: usually flite-1.3-*****.patch is preferable for the
    names of the patches.

* parallel make
  - Please add some comments to spec file that "this package
    fails parallel make".

(In reply to comment #21)

>   - I don't think patching to configure (not configure.in) is
>     preferable. Would you rewrite sharedlibs.patch and spec
>     file so that the patch is applied for configure.in and 
>     then 'autoconf' is called (this requires 'autoconf' for
>     BuildRequires)?

Actually I think that the reverse is true. Avoiding to rerun
the autotools is better, so patching configure is better. It may be
right to patch both configure and configure.in especially if the
configure.in is interesting for submission upstream. Sometimes
the patch is too important, so patching configure is necessary.
Here the configure patch has allready been done with newer autoconf
than what came with flite, but I think that we shouldd refrain from 
using an even newer version of autotools and the patch should be 
kept as is.

> Actually I think that the reverse is true. Avoiding to rerun
> the autotools is better, so patching configure is better. It may be
> right to patch both configure and configure.in especially if the
> configure.in is interesting for submission upstream. Sometimes
> the patch is too important, so patching configure is necessary.

Sorry, here it should have read

"Sometimes the patch is too important, so patching configure.in and
rerunning the autotools is necessary."

> Here the configure patch has allready been done with newer autoconf
> than what came with flite, but I think that we shouldd refrain from 
> using an even newer version of autotools and the patch should be 
> kept as is.

I think for this package patching to configure.in is more
preferable because:
* as far as I read the patch, the diff for configure is too large
  so that I cannot figure out what is actually changed....., while
* diff for configure.in (also in the patch) is very small and
* the diff for configure are between
  * one is original configure make from configure.in with autoconf 2.57
  * the other is modified configure which _seems_ to be generated by
    autoconf 2.13 (why?)

(In reply to comment #24)
> I think for this package patching to configure.in is more
> preferable because:
> * as far as I read the patch, the diff for configure is too large
>   so that I cannot figure out what is actually changed....., while
> * diff for configure.in (also in the patch) is very small and
> * the diff for configure are between
>   * one is original configure make from configure.in with autoconf 2.57
>   * the other is modified configure which _seems_ to be generated by
>     autoconf 2.13 (why?)

Ah ok, I got it wrong, I thought the 2.13 one was from upstream. In that
case it is indeed better to rerun autoconf since 2.59 is much more
similar to 2.57 than 2.13 is. Sorry for the noise...

(In reply to comment #24)
> I think for this package patching to configure.in is more
> preferable because:
> * as far as I read the patch, the diff for configure is too large
>   so that I cannot figure out what is actually changed....., while
> * diff for configure.in (also in the patch) is very small and
> * the diff for configure are between
>   * one is original configure make from configure.in with autoconf 2.57
>   * the other is modified configure which _seems_ to be generated by
>     autoconf 2.13 (why?)

I actually got this patch from a third party, Lukas Loehrer (hence the old
autoconf), see comment #1 on this bug report. This is also the reason why I
didn't use cp -p on Source1 (as it was created by me, and is not used for
anything in the package). During the rewrite of the sharelibs patch, I actually
started doing a different ALSA patch (based on the ALSA support for Flite 1.2),
which only requires MINOR modification to configure.in and the addition of a
modified src/audio/au_alsa.c (from flite 1.2) - because maintaining the current
patch is a bit of a hassle, should things change.

However, I could not get this new patch to build cleanly before today (one of
the flite debug tools complained about undefined symbols, so I must have missed
something somewhere); I will look into it again.

As for the other points: silly mistakes :-), sorry. Will fix as soon as
possible. Thanks again for the feedback.

Ok, I modified the alsa patch; it now only patches configure.in, and calls
autoconf during %build:
Spec URL: http://dialogpalette.sourceforge.net/extras/fedora/flite.spec
SRPM URL: http://dialogpalette.sourceforge.net/extras/fedora/flite-1.3-7.src.rpm

Changes:
- Modified alsa support patch file to patch "configure.in" instead of "configure"
- Added "autoconf" step to %build
- Added BuildRequires: autoconf
- Fixed patch backup file suffixes
- Renamed patch files to a more standard format
- Moved header files from /usr/include to /usr/include/flite
- Added -p option to all cp operations (to preserve timestamps)

Okay!!

--------------------------------------------------
 This package (flite) is APPROVED by me.

Thanks for the review.
Will just add the "package fails parallel make" comment which I forgot, bump the
release, import the package and queue a build.

Package built successfully for FE-devel, FC-5 and FC-6 branches requested.
Closing review request.

Package Change Request
======================
Package Name: flite
New Branches: EL-4 EL-5
Owners: peter faucamp


Note: I already asked Francois about EPEL-branches, and he hasn't any objections.

cvs done.