Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 209311
Summary: | Review Request: espeak - Software speech synthesizer (text-to-speech) | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Francois Aucamp <francois.aucamp> |
Component: | Package Review | Assignee: | Mamoru TASAKA <mtasaka> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | michel.salim, mtasaka |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-11-14 18:06:48 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Francois Aucamp
2006-10-04 15:14:53 UTC
I cannot browse your spec file or download your srpm as connection gets timeout. Ping? Sounds like an interesting package, but as Mamoru noted, we can't get to it Hmm... it seems the person had who graciously provided me with some server space is having some router problems... :-/ I have moved to the spec and SRPM to where I put the flite (#207793) stuff: Spec URL: http://dialogpalette.sourceforge.net/extras/fedora/espeak.spec SRPM URL: http://dialogpalette.sourceforge.net/extras/fedora/espeak-1.16-1.src.rpm Well, just a quick look at this package: A. From http://fedoraproject.org/wiki/Packaging/Guidelines : * Filesystem Layout (and others) - I think installing header files into /usr/include should be avoided, they should be installed under /usr/include/espeak. - By the way, why do you install only 'speak_lib.h'? I can find other header files. - http://espeak.sourceforge.net/index.html ----------------------------------------------------- The project name speak had already been taken by another project on SourceForge (for a Windows TTS front-end) so I added a letter 'e' to the front to make eSpeak. For now, the program executable remains speak and is referred to as such in the documentation. ----------------------------------------------------- I think the binary name 'speak' is somewhat troublesome and recommend that the name should be 'espeak' (and the related documentation should be changed). - Check if /usr/share/espeak-data/soundicons/ is required as this is a empty directory. * BuildRequires: - By the way, is 'portaudio' requires for BuildRequires? * Compiler flags - Fedora specific compiler flags are not passed and this leads to useless debuginfo rpm. B. From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines : = Perhaps nothing, however, I just did a quick look... C. Other things I have noticed. * Installation process. - Well, I think the installation process of this package is somewhat illegible. Especially, the need of explicit description of soname in spec file should be avoided as you have to check if soname of shared library is changed each time you build this. This should be something like: -------------------------------------------------------- cd src install -m 0755 libespeak.so.*.*.* %{buildroot}%{_libdir} ln -sf libespeak.so.*.*.* %{buildroot}%{_libdir}/libespeak.so /sbin/ldconfig -n %{buildroot}%{_libdir}/ -------------------------------------------------------- * Voice data - Well, I cannot verify the license issue of binary voice data named *_dict as this is a binary. If these data can be reproduced from ascii text files, it should do so. By a quick look, 'speak' has a option of '--compile=?'. Is it possible to recompile voice data *dict files by this? (Note: executing 'speak' binary requires 'portaudio' on BuildRequires. Also, by default 'speak --compile=?' needs superuser admin as it tries to access /usr/share/espeak so a patch is required to make 'speak' binary have the argument of output files) Just adding a few things: - portaudio is a BuildRequire (see Makefile), so that's fine. It's a bit confusing that there is no portaudio-devel - Might want to create an 'install' target in the Makefile and do the copy operations there. This way you can push the changes upstream. Probably put the RPM_OPT_FLAGS changes in a separate diff since that's Fedora-specific - Tested on x86_64 Has anyone sponsored you yet? I can't do it myself, but you might want to mention it on the mailing list Well, I am a membership of sponsors and I can sponsor Francois if I can judge it is preferable. "portaudio-devel" is a virtual sub-package of "portaudio", so you can do "BuildRequires: portaudio-devel" just fine and ought to prefer doing it. Thanks for the feedback! New build: Spec URL: http://dialogpalette.sourceforge.net/extras/fedora/espeak.spec SRPM URL: http://dialogpalette.sourceforge.net/extras/fedora/espeak-1.16-2.src.rpm Changes: - Added "install" target to makefile (makefile_install_target.patch) - Added patch to fix AMD64 sizeof(char *) assumption bug (upstream request ID 1588938) - Changed "portaudio" BuildRequires to "portaudio-devel" - Added patch to makefile to allow RPM_OPT_FLAGS - Added patch to replace all references to "speak" binary with "espeak" - Moved header files to /usr/include/espeak A few comments: Development headers: As mentioned in the ReadMe, the speak_lib.h provides the entire API to the libespeak shared library, and it references no other espeak-specific headers, so it is unecessary to include any other .h files. Binary voice data: The espeak program itself (formerly "speak" ;-) ) cannot compile the binary voice data (using the --compile arg) from source without a binary version of the phoneme tables being present. These phoneme table data files cannot be compiled from source using espeak/speak; they are compiled with a seperate program, "espeakedit", which is an interactive, GUI-based editing tool, also released under the GPL. There is no explicit license file for the binary voice data/phoneme tables, but since the source from which these are compiled is under the GPL, I don't think there are any legal problems. Patches: Depending on the feedback from this package build, I will push the makefile patches upstream (except for the RPM_OPT_FLAGS patch). espeak name: I agree that the "speak" name is troublesome, and have removed it from this rpm, as per suggestion. However, we must remember that some other applications may already be using eSpeak via the "speak" executable (especially since the shared library is a relatively new addition to espeak); this patch may break compatability with such programs. Some HOWTO's and guides on the Internet will also be (very slightly) incompatible with this naming scheme. There are ways around this, naturally, but I'm uncertain whether changing the name in the Fedora package is the best course of action. Nevertheless, depending on the feedback here, I will push upstream for the name change... :-) I've built this package in mock on FC6/i386. rpmlint is silent, except for the no-documentation stuff for the -devel subpackage. Well, a lot of improvements!! The left things are: 1. From http://fedoraproject.org/wiki/Packaging/Guidelines : * Use rpmlint - Now -debuginfo rpm is correctly created, however, this package (-debuginfo) bears lots of complaint, e.g. E: espeak-debuginfo script-without-shebang \ /usr/src/debug/espeak-1.16-source/src/sintab.h This is due to incorrect permission. Please change this to 0644. 2. From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines : (= Nothing). 3. Other things I have noticed : * Well, I recommend changing suffix for different patches. * It seems that makefile_rpmoptflags.patch is not necessary, which can be replaced with: make %{?_smp_mflags} BIN_NAME=espeak CXXFLAGS=${RPM_OPT_FLAGS} * The original files .orig files e.g. /usr/share/doc/espeak-1.16/html/add_language.html.orig are not necessary. = Well, * the license issue of binary voice data * file list of -devel package are okay. -------------------------------------------------------------- Please fix above. After that, I will accept this package and sponsor you. New build: Spec URL: http://dialogpalette.sourceforge.net/extras/fedora/espeak.spec SRPM URL: http://dialogpalette.sourceforge.net/extras/fedora/espeak-1.16-3.src.rpm Changes: - Fixed source file permissions for -debuginfo package in %prep - Added RPM_OPT_FLAGS to "make" command in %build; removed RPM_OPT_FLAGS makefile patch - Modified makefile install target patch to include general support for setting compiler optimization flags via CXXFLAGS - Removed creation of .orig backup files during patching - Modified patch files to have different suffixes This time, rpmlint is truly silent ;-) I'm not too sure what you mean by "different suffixes" for different patch files; I've resorted to using a suffix corresponding to the patch ID number in the spec (e.g. somefile.patch0). Thanks for the feedback! Well, what I meant was adding some different suffixes each time you apply patches are preferred and I didn't meant you have to change the name of the patches. Like: %patch0 -p1 -b .pthread %patch1 -p1 -b .install_target %patch2 -p1 -b .sizeof ........ to make it easy that you can check how the patches were applied afterwards. (I have not yet rebuilt the srpm you re-uploaded Well, it seems that I cannot access http://dialogpalette.sourceforge.net/. Note: I will be busy till this Thursday (in Japan: EST+14h) and I may not be able to check your srpm by that day. Thanks for the suffix clarification. :-) I've bumped the release, and did a few minor fixes: New build: Spec URL: http://dialogpalette.sourceforge.net/extras/fedora/espeak.spec SRPM URL: http://dialogpalette.sourceforge.net/extras/fedora/espeak-1.16-4.src.rpm Changes: - Modified patch steps to create backups with different suffixes - Renamed patch file extensions to .patch - Added step in %install to remove patch backup files in documentation Final check. Okay. -------------------------------------------------------------- This package (espeak) is APPROVED by me. -------------------------------------------------------------- Please go forward according to http://fedoraproject.org/wiki/Extras/Contributors to import this package to Fedora Extras. I will sponsor you when you have taken steps partway written in the page above (then I should receive the mail that you need a sponsor around 6h EST). Thanks for the review and sponsorship! I have imported the package and requested branches for FC-5 and FC-6. Okay, I saw by mail that you successfully imported espeak to FE-devel CVS branch. Well, usually SyncNeeded process may take some time (one or two day) to be completed by the admin of CVS. Before waiting it (perhaps now), you should be able to send a build queue for FE-devel. Try it accroding to http://fedoraproject.org/wiki/Extras/Contributors . When you successfully built espeak on FE-devel, you can close this package as "CLOSED NEXTRELEASE" (some people suspend to close the review request till rebuild for all branches are done, however I think that you can close this when * rebuild for FE-devel is done * SyncNeeded is requested for other branches ) ------------------------------------------------ NOTE: please don't forget to add espeak to comps/comps-fe?.xml.in when building espeak is done, see: http://fedoraproject.org/wiki/Extras/CompsXml and... please also check my review for flite (bug 207793). By the way, have you tried to rebuild this package on FE buildsys? Yes, but the plague-client build request itself is denied with the following error: "Server returned an error: Insufficient privileges.". All of by client-side setup seems to be fine, so following advice from fedora-maintainers, I have requested assistance from the Fedora Infrastructure team (created a ticket in their bugtracker), and am now waiting for a response. Ok, build for FE-devel succeeded; FC-5 and FC-6 building in progress. Closing review request. Thanks to all for helping out during the review! |