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.