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 165686
Summary: | Review Request: Syck - YAML for Ruby, Python, PHP and OCaml | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Oliver Falk <oliver> | ||||||
Component: | Package Review | Assignee: | Toshio Kuratomi <toshio> | ||||||
Status: | CLOSED ERRATA | QA Contact: | David Lawrence <dkl> | ||||||
Severity: | medium | Docs Contact: | |||||||
Priority: | medium | ||||||||
Version: | rawhide | CC: | cstpierr, fedora-package-review, pahan | ||||||
Target Milestone: | --- | Flags: | kevin:
fedora-cvs+
|
||||||
Target Release: | --- | ||||||||
Hardware: | All | ||||||||
OS: | Linux | ||||||||
URL: | http://filelister.linux-kernel.at/mod_perl?current=/packages/FC_EXTRAS_APPROVAL/syck | ||||||||
Whiteboard: | |||||||||
Fixed In Version: | syck-0.55-10.el5 | Doc Type: | Bug Fix | ||||||
Doc Text: | Story Points: | --- | |||||||
Clone Of: | Environment: | ||||||||
Last Closed: | 2005-08-27 20:46:53 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, 165688 | ||||||||
Attachments: |
|
Description
Oliver Falk
2005-08-11 12:07:02 UTC
Not a full review: - Only the C library is currently being built. You have to do more to get the scripting language bindings built. When you do that you probably want to split the package up with one for each language binding. - The Ruby bindings are included in the FC4 Ruby (1.8.0 was the first version that should have it.) - On x86_64, make check for the main library seems to be having problems. YtsSpecificationExamples_27 seems to go on forever (10 minutes wallclock before I hit Ctrl-C.) - The spec file looks nothing like the fedora rpm spectemplate (available from fedora-rpmdevtools). Some things undoubtably don't matter but others have very obviously been left out (for instance the ldconfig call in %post/%postun). Running the package through the packaging guidelines http://www.fedoraproject.org/wiki/PackagingGuidelines or the guidelines "cheat sheet": http://www.fedoraproject.org/wiki/PackageReviewGuidelines would be a good idea so the spec doesn't leave reviewers trying to figure out where to start :-) - A definition of YAML in the %description would be a good idea. Otherwise people won't know what this package really does (After looking, at the yaml web page, it looks really nice. :-) (In reply to comment #2) > - A definition of YAML in the %description would be a good idea. Otherwise > people won't know what this package really does (After looking, at the yaml web > page, it looks really nice. :-) Will add this in the next release. (In reply to comment #1) > - Only the C library is currently being built. You have to do more to get the > scripting language bindings built. When you do that you probably want to split > the package up with one for each language binding. OK. Will do that. > - The Ruby bindings are included in the FC4 Ruby (1.8.0 was the first version > that should have it.) Which means, that I don't have to build it? Looking at the ruby 1.8.2 package, I cannot find the yaml extension... (But normally I don't have anything to do with ruby). > - On x86_64, make check for the main library seems to be having problems. > YtsSpecificationExamples_27 seems to go on forever (10 minutes wallclock before > I hit Ctrl-C.) Bad. But as I don't have a x86_64 around, it's hard to find out what's happening. :-/ > - The spec file looks nothing like the fedora rpm spectemplate (available from > fedora-rpmdevtools). Some things undoubtably don't matter but others have very > obviously been left out (for instance the ldconfig call in %post/%postun). You are correct, I'm a bad guy; I should have cleaned the spec before submitting it... > Running the package through the packaging guidelines > http://www.fedoraproject.org/wiki/PackagingGuidelines > or the guidelines "cheat sheet": > http://www.fedoraproject.org/wiki/PackageReviewGuidelines > > would be a good idea so the spec doesn't leave reviewers trying to figure out > where to start :-) Mea culpa. Please have a look at it now (Build 3). I have added subpackages for php and python. Did a spec-cleanup. And so on... Note: Re Comment #4 Ruby on FC4: [badger@katahdin syck-0.55-3]$ rpm -ql ruby-libs |grep syck /usr/lib64/ruby/1.8/x86_64-linux/syck.so /usr/lib64/ruby/1.8/yaml/syck.rb Good: - Naming matches upstream and folows the naming guidelines - spec file named appropriately - License is BSD - License included in package - Spec is in American English - Spec is comprehensible - Source matches upstream - No locale so no %find_lang - No Prefix Needswork: - License in spec should read BSD rather than GPL - Remove BR: /usr/bin/install (it comes from coreutils) - Remove BR: python (python-devel pulls it in) - For the python subpackage we need:: Requires: python-abi = %(%{__python} -c "import sys; print sys.version[:3]") - Leave out OCaml from the Summary as there's no OCaml extension. Also leave out Syck from the summary as it's redundant. - Leave out README.BYTECODE as you're not building that extension (or build and include it. Reading the docs seems to impy it's an extension to the syck library that will preparse a YAML document into bytecode that can then be re-loaded by the library. Could be reading that wrong, though.) - Shared libraries need to be left executable or they won't be stripped for debuginfo packages. - Static libraries and headers go in a -devel package. See below about creating a dynamic library for the extensions: - When making the library and loadable mdules, needs to supply RPM_OPT_FLAGS to the build process. - We can't BR: php-devel = ... [something that expands to the php-config command] because the comand isn't present until after the BR is fulfilled. If there's a specific version that's needed, use that, otherwise just use BR: php-devel. - Doesn't build the python or php extensions due to not having a libsyc compiled with -fPIC. (ie: we have a libsyck.a which was build without -fPIC. We need a libsyck.so that was built with -fPIC.) This problem has to go upstream to be fixed because with shared libraries comes stricter versioning requirements. However, hacking together some libtool based scripts is not incredibly hard. As long as upstream is amenable we could do the actual work here. - The php doesn't build due to the build process not finding either the syck library or header. Something like this should work: CFLAGS="$RPM_OPT_FLAGS -I../../lib" %configure --with-syck=../../ Minor: - Since the %post/%postun scripts contain just /sbin/ldconfig it would be possible to use %post -p /sbin/ldconfig and %postun -p /sbin/ldconfig. My understanding is this format allows rpm to do some optimizations (not loading a shell to invoke ldconfig and running ldconfig only once when a group of libraries are installed as one transaction.) - There's a slew of 64bit related warnings. However, I don't see anything that's obviously causing problems (such as the failure of the check on x86_64.) Summary ======= If you want to work with upstream about the failure of the regression test and whether libtool-based dynamic libraries are desirable I can get started on creating some libtool infrastructure. TODO ==== Since I couldn't get a completed package, these things have not been checked: - rpmlint - Check against complete PackagingGuidelines - Recheck BR in mock Created attachment 118099 [details]
Make syck build with libtool
Here's a patch to add libtool support to syck. It should make the C libsyck
and tests directory fine. I'm reasonably confident the python setup.py changes
will hold up under scrutiny but the php stuff is a bit of a hack. I haven't
touched the cocoa, ruby, or yamlbyte directories.
It's also possible the author has some idea of how to get this done better
since the ruby extension has to be built against shared libraries (so there's a
shard syck as part of that.)
FWIW, explicit "Requires: python-abi = ..." is not usually needed with FC4+, rpmbuild takes care of it automatically in the form of python(abi). (In reply to comment #8) > FWIW, explicit "Requires: python-abi = ..." is not usually needed with FC4+, > rpmbuild takes care of it automatically in the form of python(abi). OK, then I'm not going to add this... (In reply to comment #6) > Note: Re Comment #4 > Ruby on FC4: > [badger@katahdin syck-0.55-3]$ rpm -ql ruby-libs |grep syck > /usr/lib64/ruby/1.8/x86_64-linux/syck.so > /usr/lib64/ruby/1.8/yaml/syck.rb Yes, yes. I just had a look into the ruby-package not the -libs. > Needswork: > - License in spec should read BSD rather than GPL Done. > - Remove BR: /usr/bin/install (it comes from coreutils) Done. > - Remove BR: python (python-devel pulls it in) Done. > - For the python subpackage we need:: > Requires: python-abi = %(%{__python} -c "import sys; print sys.version[:3]") See comment from Ville... > - Leave out OCaml from the Summary as there's no OCaml extension. Also leave > out Syck from the summary as it's redundant. > - Leave out README.BYTECODE as you're not building that extension (or build > and include it. Reading the docs seems to impy it's an extension to the > syck library that will preparse a YAML document into bytecode that can then > be re-loaded by the library. Could be reading that wrong, though.) Done. > - Shared libraries need to be left executable or they won't be stripped for > debuginfo packages. Guess, I catched that. > - Static libraries and headers go in a -devel package. See below about creating > a dynamic library for the extensions: Do you really think, I should create another subpackage? > - When making the library and loadable mdules, needs to supply RPM_OPT_FLAGS > to the build process. OK, I think I catched that also. > - We can't BR: php-devel = ... [something that expands to the php-config > command] because the comand isn't present until after the BR is fulfilled. > If there's a specific version that's needed, use that, otherwise just use > BR: php-devel. This copied from another php extension. This works fine, as there is the || echo stuff... > - Doesn't build the python or php extensions due to not having a libsyc compiled > with -fPIC. (ie: we have a libsyck.a which was build without -fPIC. We need a > libsyck.so that was built with -fPIC.) Added RPM_OPT_FLAGS and -fPIC... > This problem has to go upstream to be fixed because with shared libraries > comes stricter versioning requirements. However, hacking together some libtool > based scripts is not incredibly hard. As long as upstream is amenable we could > do the actual work here. > - The php doesn't build due to the build process not finding either the syck > library or header. Something like this should work: > CFLAGS="$RPM_OPT_FLAGS -I../../lib" %configure --with-syck=../../ Added your patch. > Minor: > - Since the %post/%postun scripts contain just /sbin/ldconfig it would be > possible to use %post -p /sbin/ldconfig and %postun -p /sbin/ldconfig. My > understanding is this format allows rpm to do some optimizations (not > loading a shell to invoke ldconfig and running ldconfig only once when a > group of libraries are installed as one transaction.) You're right. Done. [ ... ] > Summary > ======= > If you want to work with upstream about the failure of the regression test and > whether libtool-based dynamic libraries are desirable I can get started on > creating some libtool infrastructure. Sounds fine. > TODO > ==== > Since I couldn't get a completed package, these things have not been > checked: > - rpmlint Fixed lint warnings. > - Check against complete PackagingGuidelines Did that. > - Recheck BR in mock Doin' that right now. Please review again. Created attachment 118131 [details]
Spec patch to solve problems in this review
Needswork:
- Using php-config in BR does throw an error. This is from mock's root.log:
sh: php-config: command not found
sh: php-config: command not found
sh: php-config: command not found
[...]
/usr/sbin/mock-helper yum --installroot /var/lib/mock/fedora-4-x86_64-core/root
resolvedep 'flex' 'python-devel' 'bison' 'php-devel = bad' 'gawk'
No Package Found for php-devel = bad
0:flex-2.5.4a-34.x86_64
0:python-devel-2.4.1-2.x86_64
0:bison-2.0-6.x86_64
0:gawk-3.1.4-5.2.x86_64
No Package Found for php-devel = bad
Using it in the plain Requires: is fine.
- There does need to be a -devel subpackage divided like so:
%files
%defattr(-,root,root,0755)
%doc COPYING README TODO RELEASE README.TXT CHANGELOG
%{_libdir}/*.so.*
%files devel
%{_libdir}/*.a
%{_libdir}/*.so
%{_includedir}/*.h
+ Notice that I've removed the "file mode" from %defattr. This means rpm
will use whatever file permissions are on the file system when packaging
the files. This is desirable so the shared libraries have the execute bit
set.
That allows rpm to automatically strip and create debuginfo packages for
the shared objects. The same could be achieved with your old %defattr line
and a %attr (0755, root, root) for every shared object. Which one to use
depends on whether make install gets permissions mostly right on its own.
(Hmmm.. the bug refering to this was closed WONTFIX but the problem doesn't
seem to exist anymore...)
+ Note also I left out the *.la files. On Linux these aren't necessary and
can cause linkage problems. The Fedora Core policy is to delete them in
the %install section.
- Sorry I was unclear -- with the libtool patch you don't need to pass -fPIC
explicitly. libtool builds the static library (libsyck.a) without -fPIC
for some speed gains for those who use it and the dynamic libraries
(libsyck.so.0.0.55) get built with -fPIC for everyone else.
- The php building needs a nasty hack to build: CFLAGS needs to be passed at
configure time like so:
phpize
CFLAGS="$RPM_OPT_FLAGS -I../../lib -L../../lib/.libs" %configure
--with-syck=[Any dir here]
%{__make} %{?_smp_mflags}
It's probably a problem that could be resolved gracefully in the
ext/php/config.m4 file.
+ Notice since we passed CFLAGS in at configure time (and required the nasty
hack) we don't want to push it back in at make time otherwise it'll
overwrite the CFLAGS that configure wrote out for us.
- rpmlint on the various files is showing that php is picking up an rpath into
the built directory. Deleting the *.la's before building the php module and
setting php_cv_cc_rpath=no for php's configure does trick. (*Phew*)
- For some reason when you imported my patch, one of the lines was
transcribed wrong::
+lybsyck_la_LDFLAGS = -version-info 0:55:0
Should be::
+libsyck_la_LDFLAGS = -version-info 0:55:0
- README.EXT ought to go into the devel package.
Minor:
- Group for php and python subpackages should probably be Development/Languages
(The majority of php and python modules are.)
- Group for the main syck library should be System Environment/Libraries
- python-abi is left out so just remember not to build on < FC4.
- Since you're building only for FC4+ you might as well target the FC4 default
php extension dir at %{_libdir}/php in case of php-config failure.
- Summaries for the php and python packages could be more informative (the
name is already syck-php and syck-python) Maybe "YAML extension for php"
"YAML extension for python".
- I'm in favor of repeating the main package description in the subpackages
so people don't have to query both the subpackage and the main package to
figure out what the subpackage does. It's not done consistently, though,
so the choice is yours.
- Is syck meant for consumption directly from C as well? If so I'd list C
in the main library summary.
- .pyo's aren't ghosted but it looks like we're phasing out that method of
doing things so it's up to the packager whether to ghost.
- %post/%postun is traditionally put before %file.
***
* A patch to fix these issues is attached.
***
Good:
- Naming matches upstream and folows the naming guidelines
- spec file named appropriately
- License is BSD
- License included in package
- Spec is in American English
- Spec is comprehensible
- Source matches upstream
- No locale so no %find_lang
- No Prefix
- After the above changes rpmlint gives these errors:
E: syck hardcoded-library-path in ../../lib/.libs"
(Ignorable -- it points to the not-yet-installed libsyck)
W: syck-python no-documentation
W: syck-php no-documentation
(If you can dig up any documentation, you can include it.)
If you apply the attached patch or fix the issues another way and fix the
error in the syck-libtool.patch, I'll approve this package.
Toshio, thanks a lot. I applied the patch, fixed the old patch and will import it now to CVS. Please note, that you're always invited to fix things on the syck package, in case I'm on vacation or have no time to do it. It seems you could be a good maintainer for this package... Means, I give you my cvs-commit-permission for this package! And if you ever want to takeover ownership for this - just let me know. Thanks for all your help! Toshio, could you maybe have also a look at Bug #165688. It's the perl-package: perl-YAML-Parser-Syck... No problem! I look forward to trying it out. As for the perl package, I'll take a look but I can't promise a review. I haven't reviewed (or looked at) a single perl package yet so my learning curve there will be pretty steep. Reviewing perl-packages isn't too hard, I think, but as syck is now in FE. Maybe someone else is goin' to review... The package checkin looks good. I see you've got the package built too. You can close the bug when you're ready. Package Change Request ====================== Package Name: syck New Branches: EL-4 EL-5 Owners: oliver cstpierre I am going to be maintaining the EPEL branches of Syck, and helping to co-maintain the Fedora branches. Thanks! Chris already informed me about his wish to add syck to EPEL - it's OK from my side. cvs done. syck-0.55-10.el5 has been submitted as an update for Fedora EPEL 5. http://admin.fedoraproject.org/updates/syck-0.55-10.el5 syck-0.55-10.el5 has been pushed to the Fedora EPEL 5 stable repository. If problems still persist, please make note of it in this bug report. |