Bug 532306 - Review Request: rubygem-ruby-debug - Faster implementation of the standard Debugging
Summary: Review Request: rubygem-ruby-debug - Faster implementation of the standard De...
Keywords:
Status: CLOSED DUPLICATE of bug 630481
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 533725
Blocks: 566536
TreeView+ depends on / blocked
 
Reported: 2009-11-01 13:07 UTC by Jeroen van Meeuwen
Modified: 2010-09-05 20:55 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2010-09-05 20:55:28 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
Patch for ruby-debug to use system-wide -base and test fix (4.71 KB, patch)
2009-12-20 20:06 UTC, Mamoru TASAKA
no flags Details | Diff

Description Jeroen van Meeuwen 2009-11-01 13:07:32 UTC
Spec URL: http://www.kanarip.com/custom/SPECS/rubygem-ruby-debug.spec
SRPM URL: http://www.kanarip.com/custom/f12/SRPMS/rubygem-ruby-debug-0.10.3-1.fc12.src.rpm
Description: Faster implementation of the standard debug.rb using a
native extension with a new hook Ruby C API.

rpmlint is silent but for an empty file and some % in the filenames (it says "wrong macro")

koji scratch builds:
- dist-f11-updates-candidate: https://koji.fedoraproject.org/koji/taskinfo?taskID=1780993
- dist-f12-updates-candidate: https://koji.fedoraproject.org/koji/taskinfo?taskID=1780998
- dist-f13: https://koji.fedoraproject.org/koji/taskinfo?taskID=1781003
- dist-5E-epel: https://koji.fedoraproject.org/koji/taskinfo?taskID=1781006

Comment 1 Mamoru TASAKA 2009-11-02 17:45:05 UTC
- Please create one srpm per one gem.
- Also, you must not strip rebuilt binaries and create debuginfo rpm
  correctly. Please refer to:
  https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Gem_with_extension_libraries_written_in_C
- Also unless unavoided I don't think to add || : for test results.
  If test fails, you should debug it first.

Comment 2 Jeroen van Meeuwen 2009-11-02 18:51:46 UTC
(In reply to comment #1)
> - Please create one srpm per one gem.

These gems do not distribute very well separately, they have the same upstream and one requires the other in a %{version}-%{release} specific manner.

> - Also, you must not strip rebuilt binaries and create debuginfo rpm
>   correctly. Please refer to:
>  
> https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Gem_with_extension_libraries_written_in_C

I don't know how to resolve the inclusion of a reference to the buildroot in the binary blob (I did know how to make it disappear in the included Makefile, but not the blob):

"""
+ /usr/lib/rpm/check-buildroot
Binary file /home/jmeeuwen/devel/rpmbuild/BUILDROOT/rubygem-ruby-debug-0.10.3-1.fc12.x86_64/usr/lib64/ruby/site_ruby/1.8/x86_64-linux/ruby_debug.so matches
Found '/home/jmeeuwen/devel/rpmbuild/BUILDROOT/rubygem-ruby-debug-0.10.3-1.fc12.x86_64' in installed files; aborting
error: Bad exit status from /var/tmp/rpm-tmp.eZZmKv (%install)
"""

> - Also unless unavoided I don't think to add || : for test results.
>   If test fails, you should debug it first.  

I've fiddled around with it a bit and it now seems OK.

Comment 3 Mamoru TASAKA 2009-11-03 16:52:54 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > - Please create one srpm per one gem.
> 
> These gems do not distribute very well separately, they have the same upstream
> and one requires the other in a %{version}-%{release} specific manner.

- Ah, I checked two gems and actually dependency is in loop...


> > - Also, you must not strip rebuilt binaries and create debuginfo rpm
> >   correctly. Please refer to:
> >  
> > https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Gem_with_extension_libraries_written_in_C
> 
> I don't know how to resolve the inclusion of a reference to the buildroot in
> the binary blob (I did know how to make it disappear in the included Makefile,
> but not the blob):
> 
> """
> + /usr/lib/rpm/check-buildroot
> Binary file
> /home/jmeeuwen/devel/rpmbuild/BUILDROOT/rubygem-ruby-debug-0.10.3-1.fc12.x86_64/usr/lib64/ruby/site_ruby/1.8/x86_64-linux/ruby_debug.so
> matches
> Found
> '/home/jmeeuwen/devel/rpmbuild/BUILDROOT/rubygem-ruby-debug-0.10.3-1.fc12.x86_64'
> in installed files; aborting
> error: Bad exit status from /var/tmp/rpm-tmp.eZZmKv (%install)
> """

- The above guideline is actually for resolving this issue.
  The reason check-buildroot fails is that gem is installed under
  %buildroot directly. Also find-debuginfo rpm expects that
  compilation of C codes is done under %_builddir.
  Please follow this lines:
---------------------------------------------------------------
#  First, %prep stage must contain %setup -q -c -T to create the directory where C libraries are compiled.
# Then at %build stage the Ruby Gem must be installed under the directory created at %prep stage to get C libraries compiled under there. 
---------------------------------------------------------------
  For example:
  http://cvs.fedoraproject.org/viewvc/rpms/rubygem-hpricot/devel/rubygem-hpricot.spec?view=co

Comment 4 Jeroen van Meeuwen 2009-11-04 12:40:05 UTC
I modified the procedure just a little bit, as I generally want to prevent installing anything in %prep; in this case, it works out just fine (I think).

New SPEC: http://www.kanarip.com/custom/SPECS/rubygem-ruby-debug.spec
New SRPM: http://www.kanarip.com/custom/f12/SRPMS/rubygem-ruby-debug-0.10.3-2.fc12.src.rpm

Comment 5 Mamoru TASAKA 2009-11-07 16:49:20 UTC
Well,
- Please package linecache first.
  %check fails without linecache and even without %check ruby-debug-base
  actually needs linecache (see ruby-debug-base-0.10.3/lib/ruby-debug-base.rb)
  http://koji.fedoraproject.org/koji/taskinfo?taskID=1794237
  ( Does your srpm actually builds for you? )

- "head" command or so is very dangerous when binary files exist.
  Actually
---------------------------------------------------------------
    61  # Fix anything executable that does not have a shebang
    62  for file in `find %{buildroot}/%{geminstdir} %{buildroot}/%{geminstdir2} -type f -perm /a+x`; do
    63      [ -z "`head -n 1 $file | grep \"^#!/\"`" ] && chmod -v 644 $file
    64  done
---------------------------------------------------------------
  changes the permission of ruby_debug.so to 0644 and currently
  debuginfo rpm is not correctly created.

- Executing %check under %buildroot is wrong for this package
  because "rake test" again tries to create ruby_debug.so under
  %buildroot%{geminstdir2}/ext/ because we move this file to
  %buildroot%ruby_sitearch
---------------------------------------------------------------
   853  Executing(%check): /bin/sh -e /var/tmp/rpm-tmp.5gRRsq
   860  + rake -f ../ruby-debug-0.10.3/Rakefile test
   865  gcc -shared -o ruby_debug.so breakpoint.o ruby_debug.o -L. -L/usr/lib -L.  -rdynamic -Wl,-export-dynamic    -lruby  -lpthread -lrt -ldl -lcrypt -lm   -lc
---------------------------------------------------------------
  %check must be done under %_builddir for this package.

Comment 6 Jeroen van Meeuwen 2009-11-08 16:36:07 UTC
(In reply to comment #5)
> Well,
> - Please package linecache first.
>   %check fails without linecache and even without %check ruby-debug-base
>   actually needs linecache (see ruby-debug-base-0.10.3/lib/ruby-debug-base.rb)
>   http://koji.fedoraproject.org/koji/taskinfo?taskID=1794237
>   ( Does your srpm actually builds for you? )
> 

It does build on my machine locally but then again I already have a linecache package; this package is part of a bunch of packages I'm working on. I'll be submitting rubygem-linecache for review asap, and create a dependency between the two reviews.

> - "head" command or so is very dangerous when binary files exist.
>   Actually
> ---------------------------------------------------------------
>     61  # Fix anything executable that does not have a shebang
>     62  for file in `find %{buildroot}/%{geminstdir}
> %{buildroot}/%{geminstdir2} -type f -perm /a+x`; do
>     63      [ -z "`head -n 1 $file | grep \"^#!/\"`" ] && chmod -v 644 $file
>     64  done
> ---------------------------------------------------------------
>   changes the permission of ruby_debug.so to 0644 and currently
>   debuginfo rpm is not correctly created.
> 

This should also filter on -name "*.rb", which I fixed.

> - Executing %check under %buildroot is wrong for this package
>   because "rake test" again tries to create ruby_debug.so under
>   %buildroot%{geminstdir2}/ext/ because we move this file to
>   %buildroot%ruby_sitearch
> ---------------------------------------------------------------
>    853  Executing(%check): /bin/sh -e /var/tmp/rpm-tmp.5gRRsq
>    860  + rake -f ../ruby-debug-0.10.3/Rakefile test
>    865  gcc -shared -o ruby_debug.so breakpoint.o ruby_debug.o -L. -L/usr/lib
> -L.  -rdynamic -Wl,-export-dynamic    -lruby  -lpthread -lrt -ldl -lcrypt -lm  
> -lc
> ---------------------------------------------------------------
>   %check must be done under %_builddir for this package.  

Fixed this too.

New SPEC: http://www.kanarip.com/custom/SPECS/rubygem-ruby-debug.spec
New SRPM: http://www.kanarip.com/custom/f12/SRPMS/rubygem-ruby-debug-0.10.3-3.fc12.src.rpm

Comment 7 Mamoru TASAKA 2009-11-14 18:29:54 UTC
(Just putting a note that I postpone reviewing this review request until
 bug 533725 is completed)

Comment 9 Mamoru TASAKA 2009-12-20 20:06:52 UTC
Created attachment 379509 [details]
Patch for ruby-debug to use system-wide -base and test fix

Ah...

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > - Please create one srpm per one gem.
> > 
> > These gems do not distribute very well separately, they have the same upstream
> > and one requires the other in a %{version}-%{release} specific manner.
> 
> - Ah, I checked two gems and actually dependency is in loop...

Well, I checked these two gems' dependency again and actually
ruby-debug-base does not require ruby-debug
( ruby-debug-base-0.10.3/test/base/base.rb has "require 'ruby_debug'"
  for example, but this is satisfied by "ruby_debug.so" in ruby-debug-base
  (and not by ruby-debug.rb in ruby-debug gem) )
Also "rake test" under ruby-debug-base succeeds without ruby-debug
gem. So still ruby-debug-base and ruby-debug should be packaged seperately.
(ruby-debug-base does not depend on ruby-debug but ruby-debug does depend
 on ruby-debug-base)

For ruby-debug, currently your spec file contains:
---------------------------------------------------------
    99  %check
   100  pushd %{_builddir}/%{name}-%{version}/%{geminstdir2}
   101  rake -f ../ruby-debug-0.10.3/Rakefile test
---------------------------------------------------------
This skips many tests, because ruby-debug Rakefile contains
---------------------------------------------------------
    27  CLI_TEST_FILE_LIST = FileList['test/cli/commands/unit/*.rb',
    28                                'test/cli/commands/*_test.rb', 
    29                                'test/cli/**/*_test.rb', 
    30                                'test/test-*.rb'] 
    61      t.test_files = CLI_TEST_FILE_LIST
----------------------------------------------------------
however many of these files cannot be detected if the working
directory is ./%{geminstdir2}, not ./%{geminstdir}.
"rake test" for ruby-debug should be done under .%{geminstdir}.
Note that when executing "rake test" correctly under ./%{geminstdir}
many tests fail and patch is needed (attached). After applying patch:
-----------------------------------------------------------
[tasaka1@localhost ruby-debug-0.10.3]$ rake test
(in /home/tasaka1/rpmbuild/Reviewing/rubygem-related/rubygem-ruby-debug/rubygem-ruby-debug-0.10.3-4.fc12.src/TMP2/DEBUGINSTDIR/gems/ruby-debug-0.10.3)
ruby_debug already installed, skipping creating shared library extension
/usr/bin/ruby -I"lib:./ext:./lib" "/usr/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader.rb" "test/base/base.rb" "test/base/binding.rb" "test/base/catchpoint.rb" 
Loaded suite /usr/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader
Started
.....
Finished in 0.003895 seconds.

5 tests, 30 assertions, 0 failures, 0 errors
/usr/bin/ruby -I"lib:./ext:./lib:./cli" "/usr/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader.rb" "test/cli/commands/unit/regexp.rb" "test/cli/commands/catchpoint_test.rb" "test/cli/commands/catchpoint_test.rb" "test/test-edit.rb" "test/test-info.rb" "test/test-source.rb" "test/test-annotate.rb" "test/test-ctrl.rb" "test/test-list.rb" "test/test-catch.rb" "test/test-save.rb" "test/test-info-thread.rb" "test/test-emacs-basic.rb" "test/test-quit.rb" "test/test-init.rb" "test/test-method.rb" "test/test-info-var.rb" "test/test-output.rb" "test/test-trace.rb" "test/test-setshow.rb" "test/test-help.rb" "test/test-hist.rb" "test/test-raise.rb" "test/test-breakpoints.rb" "test/test-finish.rb" "test/test-stepping.rb" "test/test-frame.rb" "test/test-condition.rb" "test/test-enable.rb" "test/test-display.rb" "test/test-dollar-0.rb" "test/test-break-bad.rb" "test/test-pm.rb" 
Loaded suite /usr/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader
Started
.........................Skipping method sig test
..........
Finished in 18.947538 seconds.

35 tests, 46 assertions, 0 failures, 0 errors
-----------------------------------------------------------
Currently only the first 5 tests are executed.

Comment 10 Jeroen van Meeuwen 2010-01-09 10:47:29 UTC
Would it be acceptable to ship these two packages from the same CVS module (and so with one .spec as well)?

Comment 11 Mamoru TASAKA 2010-01-09 11:46:37 UTC
I think 2 gems (-ruby-debug & -ruby-debug-base) should be packaged
into seperated srpms (and so CVS request should be done seperately).

Also I think seperating these 2 gems will make spec file more readable.

Comment 12 Jeroen van Meeuwen 2010-01-09 12:06:22 UTC
Fair enough, let's hold off on this one until we have rubygem-ruby-debug-base packaged/reviewed then

Comment 13 Mamoru TASAKA 2010-01-31 12:32:07 UTC
Would you intend to package rubygem-ruby-debug-base?

Comment 16 Jeroen van Meeuwen 2010-02-18 18:48:14 UTC
(In reply to comment #15)
> (In reply to comment #14)
> > 1) I cannot get this:
> >   - http://www.kanarip.com/custom/SPECS/rubygem-ruby-debug.spec
> > http://www.kanarip.com/custom/f12/SRPMS/rubygem-ruby-debug-0.10.3-4.fc12.src.rpm
> >   to work with this:
> >   - http://www.kanarip.com/custom/SPECS/rubygem-ruby-debug-base.spec
> > http://www.kanarip.com/custom/f12/SRPMS/rubygem-ruby-debug-base-0.10.3-5.fc12.src.rpm
> 
> - See my comment 9
> 

I don't see how rake test in rubygem-ruby-debug can succeed, when executed from the %{_builddir}/%{name}-%{version}/%{geminstdir}, without the ext/ directory from rubygem-ruby-debug-base in /usr/lib/ruby/gems/1.8/ruby-debug-0.10.3/

> > 2) I see no use for two separate packages
> - It makes maintenanse much easier.    

Actually it doesn't, as we have seen with the rails packages as well.

Comment 17 Jeroen van Meeuwen 2010-02-18 18:49:42 UTC
(In reply to comment #16)
> (In reply to comment #15)
> > - See my comment 9
> > 
> 
> I don't see how rake test in rubygem-ruby-debug can succeed, when executed from
> the %{_builddir}/%{name}-%{version}/%{geminstdir}, without the ext/ directory
> from rubygem-ruby-debug-base in /usr/lib/ruby/gems/1.8/ruby-debug-0.10.3/
> 

This should have said:

I don't see how rake test in rubygem-ruby-debug can succeed, when executed from
the %{_builddir}/%{name}-%{version}/%{geminstdir}, without the ext/ directory
from rubygem-ruby-debug-base in /usr/lib/ruby/gems/1.8/ruby-debug-0.10.3/ or %{_builddir}/%{name}-%{version}/%{geminstdir}

Comment 18 Mamoru TASAKA 2010-02-19 05:02:20 UTC
(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > - See my comment 9
> > > 
> > 
> > I don't see how rake test in rubygem-ruby-debug can succeed, when executed from
> > the %{_builddir}/%{name}-%{version}/%{geminstdir}, without the ext/ directory
> > from rubygem-ruby-debug-base in /usr/lib/ruby/gems/1.8/ruby-debug-0.10.3/
> > 
> 
> This should have said:
> 
> I don't see how rake test in rubygem-ruby-debug can succeed, when executed from
> the %{_builddir}/%{name}-%{version}/%{geminstdir}, without the ext/ directory
> from rubygem-ruby-debug-base in /usr/lib/ruby/gems/1.8/ruby-debug-0.10.3/ or
> %{_builddir}/%{name}-%{version}/%{geminstdir}    

- So please see my comment 9. Already patch attached.

(In reply to comment #16)
> > > 2) I see no use for two separate packages
> > - It makes maintenanse much easier.    
> 
> Actually it doesn't, as we have seen with the rails packages as well. 
- No. It is just on Fedora chain-build cannot be used for rails
  related packages other than rawhide branch. It is just Fedora's
  builder issue.
  Seperating srpm is always preferable, for example when security
  issue is found on actionpack, we just apply a patch against actionpack
  only and only release new actionpack. Recompiling rails releated
  packages all is just silly in such case.

Comment 19 Mamoru TASAKA 2010-09-05 20:55:28 UTC
I refreshed this review request as bug 630481, so closing this one.

*** This bug has been marked as a duplicate of bug 630481 ***


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