Bug 719854 - Review Request: rubygem-xmlparser-0.6.81-1 - Ruby bindings to the Expat XML parsing library
Summary: Review Request: rubygem-xmlparser-0.6.81-1 - Ruby bindings to the Expat XML ...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: 16
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Steve Traylen
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard: opennebula-dep
: 722364 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-07-08 08:23 UTC by Ulrich Schwickerath
Modified: 2012-05-28 18:00 UTC (History)
6 users (show)

Fixed In Version: rubygem-xmlparser-0.6.81-9.fc15
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-05-08 04:20:16 UTC
Type: ---
Embargoed:
steve.traylen: fedora-review+
ulrich.schwickerath: rhel-rawhide+


Attachments (Terms of Use)

Description Ulrich Schwickerath 2011-07-08 08:23:02 UTC
Spec URL: http://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser.spec
SRPM URL: http://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser-0.6.81-1.el6.src.rpm
Description: Ruby bindings to the Expat XML parsing library

Comment 1 Steve Traylen 2011-07-08 18:15:59 UTC
Hi Ulrich, 

Thanks for submitting the review,

A quick glance at the .spec file I notice the following things:

1)  Your %build section references %{SOURCE0} which of course refers directly to your source.
     You need to prepare you source in %prep into a new directory and then build what you have
     prepared.

2) The .spec file contains some commented out lines, e.g 
    #rm -f %{buildroot}%{gemdir}/gems/xmlparser-0.6.81/ext/xmlparser/*.so
    #rm -f %{buildroot}%{gemdir}/gems/xmlparser-0.6.81/ext/xmlparser/*.o
    
    for no apparent reason, at the very least it should be commented to as to why but 
    probably it should just go.

3) You use %define and %global at the top, should almost always be %global.

http://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define
https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define

Now actually trying the .src.rpm.

4) Building on F15 results in:

ERROR:  Error installing /home/steve/rpmbuild/SOURCES/xmlparser-0.6.81.gem:
	ERROR: Failed to build gem native extension.

Since it does not build I've added "BuildFails" to the whiteboard above, be sure to remove
that once the package builds.

I confess I don't know the ruby guidelines well so if I contradict anything in those
I may well be wrong.

Comment 2 Ulrich Schwickerath 2011-07-09 08:58:29 UTC
I've updated the sources. 

The cause of the building problem eventually needs further investigation, I've reviewed the build dependencies to address this (possibly missing ruby-devel).

Comment 3 Steve Traylen 2011-07-09 16:23:53 UTC
Can you provide the link to the new .src.rpm and .spec file.

You should increase the release number at each iteration of the review.

Comment 5 Steve Traylen 2011-07-09 19:35:08 UTC
Hi, 

1)
I'm afraid the build still fails:

1xmlparser.c:50:24: fatal error: xmlparse.h: No such file or directory

to check the dependencies are correct you should definitely be 
checking with mock:

 mock -r fedora-rawhide-x86_64 \
     --rebuild ../SRPMS/rubygem-xmlparser-0.6.81-2.fc15.src.rpm

 will produce the above error.

2)
Concerning the "Your %build section references %{SOURCE0}" I'll try
to explain better.

Source0: http://whatever.org/%{gemname}-%{version}.gem

was correct, the URL before the file name is all only informational 
anyway but of course very useful.

What I did not explain well is that during %prep you prepare all your
sources into a directory ready for building.

So
%prep
%setup -q -c -T

copies the file over to a fresh directory and then %build
should be the following to operate on those files.

gem install -V --local --install-dir $(pwd)/%{gemdir} \
  --no-rdoc --no-ri --force %{gemname}-%{version}.gem

The problem with %{SOURCE0} is that this expands to the full source location,
probably $HOME/rpmbuilds/SOURCES/%{gemname}-%{version}.gem

3)
I expect 
BuildRequires: ruby-devel, rubygem-rake, rubygem-mkrf

is wrong and reubygems should be specified as 

BuildRequires: rubygem(mkrf), rubygem(rake)

Check some existing rubygem packages to see what they do.
e.g. 
http://pkgs.fedoraproject.org/gitweb/?p=rubygem-rake.git;a=blob;f=rubygem-rake.spec;h=9accf911a5cd25db4d4dbe392341f35a6f4d8dde;hb=HEAD

Comment 6 Ulrich Schwickerath 2011-07-09 21:02:57 UTC
Hi, Steve,

thanks for the comments. 

ad 1/ Obviously, expat is needed. I've added a a build dependency on expat-devel, and an install dependency on expat. That should solve this problem.

ad 2/ I think you are wrong here ... Nothing is being downloaded, and if I follow what you suggest, it bombs in fact out:
-bash-4.1$ rpmbuild -bi rubygem-xmlparser.spec
Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.Fp4pk7
+ umask 022
+ cd /afs/cern.ch/user/u/uschwick/rpm/BUILD
+ LANG=C
+ export LANG
+ unset DISPLAY
+ cd /afs/cern.ch/user/u/uschwick/rpm/BUILD
+ rm -rf rubygem-xmlparser-0.6.81
+ /bin/mkdir -p rubygem-xmlparser-0.6.81
+ cd rubygem-xmlparser-0.6.81
+ /bin/chmod -Rf a+rX,u+w,g-w,o-w .
+ exit 0
Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.gbzH0c
+ umask 022
+ cd /afs/cern.ch/user/u/uschwick/rpm/BUILD
+ cd rubygem-xmlparser-0.6.81
+ LANG=C
+ export LANG
+ unset DISPLAY
+ mkdir -p ./usr/lib/ruby/gems/1.8
++ pwd
+ gem install -V --local --install-dir /afs/cern.ch/user/u/uschwick/rpm/BUILD/rubygem-xmlparser-0.6.81//usr/lib/ruby/gems/1.8 --no-rdoc --no-ri --force xmlparser-0.6.81.gem
ERROR:  Could not find a valid gem 'xmlparser-0.6.81.gem' (>= 0) in any repository
error: Bad exit status from /var/tmp/rpm-tmp.gbzH0c (%build)

What I have been doing is strictly following the procedures, and if you check the link you sent in the previous comment you can see that they are actually using SOURCE0 in the install step of the gem, just as I did and as it is described in the instructions. 
I've checked at least 10 other gems as well, and they all do it this way. 

AFAIK I roll back to what I had in my first rpm.


ad 3) From the dependencies of the required packages (checking them with -q --provides) both ways seem to be technically possible. So it's not wrong, it's rather a matter of taste or policy. I've changed it anyway as you requested. 

So, here's the result of the mock run:
-bash-4.1$ mock -r fedora-rawhide-x86_64      --rebuild ./rubygem-xmlparser-0.6.81-3.el6.src.rpm 
INFO: mock.py version 1.1.11 starting...
State Changed: init plugins
INFO: selinux enabled
State Changed: start
INFO: Start(./rubygem-xmlparser-0.6.81-3.el6.src.rpm)  Config(fedora-rawhide-x86_64)
State Changed: lock buildroot
State Changed: clean
INFO: chroot (/var/lib/mock/fedora-rawhide-x86_64) unlocked and deleted
State Changed: unlock buildroot
State Changed: init
State Changed: lock buildroot
Mock Version: 1.1.11
INFO: Mock Version: 1.1.11
INFO: enabled root cache
State Changed: unpacking root cache
INFO: enabled yum cache
State Changed: cleaning yum metadata
INFO: enabled ccache
State Changed: running yum
State Changed: unlock buildroot
State Changed: setup
State Changed: build
INFO: Done(./rubygem-xmlparser-0.6.81-3.el6.src.rpm) Config(fedora-rawhide-x86_64) 0 minutes 22 seconds
INFO: Results and/or logs in: /var/lib/mock/fedora-rawhide-x86_64/result
State Changed: end

Created packages:
-rw-rw-r--. 1 uschwick c3    12658 Jul  9 22:51 build.log
-rw-rw-r--. 1 uschwick c3    18756 Jul  9 22:51 root.log
-rw-rw-r--. 1 uschwick mock  92331 Jul  9 22:51 rubygem-xmlparser-0.6.81-3.fc16.src.rpm
-rw-rw-r--. 1 uschwick mock 169108 Jul  9 22:51 rubygem-xmlparser-0.6.81-3.fc16.x86_64.rpm
-rw-rw-r--. 1 uschwick mock  46288 Jul  9 22:51 rubygem-xmlparser-debuginfo-0.6.81-3.fc16.x86_64.rpm
-rw-rw-r--. 1 uschwick c3      422 Jul  9 22:51 state.log

New packages:
http://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser.spec
http://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser-0.6.81-3.el6.src.rpm

Comment 7 Steve Traylen 2011-07-11 10:47:50 UTC
Concerning the %{SOURCE} item the following is I think better and does 
work.


%prep
%setup -q -c -T
cp -p %{SOURCE0} %{gemname}-%{version}.gem

%build
mkdir -p .%{gemdir}
#gem install -V --local --install-dir $(pwd)/%{gemdir} --no-rdoc --no-ri --force %{gemname}-%{version}.gem
gem install -V --local --install-dir $(pwd)/%{gemdir} --no-rdoc --no-ri --force %{gemname}-%{version}.gem

I appreciate it makes no difference in reality but is consistant with the flow of
other packages. I also see my earlier example rubygem-rake uses %{SOURCE0} in %build but
I believe this to be  wrong.

Anyway other things:

1) Requires: expat

is not needed, see:
http://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

%global ruby_sitearch %(ruby -rrbconfig -e "puts Config::CONFIG['sitearchdir']")
%global ruby_sitelib %(ruby -rrbconfig -e "puts Config::CONFIG['sitelibdir']")

is defined conditionally,  http://fedoraproject.org/wiki/Packaging:Ruby

%{!?ruby_sitelib: %global rub....

The rational being that in some future fedora version these macros will be defined
like e.g. the python ones.

3) Your changelog is not actually being updated between these increments.


Genrally this package is looking better now, in order for me to sponsor you which I am willing to do
can you maybe submit another package for review possibly but certainly provide an
informal review of some other packages. 

Choose a package from :

http://fedoraproject.org/PackageReviewStatus/NEW.html

I would avoid the shaded "sponsor needed ones"

when you have made a review of package there which you mark as informal review provide
a link here to it.

The other thing you can do now is submit this .src.rpm to koji as a --scratch
build.

For the sake of openness I declare also now that I work with and know you.

Steve.

Comment 8 Ulrich Schwickerath 2011-07-12 07:45:41 UTC
As discussed offline, I have changed the package doing a cp of the sources instead of using {SOURCES0} during the build step.

ad 1/ indeed, the explicit requirement on expat is not needed, removed it. For build it is kept of course, else it won't build

ad 2/ config paths updated. Hope that is what you mean... Please let me know

ad 3/ changelog updated.

new version:
http://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser.spec
http://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser-0.6.81-4.el6.src.rpm

Comment 9 Vít Ondruch 2011-07-12 08:36:52 UTC
Hi Ulrich, I am not sponsor so I cannot help with that, but I have several comments to your package:

1) First of all, the gem seems to be rather old. It seems that upstream released new version [1] but the update was never released in gem form. Also, the expat upstream doesn't seems to be extremely vital. So are you really sure that you like to import the gem into Fedora, instead of considering to switching to Nokogiri for example?

2) License
The license should be probably "GPLv2+ or Ruby or MIT" and according to readme, there are also some files licensed under the same terms as Perl.

3) You don't need the ruby_sitelib at all (btw you have type there).

4) BuildRoot is not required anymore

5) %clean section is not required

6) defattr is obsolete

7) You should submit standard build flags for building the C extension:
   export CONFIGURE_ARGS="--with-cflags='%{optflags}'"

8) Since the gem command itself merges all tree steps which are usually required, i.e. %prep, %build, %install, it is preferred to install the gem in %build section and then copy it in %install section into final place.

9) There has to be spaces between each changelog entry, otherwise "fedpkg clog"
   fails to extract the log entries correctly. For example
* Wed Jul 06 2011 Ulrich Schwickerath <uschwick.ch> - 0.6.81-4
- cleaner way to treat SOURCE
- remove explicit dependency on expat
- make globals conditional

* Wed Jul 06 2011 Ulrich Schwickerath <uschwick.ch> - 0.6.81-3
- fix build problems 


[1] http://www.yoshidam.net/Ruby.html#xmlparser

Comment 10 Steve Traylen 2011-07-12 08:45:16 UTC
 
> 4) BuildRoot is not required anymore
> 
> 5) %clean section is not required
> 
> 6) defattr is obsolete

But you can leave them in if you are targeting older platforms as well, e.g RHEL6 as well.

The other  comments look to all be valid.

Once you provide some informal review examples I will complete the formal review of the package.

Steve.

Comment 11 Ulrich Schwickerath 2011-07-12 12:21:55 UTC
Hi,

thanks for the additional comments!

ad 1/ for the time being it is still needed. For a specific application (OpenNebula) a huge speedup is archived by using this package (in addition to nokogiri). I've contacted the developers, the problem seems to come from dependencies on a third package. So, yes, although this package is old it would be good to have it in Fedora. It'll be needed in EPEL for  RHES5 and RHES6

ad 2/ should be ruby, from the README. Fixed.

ad 3/ true. Removed it.

ad 4/5/6 : I need to keep them as I need to build the package also on RHES5 and RHES6. Getting it for these distributions is the goal of this submission

ad 7/ done

ad 8/ This is what I'm doing in this spec. The copy in the prep step was requested by Steve. 

ad 9/ done


http://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser.spec
http://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser-0.6.81-5.el6.src.rpm

Comment 12 Steve Traylen 2011-07-12 12:31:11 UTC
(In reply to comment #9)

> 2) License
> The license should be probably "GPLv2+ or Ruby or MIT" and according to readme,
> there are also some files licensed under the same terms as Perl.

> [1] http://www.yoshidam.net/Ruby.html#xmlparser

Just to be clear I had not done the full review yet and had not checked the
licensing.

The licensing is impressively confused in fact.

http://www.yoshidam.net/xmlparser_en.txt  and the README

gives part Ruby or "license of expat"
and also bizarrely partly there is a Perl license on encoding.h

In addition: xpath.rb is GPLv2+ 

so this all resolves to

License: GPLv2+ and ( Ruby or GPLv2+ ) and ( GPLv2+ or Artistic )

which would definitely need some comments to explain which file is which.

Comment 13 Vít Ondruch 2011-07-12 13:01:46 UTC
(In reply to comment #12)
Since Expat is MIT licensed, there should be mentioned also MIT, otherwise I agree, the licensing is a mess :/

Comment 14 Ulrich Schwickerath 2011-07-12 13:35:33 UTC
So, 

License: GPLv2+ and ( Ruby or GPLv2+ ) and ( GPLv2+ or Artistic ) and MIT

+ add something in the description ?

Comment 15 Ulrich Schwickerath 2011-07-12 14:29:15 UTC
How about
...
License: GPLv2+ and ( Ruby or GPLv2+ or MIT ) and ( GPLv2+ or Artistic ) 
...
%description
Ruby bindings to the Expat XML parsing library. For details about the license conditions of individual files in this package please refer to the README.

Comment 16 Steve Traylen 2011-07-12 14:37:10 UTC
Look at an example for how to add comments explaining the license, the comment
is to describe the License: in the spec file.

http://pkgs.fedoraproject.org/gitweb/?p=gridsite.git;a=blob;f=gridsite.spec;h=9ee371d19a1c50cfabe0a2b89c880cc4c6e4df41;hb=HEAD


In particular read 

http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Dual_Licensing_Scenarios

Comment 17 Ulrich Schwickerath 2011-07-12 15:44:45 UTC
I've updated the spec file and bumped up the release to 6

Comment 19 Shawn Starr 2011-07-15 15:27:37 UTC
Alright, you are also trying to package xmlparser for OpenNebula 2.2+?

I also was trying to package this, and other dependencies needed.

See bug #722364

Comment 20 Shawn Starr 2011-07-15 15:32:52 UTC
Ulrich, if you want to help with OpenNebula packaging please come on IRC in the #opennebula channel and chat with jmelis and me spstarr, let's coordinate :)

Comment 21 Ulrich Schwickerath 2011-07-16 07:38:23 UTC
Shawn, I can't use IRC due to security restrictions, sorry. I've been discussing this with Ruben. If you want to take over on this one that's fine with me.

Comment 22 Shawn Starr 2011-07-17 17:56:09 UTC
Please go ahead with your submission, I will focus on dm-sqlite/mysql-adapter rubygems.

I will close my bug.

Comment 23 Shawn Starr 2011-07-17 17:57:22 UTC
*** Bug 722364 has been marked as a duplicate of this bug. ***

Comment 24 Steve Traylen 2011-07-17 19:03:47 UTC
Package Review
==============
rubygem-xmlparser-0.6.81-5.fc15.src.rpm
Builds in F15 x86_64 mock.

$ rpmlint rubygem-xmlparser.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/*.rpm
0 packages and 0 specfiles checked; 0 errors, 0 warnings.

Key:
- = N/A
x = Check
! = Problem
? = Not evaluated

=== REQUIRED ITEMS ===
[x]  Package is named according to the Package Naming Guidelines. 
[x]  Spec file name must match the base package %{name}, in the format %{name}.spec.
[x]  Spec file is legible and written in American English.
[x]  Spec file lacks Packager, Vendor, PreReq tags.
[x]  Spec uses macros instead of hard-coded directory names.
[x]  Package consistently uses macros.
[x]  Macros in Summary, %description expandable at SRPM build time.
[x]  PreReq is not used.
[x]  Requires correct, justified where necessary.
[x]  All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. 
[x]  Buildroot is correct (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)).
[x]  Package run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) and the beginning of %install.
[x]  Package use %makeinstall only when ``make install DESTDIR=...'' doesn't work.
[x]  Package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[-]  The spec file handles locales properly.
[x]  Changelog in prescribed format.
[x]  Rpmlint output is silent.
[x]  License field in the package spec file matches the actual license.
[-]  If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc.
[-]  License file installed when any subpackage combination is installed.
[x]  Sources contain only permissible code or content.
[x]  Sources used to build the package matches the upstream source, as provided in the spec URL.
     MD5SUM this package     :  dc494768a4ea0c7bedd3444a112ac3ae
     MD5SUM upstream package : dc494768a4ea0c7bedd3444a112ac3ae
[x]  Compiler flags are appropriate.
Yes %{optflags} is being used.
[-]  ldconfig called in %post and %postun if required.
[!]  Package must own all directories that it creates.
[x]  Package does not own files or directories owned by other packages.
[x]  Package requires other packages for directories it uses.
[x]  Package does not contain duplicates in %files.
[x]  Permissions on files are set properly.
[x]  Each %files section contains %defattr.
[x]  No %config files under /usr.
[-]  %config files are marked noreplace or the reason is justified.
[-]  Package contains a properly installed %{name}.desktop using desktop-file-install file if it is a GUI application. 
[-]  Package contains a valid .desktop file.
[x]  Package contains code, or permissable content.
[-]  Package contains a SysV-style init script if in need of one.
[x]  File names are valid UTF-8.
[-]  Large documentation files are in a -doc subpackage, if required.
[x]  Package uses nothing in %doc for runtime.
[x]  Package contains no bundled libraries.
[-]  Header files in -devel subpackage, if present.
[-]  Static libraries in -static subpackage, if present.
[x]  Package contains no static executables.
[-]  Package requires pkgconfig, if .pc files are present.
[-]  Development .so files in -devel subpackage, if present.
[-]  Fully versioned dependency in subpackages, if present.
[-]  Package does not contain any libtool archives (.la).
[x]  Useful -debuginfo package or justification otherwise.
[x]  Rpath absent or only used for internal libs.
[x]  Package does not genrate any conflict.
[x]  Package does not contains kernel modules.
[x]  Package is not relocatable.
[x]  Package successfully compiles and builds into binary rpms on at least one supported architecture.
[x]  Package is not known to require ExcludeArch.
[x]  Package installs properly.
[x]  Package obeys FHS, except libexecdir and /usr/target.
[x]  Package meets the Packaging Guidelines. 

=== Issues ===
1. Not all directories you created are owned or belong to
   packages you pulled in, in particular:

   /usr/lib/ruby/gems/1.8/gems/xmlparser-0.6.81

   is not owned but you created it.

   See: http://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership

2. While your .spec file includes the extra licensing comments the
   .spec file in the .src.rpm does. Be sure to bring this into with
   with the next iteration.

=== Final Notes ===
The package looks good now other than the two items above.

Please do some informal reviews of other packages and provide a reference
here so I can find them.

This is needed before I can sponsor you.

Comment 25 Shawn Starr 2011-07-20 15:34:43 UTC
Additional comments:

the DSO library should be in %{ruby_sitearch}/%{gemname}/*.so Same as rubygem-cairo, although rubygem-json uses %{ruby_sitearch}/%{gemname}/etc/ for the DSO location.

Comment 26 Shawn Starr 2011-08-04 04:49:44 UTC
I guess that doesn't matter, there doesn't seem to be a clear guideline, but I've been following %{ruby_sitearch}/%{gemname}/*.so

Can this package be approved for submission?

Comment 27 Steve Traylen 2011-08-04 09:53:22 UTC
I can sponser ulrich and approve package, but i still need to see an informal review of some other package.

Steve.

Comment 28 Vít Ondruch 2011-08-04 09:57:30 UTC
(In reply to comment #26)
> I guess that doesn't matter, there doesn't seem to be a clear guideline, but
> I've been following %{ruby_sitearch}/%{gemname}/*.so

The *.so file has to follow the original gem folder structure. E.g: 

1) %{geminstdir}/lib/foo.so => %{ruby_sitearch}/foo.so
2) %{geminstdir}/lib/%{gemname}/foo.so => %{ruby_sitearch}/%{gemname}/foo.so
2) %{geminstdir}/ext/foo.so => %{ruby_sitearch}/ext/foo.so

If you do not follow this rule, then you would need to patch the require of *.so file somewhere in gem ruby code, because the *.so file wouldn't be possible to find using Ruby $LOAD_PATH.

Comment 29 Ulrich Schwickerath 2011-10-01 20:28:14 UTC
Hi,sorry for the long silence on this, I was away for a while on holiday. I'll get in touch with Steve to see how to unblock this gem.

Comment 30 Ulrich Schwickerath 2011-12-07 10:49:09 UTC
I've done an informal review done on 
https://bugzilla.redhat.com/show_bug.cgi?id=760357

Comment 31 Ulrich Schwickerath 2011-12-07 13:53:59 UTC
I've tried to follow the rule for moving the .so files. If I do so, i.e. if I move the library to %{ruby_sitearch}/lib/, the package no longer works, at least not on my test machine. Adding a link solved this problem. 

Concerning %{gemdir}/gems/%{gemname}-%{version} which was created but not owned by the package, I have added it as %dir in the new version. 


Here's a new version of the package:

http://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser.spec
http://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser-0.6.81-7.el6.src.rpm


The spec file in the src.rpm and the one given are identical (I've checked) but maybe I misinterpreted this comment from Steve.

Comment 32 Vít Ondruch 2011-12-07 14:33:48 UTC
(In reply to comment #31)
> I've tried to follow the rule for moving the .so files. If I do so, i.e. if I
> move the library to %{ruby_sitearch}/lib/, the package no longer works, at
> least not on my test machine. Adding a link solved this problem. 

You should first understand how Ruby together with RubyGems are treating load paths. I'll try to shortly explain:
1) If you do "require 'foo'", Ruby searches all the load paths and if finds the file on some of them, it loads it. Ruby has some set of default load paths and %{buildroot}%{ruby_sitearch} is one of them. I.e. if you place file foo.rb (or foo.so) into that path, then you can do later "require 'foo'".
2) If the file is not found in load paths, then the RubyGems loading mechanism kicks in. RubyGems goes through all installed gems and looks for suitable file to load. When it is found, RubyGems will insert its paths (which are specified by #require_paths method in .gemspec file) into Ruby's load paths and issues again regular Ruby's require.

So if you place the library into the %{buildroot}%{ruby_sitearch}/xmlparser.so location, then there is no need to do any link, since Ruby can find the file on its load path. And that is the way how it should be done currently.


And I have some additional notes:
* It seems that the author of the gem is somebody else then the original upstream. However, it would be nice if the link to homepage was somehow useful, may be http://www.yoshidam.net/Ruby.html#xmlparser is the correct one?
* It might make sense to ask the author of the gem for update to the latest upstream version of the library?

Comment 33 Ulrich Schwickerath 2011-12-07 14:52:22 UTC
Hi, Vít,

thanks for the explanation. I admit that I'm not a ruby geek ;-) I did the change in this latest version because of comment 28. What you suggest now is what I did before. Personally I have a slight preference for the original approach because it is simpler, I have no problem to roll back. Let me know. 

About upgrading: this was discussed earlier in this thread already. I agree that it is a good idea to ping the original author to get an updated package. For the time being though this one seems to be the best we can do. It's needed for OpenNebula.

Comment 34 Vít Ondruch 2011-12-07 15:14:55 UTC
(In reply to comment #33)
> Hi, Vít,
> 
> thanks for the explanation. I admit that I'm not a ruby geek ;-) I did the
> change in this latest version because of comment 28. What you suggest now is
> what I did before. Personally I have a slight preference for the original
> approach because it is simpler, I have no problem to roll back. Let me know. 

Sorry for the confusion. In comment 28, I just tried to point out that Shawn was not right, that the guideline is clear, but I apparently achieved the opposite effect ;)

> About upgrading: this was discussed earlier in this thread already. I agree
> that it is a good idea to ping the original author to get an updated package.
> For the time being though this one seems to be the best we can do. It's needed
> for OpenNebula.

Not an issue. It was just note.

Comment 36 Shawn Starr 2011-12-20 17:44:52 UTC
Are we good for submission? Can someone approve the review if this is acceptable?

Comment 37 Shawn Starr 2011-12-20 17:53:36 UTC
also, has Ulrich been mentored for packager/provenpackager? who is maintainer of this once it's approved?

Comment 38 Steve Traylen 2011-12-20 20:44:53 UTC
Vít,

I'd like to sponsor Ulrich as I know he knows what he is doing but while my packaging is generally good my ruby is less so.

This package looks almost good to me but if you have further comment I would be grateful, this turns out to be a non-trivial ruby package.

The alternative is I release this as being assigned to me.

Steve.

Problems I still see however:

(1)
Requires: ruby-libs

is almost certainly not needed and is you get anyway from both
libruby.so.1.8()(64bit)  
ruby(abi) = 1.8

the first one of which in particular is autogenerate, basically you should
not added which is determined automatically anyway.

(2)
The fact the package contains the origional gem seems bad.

/usr/lib/ruby/gems/1.8/cache/xmlparser-0.6.81.gem

it should not.

Comment 39 Vít Ondruch 2011-12-21 09:51:56 UTC
(In reply to comment #38)
> Vít,
> 
> I'd like to sponsor Ulrich as I know he knows what he is doing but while my
> packaging is generally good my ruby is less so.
> 
> This package looks almost good to me but if you have further comment I would be
> grateful, this turns out to be a non-trivial ruby package.
> 
> The alternative is I release this as being assigned to me.
> 
> Steve.
> 
> Problems I still see however:
> 
> (1)
> Requires: ruby-libs
> 
> is almost certainly not needed and is you get anyway from both
> libruby.so.1.8()(64bit)  
> ruby(abi) = 1.8
> 
> the first one of which in particular is autogenerate, basically you should
> not added which is determined automatically anyway.

Actaully the 'ruby-libs' should be replaced with 'ruby', as is stated in Ruby packaging guidelines.

> (2)
> The fact the package contains the origional gem seems bad.
> 
> /usr/lib/ruby/gems/1.8/cache/xmlparser-0.6.81.gem
> 
> it should not.

Although there is a lot of gems which keeps the original gem in the RPM and it is not against packaging guidelines, I also recommend to use %exclude for the cached gem.

Otherwise I am fine with the package. Feel free to approve it and sponsor Ulrich any time.

Comment 40 Steve Traylen 2012-01-11 15:53:11 UTC
APPROVED

You can now proceed to request an SCM area for this package:

http://fedoraproject.org/wiki/Package_SCM_admin_requests

Comment 41 Steve Traylen 2012-01-11 16:01:29 UTC
Ulrich you make the changes that VIt mentions in comment #39 first of course.

Comment 42 Shawn Starr 2012-02-10 04:14:28 UTC
Can I get an update please? This is now urgent for OpenNebula and Fedora 17 timeframes.

Comment 43 Ulrich Schwickerath 2012-03-06 13:04:19 UTC
Sorry for the delay on this. I've fixed the ruby-lib dependency in 

http://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser.spec
http://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser-0.6.81-9.el6.src.rpm

Comment 44 Ulrich Schwickerath 2012-03-06 13:24:49 UTC
New Package SCM Request
=======================
Package Name: rubygem-xmlparser
Short Description: Ruby bindings to the Expat XML parsing library
Owners: schwicke
Branches: f15 f16 el6
InitialCC:

Comment 45 Gwyn Ciesla 2012-03-06 14:07:13 UTC
Git done (by process-git-requests).

Don't include the version in future summaries.  Thanks!

Comment 46 Ulrich Schwickerath 2012-03-09 07:40:15 UTC
The build for el6 currently fails because rubygem-mkrf is missing for el6. Can that be included in el6 ?

Comment 47 Vít Ondruch 2012-03-09 08:04:45 UTC
(In reply to comment #46)
Well if you become maintainer for epel6, I guess it should be no problem. Please ask in the mkrf review ticket #508416

Comment 48 Ulrich Schwickerath 2012-03-21 09:03:38 UTC
The original package does not build with ruby >= 1.9. I've contacted the original developer who provided an updated gemspec from which I built a newer version of the gem. It's available here:

https://uschwick.web.cern.ch/uschwick/software/xmlparser-0.7.2.gem

Updated spec file:
https://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser.spec

Updated source rpm:
https://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser-0.7.2-1.el6.src.rpm

Comment 49 Bohuslav "Slavek" Kabrda 2012-03-21 09:11:54 UTC
Please note, that you need to update to 0.7.2.1. The previous release had buggy require_paths and was removed.

Comment 50 Ulrich Schwickerath 2012-03-22 08:07:33 UTC
Looks like I'm doomed. The new version compiles fine on fed17 and fed18 but the package was removed from there because it did not build, and I cannot upgrade. On fed16 the old package builds but the new one fails in the installation step. This might be related to the version of rubygem-mkrf which is older than on fed17. It builds fine locally on an el6 box.

Comment 51 Bohuslav "Slavek" Kabrda 2012-03-22 08:26:55 UTC
I'm not sure if I understand you problem correctly, but wouldn't placing the 0.6.81 into F16 and 0.7.2.1 into F17 and above solve the problem?

Comment 52 Ulrich Schwickerath 2012-03-22 10:47:28 UTC
Yes, that was my original intention. I probably misunderstood you then. I was trying to upgrade f16 as well. Sorry about that, will have to roll that back. 

I lost access to f17 and the development branch, cannot commit any longer.

Comment 53 Steve Traylen 2012-03-22 12:55:06 UTC
Ticket stays with me as record of who approved the package.

Comment 54 Ulrich Schwickerath 2012-03-22 13:08:54 UTC
I've opened a new review request (805911) for f17 and rawhide for the new version 0.7.2.1

This version here is good for up to f16 (included).

Comment 55 Ulrich Schwickerath 2012-04-24 10:11:29 UTC
Will open a new SCM request for the update, as suggested in 805911

Comment 56 Ulrich Schwickerath 2012-04-24 10:17:30 UTC
Package Change Request
======================
Package Name: rubygem-xmlparser
New Branches: f17
Owners: schwicke

Comment 57 Vít Ondruch 2012-04-24 10:40:19 UTC
You need to set the cvs flag (I did that for you, hope it doesn't matter who did that :)

Comment 58 Gwyn Ciesla 2012-04-24 12:46:39 UTC
Not sure why this was retired, I unretired f17 and devel, take ownership in
pkgdb.

Comment 59 Fedora Update System 2012-04-24 13:55:52 UTC
rubygem-xmlparser-0.7.2-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/rubygem-xmlparser-0.7.2-1.fc17

Comment 60 Fedora Update System 2012-04-24 13:57:37 UTC
rubygem-xmlparser-0.6.81-10.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/rubygem-xmlparser-0.6.81-10.fc16

Comment 61 Fedora Update System 2012-04-24 14:01:39 UTC
rubygem-xmlparser-0.6.81-9.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/rubygem-xmlparser-0.6.81-9.fc15

Comment 62 Fedora Update System 2012-04-25 04:48:09 UTC
rubygem-xmlparser-0.7.2-1.fc17 has been pushed to the Fedora 17 testing repository.

Comment 63 Ulrich Schwickerath 2012-04-26 06:24:12 UTC
As pointed out by vondruch, the package for >=fed17 should apply the new packaging guidelines. Here's a new spec file 
 
https://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser.spec

https://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser-0.7.2-2.fc17.src.rpm

It still requires a bit of functional testing.

Comment 64 Vít Ondruch 2012-04-26 08:32:31 UTC
Please note that you can/should now use macros provided by rubygems-devel to ease the packaging. Please refer to macros section of Ruby packaging guidelines [1]. In other words, your files section should looks like:

%files
%{gem_extdir}/lib/xmlparser.so
%dir %{gem_instdir}/
%doc %{gem_instdir}/[A-Z]*
%{gem_instdir}/[a-z]*
%exclude %{gem_cache}
%{gem_spec}


[1] https://fedoraproject.org/wiki/Packaging:Ruby#Macros

Comment 65 Ulrich Schwickerath 2012-04-26 10:00:17 UTC
Hi Vit,

thanks a lot for your help on this! 
I have another iteration on this ready now, as I also realised that since I built this updated version there is finally an updated gem available from rubyforge while the previous version was using what I directly got from the author. 

https://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser.spec
https://uschwick.web.cern.ch/uschwick/software/rubygem-xmlparser-0.7.2.1-3.fc17.src.rpm

rpmlint output:

srpm:
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

binary (on fed17):

rubygem-xmlparser.x86_64: W: no-soname /usr/lib64/gems/exts/xmlparser-0.7.2.1/lib/xmlparser.so
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Hopefully, this will be the final version. Any feedback on it is very welcome before I proceed.

Comment 66 Vít Ondruch 2012-04-26 10:08:09 UTC
I did not tested it but it seems it should be OK. Thank you.

Comment 67 Fedora Update System 2012-04-26 11:50:12 UTC
rubygem-xmlparser-0.7.2.1-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/rubygem-xmlparser-0.7.2.1-3.fc17

Comment 68 Fedora Update System 2012-04-26 19:30:54 UTC
Package rubygem-xmlparser-0.7.2.1-3.fc17:
* should fix your issue,
* was pushed to the Fedora 17 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing rubygem-xmlparser-0.7.2.1-3.fc17'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2012-6709/rubygem-xmlparser-0.7.2.1-3.fc17
then log in and leave karma (feedback).

Comment 69 Fedora Update System 2012-05-08 04:20:16 UTC
rubygem-xmlparser-0.7.2.1-3.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 70 Fedora Update System 2012-05-10 14:22:10 UTC
rubygem-xmlparser-0.6.81-10.fc16 has been pushed to the Fedora 16 stable repository.

Comment 71 Fedora Update System 2012-05-10 14:29:53 UTC
rubygem-xmlparser-0.6.81-9.fc15 has been pushed to the Fedora 15 stable repository.

Comment 72 Fedora Update System 2012-05-13 05:20:51 UTC
rubygem-xmlparser-0.6.81-10.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/rubygem-xmlparser-0.6.81-10.el6

Comment 73 Fedora Update System 2012-05-28 18:00:27 UTC
rubygem-xmlparser-0.6.81-10.el6 has been pushed to the Fedora EPEL 6 stable repository.


Note You need to log in before you can comment on or make changes to this bug.