Bug 240008 - Review Request: ruby-shadow - ruby bindings for shadow password access
Review Request: ruby-shadow - ruby bindings for shadow password access
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-14 07:17 EDT by Kostas Georgiou
Modified: 2008-04-29 11:12 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-25 12:45:06 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
mock build log of ruby-shadow-1.4.1-4 on FC-devel i386 (21.82 KB, text/plain)
2007-05-15 09:18 EDT, Mamoru TASAKA
no flags Details

  None (edit)
Description Kostas Georgiou 2007-05-14 07:17:05 EDT
Spec URL: http://www.hep.ph.ic.ac.uk/~georgiou/ruby-shadow/ruby-shadow.spec
SRPM URL: http://www.hep.ph.ic.ac.uk/~georgiou/ruby-shadow/ruby-shadow-1.4.1-1.FC6.src.rpm
Description: ruby bindings for shadow password access (getspnam et al). Needed for puppet (already in extras) to manage local accounts
Comment 1 Mamoru TASAKA 2007-05-14 10:40:10 EDT
As I maintain several ruby modules, I make a quick comment.

* Are you sure that this is _truly_ licensed under
  public domain?
  (Well, actually many packages created by Japanese developer
   have no license documents or have some original license texts
   and each time I want to package into rpm style, I have to
   ask the developer and someone else about the license......)

* I strongly think that BuildRequires should also have
  "BuildRequires: ruby(abi) = 1.8"
  because you can rebuild this under ruby 1.9 (when ruby 1.9 rpm
  is out) but it is wrong because the rebuilt rpm should require
  ruby 1.9, not 1.8

* Fedora's default encoding is UTF-8 and please change the 
  encoding of README.euc to UTF-8 (and rename it).

* Compiler flags are wrong (do not honor fedora specific).
---------------------------------------------------------------
+ make
gcc -I. -I. -I/usr/lib/ruby/1.8/i386-linux -I. -DHAVE_GETSPENT -DHAVE_SGETSPENT
-DHAVE_FGETSPENT -DHAVE_SETSPENT -DHAVE_ENDSPENT -DHAVE_LCKPWDF -DHAVE_ULCKPWDF
 -fPIC   -c shadow.c
gcc -shared  -L"/usr/lib" -o shadow.so shadow.o  -lruby  -lpthread -ldl -lcrypt
-lm   -lc
+ exit 0
---------------------------------------------------------------
Comment 2 Kostas Georgiou 2007-05-14 13:23:04 EDT
Thanks for the comments.

1) I will ask just to make sure, the README says "License: Free for any use with
your own risk!". Looking again it seems that the debian package has the program
 with a Ruby/GPL license which is strange.

2) Thinking about it you are right. Although "BuildRequires: ruby(abi) = 1.8" wi
ll not solve the problem. Lets say that we have ruby 1.9 as the standard package
 and ruby 1.8 installed as ruby1.8 by a compat package. The require will pull in
 ruby1.8 but the package will still be build with 1.9. I'll add the buildrequire
 but if we are going to have compat packages for ruby in the future this we need
 something else.

3) Is it OK to rename it to README.jp (after it's converted in utf8)?

4) I missed this somehow, I'll fix it before the next update.
Comment 3 Mamoru TASAKA 2007-05-14 13:43:39 EDT
(In reply to comment #2)
> Thanks for the comments.
> 
> 1) I will ask just to make sure, the README says "License: Free for any use with
> your own risk!". 
Okay. We can regard this as public domain.

> 2) Thinking about it you are right. Although "BuildRequires: ruby(abi) = 1.8" wi
> ll not solve the problem. Lets say that we have ruby 1.9 as the standard package
>  and ruby 1.8 installed as ruby1.8 by a compat package. The require will pull in
>  ruby1.8 but the package will still be build with 1.9. I'll add the buildrequire
>  but if we are going to have compat packages for ruby in the future this we need
>  something else.

Truly for ruby BuildRequires: ruby(abi) = 1.8" does not prevent
this package from being rebuilt by ruby 1.9......

> 3) Is it OK to rename it to README.jp (after it's converted in utf8)?
Okay.

> 4) I missed this somehow, I'll fix it before the next update.
Thanks.

Comment 4 Kostas Georgiou 2007-05-15 04:51:39 EDT
Spec URL: http://www.hep.ph.ic.ac.uk/~georgiou/ruby-shadow/ruby-shadow.spec
SRPM URL:
http://www.hep.ph.ic.ac.uk/~georgiou/ruby-shadow/ruby-shadow-1.4.1-2.FC6.src.rpm

Things left to do: confirm license.

I would also like to find a better way to handle the abi issue, I don't think
that adding "Requires: ruby(abi) = 1.8" is the right way for a binary package,
we need to somehow detect the abi we are building with and require that. This is
an issue for all binary ruby packages though.
Comment 5 Kostas Georgiou 2007-05-15 05:22:12 EDT
Looking at what the rpm requires I get this:
$ rpm -q --requires ruby-shadow
...
libruby.so.1.8()(64bit)
ruby(abi) = 1.8
...

So we already require the 1.8 abi indirectly through libruby.so.1.8 right? This
way we will get the right requires even if we build with another version of ruby
and ruby(abi) = .. is redundant.
Comment 6 Mamoru TASAKA 2007-05-15 05:33:12 EDT
(In reply to comment #5)
> Looking at what the rpm requires I get this:
> $ rpm -q --requires ruby-shadow
> ...
> libruby.so.1.8()(64bit)
> ruby(abi) = 1.8
> ...
> 
> So we already require the 1.8 abi indirectly through libruby.so.1.8 
> right? 
I know it already, however this works only for ruby module packages
containing binary modules linked with libruby.so.*. 

So that we require to add "Requires: ruby(abi) = 1.8" is to make
sure that this method works for as many (including noarch,
arch-dependent) packages as possible. You can see the same
phenomenon on python replated packages, where
"Requires: python(abi) = 2.5" is automatically added even to
arch-dependent packages.

[tasaka1@localhost ~]$ rpm -q --requires python-imaging
libc.so.6  
libc.so.6(GLIBC_2.0)  
libc.so.6(GLIBC_2.1)  
libc.so.6(GLIBC_2.1.3)  
libc.so.6(GLIBC_2.3)  
libc.so.6(GLIBC_2.3.4)  
libc.so.6(GLIBC_2.4)  
libfreetype.so.6  
libjpeg.so.62  
libpthread.so.0  
libpthread.so.0(GLIBC_2.0)  
libpython2.5.so.1.0  <========================
libz.so.1  
python(abi) = 2.5  <========================
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rtld(GNU_HASH)  

and we want to apply this python method to ruby module packages
_manually_ (we cannot do this automatically now because rpmbuild
does not handle ruby abi for now)
Comment 7 Mamoru TASAKA 2007-05-15 05:43:46 EDT
By the way, are you sponsored?

It seems that this is your first submission of package review
request......
Comment 8 Kostas Georgiou 2007-05-15 06:00:30 EDT
The _manually_ is the part that I don't like :) What I was really saying was
that we should handle ruby in rpmbuild instead of doing things manually.

What I should have asked is if anyone is working in adding this to rpmbuild, the
manual way will cause us problems when we upgrade ruby, the noarch rpms will
still build but they'll require the wrong abi :(

If nobody is looking at adding ruby support in rpmbuild I'll have a look.
Comment 9 Kostas Georgiou 2007-05-15 06:04:23 EDT
No I am not sponsored yet. It is indeed my first submission :)
Comment 10 Mamoru TASAKA 2007-05-15 06:06:54 EDT
Yeah, I don't disagree that ruby abi detection must be
done by rpmbuild, not manual treatment *in the future*....

But hacking rpm itself is actually beyond my skill and
also I cannot tell when automatical ruby abi detection
will be added into rpm, and Fedora release.
So if you can try "rpm" hacking, I am surely appreciated!!
Comment 11 Mamoru TASAKA 2007-05-15 06:13:03 EDT
(In reply to comment #9)
> No I am not sponsored yet. It is indeed my first submission :)

Well, okay. as far as I saw your bugzilla entry I can judge
that I can sponsor you. I will review this after I take a bath
(I live in Japan).

Setting once to block FE-NEEDSPONSOR. When I receive a mail
that you need a sponsor, I will sponsor you.
Comment 12 Kostas Georgiou 2007-05-15 07:10:11 EDT
Thanks,

What do you think about this change?
Requires: ruby(abi) = %(%{_bindir}ruby  -e "puts
RUBY_VERSION.sub(/(\d+\.\d+).*/,'\1')")

The regexp could be improved I imagine but it can not be worst than just using
"1.8" and we can get away with the BuildRequires.
Comment 13 Mamoru TASAKA 2007-05-15 07:27:16 EDT
Well, if you do so, perhaps it is better that you
define the macro %ruby_abi (for example) such as

%define ruby_abi <some regexp>

and use:
Requires: ruby(abi) = %ruby_abi
Comment 14 Kostas Georgiou 2007-05-15 07:36:55 EDT
Yes you are right.

I just noticed that the library doesn't work while using Fedora's CFLAGS, time
to do some debugging I am afraid :(
Comment 15 Mamoru TASAKA 2007-05-15 08:23:06 EDT
(In reply to comment #14)
> I just noticed that the library doesn't work while using Fedora's CFLAGS, time
> to do some debugging I am afraid :(

Are there any short examples which brings up the problems
you are meeting?
Comment 16 Kostas Georgiou 2007-05-15 08:52:04 EDT
require 'shadow' under x86_64 was enough to cause a core dump. The cause was the
the struct defines used 0 instead of NULL. I uploaded a newer version with the fix. 

Spec URL: http://www.hep.ph.ic.ac.uk/~georgiou/ruby-shadow/ruby-shadow.spec
SRPM URL:
http://www.hep.ph.ic.ac.uk/~georgiou/ruby-shadow/ruby-shadow-1.4.1-3.FC6.src.rpm
Comment 17 Kostas Georgiou 2007-05-15 08:59:44 EDT
Version 1.4.1-4 uploaded in the same location with a much cleaner rubi_api macro
as suggested by lutter (ruby -rrbconfig -e "puts Config::CONFIG['ruby_version']")
Comment 18 Mamoru TASAKA 2007-05-15 09:18:45 EDT
Created attachment 154732 [details]
mock build log of ruby-shadow-1.4.1-4 on FC-devel i386

It seems that ruby_abi must be written explicitly...
Comment 19 David Lutterkort 2007-05-15 09:43:00 EDT
The problem is that ruby is not in the build root by default, so that when mock
looks at the specfile to find the BR's, ruby_abi has no value and expands to
nothing, yielding a syntactically incorrect specfile.

Either the macro needs to be changed to have a value even if there is no ruby,
or 'ruby(abi) = 1.8' should be hardcoded.
Comment 20 Kostas Georgiou 2007-05-15 16:51:17 EDT
I see, is one of the following acceptable then?

Requires: ruby(abi) = %{?ruby_abi:mock}
Requires: ruby(abi) = %{?ruby_abi:1.8}
or go back to
Requires: ruby(abi) = 1.8 (and reenable BuildRequires: ruby(abi) = 1.8)

They are listed in order of preference but I can live with any of them :)
Comment 21 Mamoru TASAKA 2007-05-16 02:52:34 EDT
(In reply to comment #20)
> I see, is one of the following acceptable then?
> 
> Requires: ruby(abi) = %{?ruby_abi:mock}
> Requires: ruby(abi) = %{?ruby_abi:1.8}

%{?ruby_abi:mock} means
* when ruby_abi is defined, the output is "mock"
* when ruby_abi is not defined, the output is nothing.

So Requires: ruby(abi) = %{?ruby_abi:mock} means
* when ruby_abi is defined, this is
  Requires: ruby(abi) = mock
* when ruby_abi is not defined, this is a error.
Comment 22 Kostas Georgiou 2007-05-16 12:01:04 EDT
You are right of course, %{ruby_abi} is always defined anyway from this:
%{!?ruby_abi: %define ruby_abi %(...)} when ruby is not found it is defined
as "".

If we want a work around for mock then this should do it (I did checked mock
this time)
%if "%{ruby_abi}" == ""
%define ruby_abi mock (or 1.8)
%endif

Maybe it's more pain than it's worth, I am not sure I like this solution either.
Any preferences?
Comment 23 Mamoru TASAKA 2007-05-16 12:36:28 EDT
IMO for now writing 1.8 explicitly is the simplest.
This is only a workaround by the time rpmbuild supports
ruby abi detection and for now I can't find a smart
solution......
Comment 24 Kostas Georgiou 2007-05-18 11:11:59 EDT
New spec and rpm uploaded. I switched back to hard coding 1.8.
Comment 25 Mamoru TASAKA 2007-05-18 14:28:20 EDT
Okay.

------------------------------------------------
  This package (ruby-shadow) is APPROVED by me
------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account". 

At a stage, you will submit a request which tells sponsor
members that  you need a sponsor. After you do so, please let me know
on this bug for confirmation. Then I will sponsor you.

Note: now Fedora 8 branch is created, the valid branches
on http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure
is:
FC-5, FC-6, F7 and devel.

If you have some questions, please let me know.
Comment 26 Kostas Georgiou 2007-05-18 16:09:29 EDT
It seems that you approved me already while bugzilla was down, thanks. Let me
have a look at the docs and I'll fill the template for the branches.
Comment 27 Kostas Georgiou 2007-05-19 18:52:10 EDT
New Package CVS Request
=======================
Package Name: ruby-shadow
Short Description: Ruby bindings for shadow password access
Owners: k.georgiou@imperial.ac.uk
Branches: FC-5 FC-6 F7 devel
InitialCC: k.georgiou@imperial.ac.uk
Comment 28 Mamoru TASAKA 2007-05-24 10:17:19 EDT
Please try to rebuild this package on koji and plague
buildsys.
Comment 29 Kostas Georgiou 2007-05-24 10:42:32 EDT
Sorry I was away until last night, I'll have a go today.
Comment 30 Kostas Georgiou 2007-05-24 19:39:21 EDT
I am a bit lost with this failure
http://koji.fedoraproject.org/koji/getfile?taskID=16950&name=build.log

Notice the following:
rm -rf /var/tmp/ruby-shadow-1.4.1-5.fc7-root-kojibuilder
...
/usr/bin/install: cannot create regular file
`/var/tmp/ruby-shadow-1.4.1-5.fc7-root-kojibuilder/usr/lib/ruby/site_ruby/1.8/powerpc-linux':
File exists

Any ideas?
Can I start another build without increasing the spec release number?
Comment 31 Mamoru TASAKA 2007-05-24 23:42:34 EDT
I saw the build log and it seems that this is a parallel
make problem.

Try to remove %{_smp_mflags} from
-----------------------------------------------------------
make %{?_smp_mflags} DESTDIR=%{buildroot} install
-----------------------------------------------------------
You have to bump release.
Comment 32 Kostas Georgiou 2007-05-25 12:36:11 EDT
Yes everything works fine now, all packages are build now.
Comment 33 Mamoru TASAKA 2007-05-25 12:45:06 EDT
Good to hear!

Now closing as NEXTRELEASE.
Comment 34 Kostas Georgiou 2008-04-28 18:39:45 EDT
Package Change Request
======================
Package Name: ruby-shadow
New Branches: EL-5
Comment 35 Kevin Fenzi 2008-04-29 11:12:52 EDT
cvs done.

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