Spec URL: http://dialogpalette.sourceforge.net/extras/fedora/flite.spec SRPM URL: http://dialogpalette.sourceforge.net/extras/fedora/flite-1.3-1.src.rpm Description: Flite (festival-lite) is a small, fast run-time speech synthesis engine developed at CMU and primarily designed for small embedded machines and/or large servers. Flite is designed as an alternative synthesis engine to Festival for voices built using the FestVox suite of voice building tools. This is my first package, and I am looking for a sponsor.
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.