Fwd: Re: ruby 1.8.7.x for rawhide

[Mamoru Tasaka]

Jim Meyering wrote, at 06/23/2010 04:12 PM +9:00:
> Mamoru Tasaka wrote:
>
>> Jim Meyering wrote, at 06/23/2010 06:03 AM +9:00:
>>> Jim Meyering wrote:
>>>
>>>> Mohammed Morsi wrote:
>>>>
>>>>> On 06/22/2010 01:26 PM, Jim Meyering wrote:
>>>>>> David Lutterkort wrote:
>>>>>>
>>>>>>> On Tue, 2010-06-22 at 12:49 +0200, Jim Meyering wrote:
>>>>>>>
>>>>>>>> Mohammed Morsi wrote:
>>>>>>>>
>>>>>>>>> I added this patch to my specfile as well, bumped the release, and
>>>>>>>>> updated the changelog to include your feedback.
>>>>>>>>>
>>>>>>>> Hi again,
>>>>>>>>
>>>>>>>> rpmlint complains about this conditionally-applied patch:
>>>>>>>>
>>>>>>>>      %ifarch ppc64 s390x sparc64 x86_64
>>>>>>>>      %patch23 -p1
>>>>>>>>      %endif
>>>>>>>>
>>>>>>>> That patch (ruby-1.8.7-multilib.patch) is small enough that
>>>>>>>> I'll include it here, for reference:
>>>>>>>>
>>>>>>>> --- ruby-1.8.7-p249/mkconfig.rb.orig    2010-06-15 11:30:44.000000000 -0400
>>>>>>>> +++ ruby-1.8.7-p249/mkconfig.rb 2010-06-15 11:31:01.000000000 -0400
>>>>>>>> @@ -102,7 +102,7 @@
>>>>>>>>
>>>>>>>>     drive = File::PATH_SEPARATOR == ';'
>>>>>>>>
>>>>>>>> -prefix = '/lib/ruby/' + RUBY_VERSION.sub(/\.\d+$/, '') + '/' + RUBY_PLATFORM
>>>>>>>> +prefix = '/lib64/ruby/' + RUBY_VERSION.sub(/\.\d+$/, '') + '/' + RUBY_PLATFORM
>>>>>>>>
>>>>>>>> It is easy to perform the same task using an unconditional patch,
>>>>>>>> so I wrote this replacement:
>>>>>>>>
>>>>>>> Couldn't we just get the right value from the environment ? I haven't
>>>>>>> looked if it's already in one of the standard env vars that rpmbuild
>>>>>>> sets up, but if bad comes to worst, couldn't we just do 'export
>>>>>>> LIB_PREFIX=%{_libdir}' in the spec file and then
>>>>>>>
>>>>>>>            prefix = ENV["LIB_PREFIX"].gsub(%r{^/usr}, "") + "/ruby" + RUBY_VERSION.sub(/\.\d+$/, '') + '/' + RUBY_PLATFORM
>>>>>>>
>>>>>> Using an rpm variable like that is more maintainable than enumerating
>>>>>> 64-bit architectures or even using a regexp like /64$/ like I did.
>>>>>> However, while it would work when building via rpm tools, if someone
>>>>>> ever builds manually, they would have to know to set that envvar --
>>>>>> too easily missed or forgotten.
>>>>>
>>>>> How would anyone go about building this manually with this change
>>>>> included? AFAIK since this change is only in the patch that is pulled
>>>>> in via the spec / rpmbuild, the only way that it will appear is if
>>>>> someone is building ruby using the rpm tools.
>>>>
>>>> Hi Mo,
>>>>
>>>> I've seen it documented to run e.g., "make prep" for a project and
>>>> then cd into the directory that creates and "develop" (i.e., run make, etc.).
>>>> This is not a hypothetical case.
>>>>
>>>>>> However, we can do something similar that might be more robust:
>>>>>> run something like this from the spec file:
>>>>>>
>>>>>>        sed -i 's,/lib/ruby/,%{_whatever}/,' .../mkconfig.rb || exit 1
>>>>>>
>>>>>> But that is fragile in its own way.  If mkconfig.rb changes, the sed
>>>>>> regexp may fail to match or it may match in some new place, introducing
>>>>>> an unwanted change.
>>>>>
>>>>> Agreed, this global sed will most likely cause headaches in the future.
>>>>
>>>> If you're so concerned, and don't like the insurance below,
>>>> it's easy to make the sed regexp more specific.  The line it's
>>>> modifying is long and unlikely to change frequently.
>>>>
>>>>>> We could mitigate that risk by adding tests
>>>>>> that would fail if our preconditions stop being met:
>>>>>>
>>>>>>        # Ensure that the "sed" command below does the right thing.
>>>>>>        grep 'prefix = ./lib/ruby/. + RUBY_VERSION.sub' || exit 1
>>>>>>        test $(grep -c '/lib/ruby/') = 1 || exit 1
>>>>>>
>>>>>>        # Use /lib64/ruby/ on a 64-bit system.
>>>>>>        sed -i 's,/lib/ruby/,%{_whatever}/,' mkconfig.rb || exit 1
>>>>>
>>>>> This will also lead to more maintenance as if either of those
>>>>> conditions become true and cause rpmbuild to exit, we will need to
>>>>> figure out an alternative solution to this problem.
>>>>
>>>> That's the whole idea of this approach.
>>>> In the unlikely event that something important changes,
>>>> you absolutely do want to be forced to get involved.
>>>> You'd get an rpmbuild failure that has to be investigated,
>>>> rather than a silent change in behavior that someone would have
>>>> to debug farther away from the source.
>>>>
>>>> It is for the same reason that patches are applied using the
>>>> most strict setting, allowing no "fuzz".
>>>
>>> FYI, I've confirmed that this works for me:
>>>
>>> commit 8cc2823aec419d282dfe3a1b62ed46e7453adbed
>>> Author: Jim Meyering<meyering at redhat.com>
>>> Date:   Tue Jun 22 22:15:59 2010 +0200
>>>
>>>       remove mkconfig.rb-modifying patch(#23) altogether
>>>
>>>       - Remove patch 23 altogether.  Replace it with mkconfig.rb-modifying
>>>         code in %install.
>>>       - ruby-multilib.patch: Remove file.
>>>
>>> diff --git a/RHEL-6/ruby.spec b/RHEL-6/ruby.spec
>>> index 00e7c50..ad0c7db 100644
>>> --- a/RHEL-6/ruby.spec
>>> +++ b/RHEL-6/ruby.spec
>>> @@ -25,7 +25,7 @@
>>>
>>>    Name:		ruby
>>>    Version:	%{rubyver}%{?dotpatchlevel}
>>> -Release:	2%{?dist}
>>> +Release:	3%{?dist}
>>>    License:	Ruby or GPLv2
>>>    URL:		http://www.ruby-lang.org/
>>>    BuildRoot:	%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
>>> @@ -53,8 +53,6 @@ Source4:	irb.1
>>>    # http://lists.fedoraproject.org/pipermail/ruby-sig/2010-May/000096.html
>>>    Source100:	ruby-1.8-rev%{tk_using_svn_number}_trunk-ext_tk.tar.gz
>>>
>>> -# Patch23 is Fedora specific
>>> -Patch23:	ruby-multilib.patch
>>>    # Patch27 is not in the upstream VCS.
>>>    # This patch, while not technically required in the context of Rails,
>>>    # does seem like a useful addition: improved safety/compatibility:
>>> @@ -193,7 +191,6 @@ pushd %{name}-%{arcver}
>>>      find tk -type d -name \.svn | sort -r | xargs rm -rf
>>>    )
>>>
>>> -%patch23 -p1
>>>    %patch27 -p0
>>>    %patch29 -p1
>>>    %patch30 -p1
>>> @@ -360,6 +357,24 @@ popd
>>>    # done
>>>    cd ..
>>>
>>> +# Change /lib/ruby/ to /lib64/ruby/ on a 64-bit system
>>> +if test %{_lib} != lib; then
>>> +  d=$RPM_BUILD_DIR/%{name}-%{version}/%{name}-%{arcver}
>>> +  t=$d/mkconfig.rb
>>> +  # ruby-1.9.2 has this line in tool/mkconfig.rb:
>>> +  # prefix = "/lib/ruby/#{version}/#{arch}"
>>> +  # while ruby-1.8.7 has this in mkconfig.rb:
>>> +  # prefix = '/lib/ruby/' + RUBY_VERSION.sub(/\.\d+$/, '') + '/' + RUBY_PLATFORM
>>> +  # The following covers both:
>>> +  test -f $t || t=$d/tool/mkconfig.rb
>>> +  if grep '^prefix = ./lib/ruby/' $t; then
>>> +      sed -i 's,^\(prefix = .\)/lib/ruby/,\1%{_lib}/ruby/,' $t || exit 1
>>> +  else
>>> +      echo "unexpected content in $t"
>>> +      exit 1
>>> +  fi
>>> +fi
>>> +
>>>    # installing binaries ...
>>>    make -C $RPM_BUILD_DIR/%{name}-%{version}/%{name}-%{arcver} DESTDIR=$RPM_BUILD_ROOT install
>>>
>>> @@ -546,6 +561,11 @@ rm -rf $RPM_BUILD_ROOT
>>>    %doc tmp-ruby-docs/ruby-libs/*
>>>
>>>    %changelog
>>> +* Tue Jun 22 2010 Jim Meyering<meyering at redhat.com>   - 1.8.7.249-3
>>> +- Remove patch 23 altogether.  Replace it with mkconfig.rb-modifying
>>> +  code in %install.
>>> +- ruby-multilib.patch: Remove file.
>>> +
>>>    * Tue Jun 22 2010 Jim Meyering<meyering at redhat.com>   - 1.8.7.249-2
>>>    - Integrate addition of patch101 by Mohammed Morsi.  This fixes
>>>      the segv-inducing marshaling bug affecting rails-3.0.0.
>>>
>>> The above isn't quite accurate.
>>> You need this additional change (to the %changelog text!)
>>> in order to avoid a cryptic "Package already exists: %package debuginfo"
>>> error from rpmbuild:
>>>
>>> -  code in %install.
>>> +  code in "install", above.
>>>
>>> I guess that's rpmbuild's way of saying you must not refer
>>> to "%install" in a %changelog entry.
>>
>> Please avoid to use such comprecated sed craft. While I use sed craft
>> so many times, this type of craft makes it difficult for other persons
>> to understand what these lines are for.
>
> That is why I added nearly as many lines of comment as of code.
> Considering the size of the existing %install section and its
> code/comment ratio, I'm surprised that you think my addition would
> be difficult to understand.

Actually as I was the "Merge review" reviewer of ruby.spec I already
consider that %install section is already so complecated
(especially for handling documents to be installed, fixing encoding,
  shebang,....) and once switching from 1.8.6.x to 1.8.7.x is done,
I really consider to clean up %install section (if time allows).
With using ruby_1_8/ext/tk head, I had to modify %install section a bit,
but I took some time to understand what part of %install I had to modify....


> If understanding is the only issue, let me know if adding more
> comments would help.
>
>> If comprecated sed craft is needed, creating a patch is much appreciated.
>
> I presume you would prefer a patch, so I am discarding the above,
> leaving the patch I used before.  I cannot take David Lutterkort's
> suggestion to use the likes of %{_lib} without adding shell code
> (or fragile envvar settings) to %install.
>
> FYI, here is my ruby-multilib.patch (patch23):
>
> diff -ruN ruby-1.8.3.orig/mkconfig.rb ruby-1.8.3/mkconfig.rb
> --- ruby-1.8.7/mkconfig.rb	2008-06-06 12:39:57.000000000 +0200
> +++ ruby-1.8.7/mkconfig.rb	2010-06-21 11:17:13.839498249 +0200
> @@ -39,6 +39,7 @@ vars = {}
>   has_version = false
>   continued_name = nil
>   continued_line = nil
> +lib_64 = ''
>   File.foreach "config.status" do |line|
>     next if /^#/ =~ line
>     name = nil
> @@ -96,13 +97,21 @@ File.foreach "config.status" do |line|
>         v_others<<  v
>       end
>       has_version = true if name == "MAJOR"
> +
> +    # If the target architecture is one of the following,
> +    #   ppc64 s390x sparc64 x86_64
> +    # then use "lib64", not "lib" in prefix.
> +    if name == "target_cpu" and (/64"$/ =~ val or val == '"s390x"')
> +      lib_64 = '64'
> +    end
>     end
>   #  break if /^CEOF/
>   end
>
>   drive = File::PATH_SEPARATOR == ';'
>
> -prefix = '/lib/ruby/' + RUBY_VERSION.sub(/\.\d+$/, '') + '/' + RUBY_PLATFORM
> +prefix = "/lib#{lib_64}/ruby/" \
> +  + RUBY_VERSION.sub(/\.\d+$/, '') + '/' + RUBY_PLATFORM
>   print "  TOPDIR = File.dirname(__FILE__).chomp!(#{prefix.dump})\n"
>   print "  DESTDIR = ", (drive ? "TOPDIR&&  TOPDIR[/\\A[a-z]:/i] || " : ""), "'' unless defined? DESTDIR\n"
>   print "  CONFIG = {}\n"

I think
%ifarch <64 bit architecture>
<apply patches specific to 64 bit>
%endif
is preferable because
- it clearly shows what special treatment should be done on x86_64 or so
- can avoid unneeded codes in 32 bit architecture
- and avoid to increase the size of the needed patch (or sed craft) unneededly.
If we use %ifarch..., encoding "if name == "target_cpu"...... or so handling in
installed codes is unneeded.

Adding 32 bit <-> 64 bit handling in the installed codes is redundant unless
we support 32 bit <-> 64 bit ruby parallel install or so. Avoiding
%ifarch .... %endif just because "rpmlint complaints" is questionable.


Regards,
Mamoru