Fwd: Re: ruby 1.8.7.x for rawhide
Jim Meyering
jim at meyering.net
Wed Jun 23 08:48:57 UTC 2010
Mamoru Tasaka wrote:
> 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.
Sorry, but I have to disagree.
I find that the benefit in having identical source trees
(independent of build arch) far outweighs the cost of
a tiny -- and clear, imho -- patch to mkconfig.rb.
Also, my patch is slightly more maintainable.
Imagine building on some new foo64 or arm64 architecture...
With the /64$/ regexp in my patch, no change is required.
However, with the enumerated list in %ifarch, you would have
to add the new arch name to the list.
> 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.
It is not merely because rpmlint complains.
It is because making the canonical, patched sources differ
depending on the target architecture is truly a poor practice.
I think rpmlint's objection is very well justified.
With a conditionally-patched build directory, I cannot
build both 32-bit and 64-bit versions of the code from
the same patched source tree.
IMHO, using conditional patches is an abuse of RPM functionality
and would require far more justification than is possible in this case.
More information about the ruby-sig
mailing list