Bug 226381

Created attachment 153401 [details]
missing directories list

For 1.8.6-2:

0. File entry:
* Static library
  - Is /usr/lib/libruby-static.a needed? If so, can this be split
    into -static rpm?
    Note: If this static archive cannot be removed, splitting into
	  different subpackage is too late as F7T4 is frozen.

* Duplicate file entry
  - Please check the following.
--------------------------------------------------------------------
[tasaka1@localhost ruby]$ rpm -qlp /var/lib/mock/ruby-*i386*rpm | grep -v debug
| sort | uniq -d
/usr/lib/ruby
/usr/lib/ruby/1.8
/usr/share/ri/1.8/system/IRB/Context/irb_level-i.yaml
/usr/share/ri/1.8/system/IRB/ExtendCommand/Nop/irb-i.yaml
/usr/share/ri/1.8/system/IRB/ExtendCommandBundle/irb_context-i.yaml
/usr/share/ri/1.8/system/IRB/ExtendCommandBundle/irb_exit-i.yaml
/usr/share/ri/1.8/system/IRB/ExtendCommandBundle/irb_load-i.yaml
/usr/share/ri/1.8/system/IRB/ExtendCommandBundle/irb_original_method_name-c.yaml

/usr/share/ri/1.8/system/IRB/ExtendCommandBundle/irb_require-i.yaml
/usr/share/ri/1.8/system/IRB/IrbLoader/irb_load-i.yaml
/usr/share/ri/1.8/system/IRB/JobManager/irb-i.yaml
/usr/share/ri/1.8/system/IRB/JobManager/main_irb-i.yaml
/usr/share/ri/1.8/system/IRB/irb-c.yaml
/usr/share/ri/1.8/system/IRB/irb_abort-c.yaml
/usr/share/ri/1.8/system/IRB/irb_exit-c.yaml
--------------------------------------------------------------------

* Missing directory ownership
  - Too many, the result is attached. 
    1447 directories are not owned by any package.
--------------------------------------------------------------------
( for f in `rpm -qlp /var/lib/mock/ruby-*i386*rpm | grep -v debug` ; do dirname
$f ; done ) | sort | uniq | LANG=C xargs rpm -qf | grep package
--------------------------------------------------------------------
    NOTE: Due to this, on my system many files related to ruby cannot be
	  seen by non-root users. Several ruby modules package I currently
	  submit for review request fail to be rebuilt.

* From build log:
  - For ruby-libs:
--------------------------------------------------------------------
warning: File listed twice: /usr/lib/ruby
warning: File listed twice: /usr/lib/ruby/1.8
warning: File listed twice: /usr/lib/ruby/site_ruby
warning: File listed twice: /usr/lib/ruby/site_ruby/1.8
warning: File listed twice: /usr/lib/ruby/site_ruby/1.8/i386-linux
---------------------------------------------------------------------

A. rpmlint - attached
Summary :
  - doc-file-dependency
    Files included as %doc with executable permission adds additional
    dependency to its package. This is generally avioded and documentations
    should not have executable permission.

  - non-executable-script
    Generally "source"d scripts should not have shebangs. It there is some
    exceptions for ruby package, please explain why.

  - spurious-executable-perm
    Same as "doc-file-dependency"

  - wrong-script-interpreter
    Setting interpreter the files under /usr/local is wrong.

Z. Question
* Automatically set CFLAGS
  - One strange thing for me is that fedora ruby rpms sets CFLAGS
    as fedora specific compilation flags by default.

    For example, currently ruby-zoom I submitted (bug 237381) is under
    review. Then
    - Just unpack ruby-zoom-0.2.2.tar.gz
    - do cd ruby-zoom-0.2.2
    - explicitly unset CFLAGS
    - do ruby extconf.rb
    Then Makefile is created. This Makefile sets CFLAGS as fedora specific
    compilation flags by default. Is this a expected behavior?

Created attachment 153402 [details]
rpmlint log for ruby 1.8.6-2

One another note:
* /usr/lib/ruby/1.8/i386-linux/config.h
 - Including config.h as header files are generally considered
   as "very bad" because
   * the name is very generic and may cause name space conflicts
   * the definition #HAVE_???? macro also may cause conflicts
     when other config.h is included.
     Actually, the content of this file 
     (/usr/lib/ruby/1.8/i386-linux/config.h) seems very dangerous.

  * Also /usr/lib/ruby/1.8/i386-linux/defines.h seems dangerous.

Well, other than config.h and defines.h, there are
many header files of which name are too generic and
the contents are too dangerous 
under /usr/lib/ruby/1.8/i386-linux/
(such as env.h missing.h regex.h version.h....)

IMO the name, structure of the files under this directory
needs reconsideration.

That's the upstream matter. I'm not quite sure if upstream agrees with changing
them - at least it won't happens on the stable release - because a lot of the
extension libraries are following it.  If that's one of the requirements to pass
through this review, I don't think I can do anything at this moment.

I'll work on other packaging issue though.

Tagoh,
Will you please work on rpmlint message output on all RPMs and SRPM of this package?

lets first try to minimize rpmlint errors and warnings.

sorry for taking a long time for response. 1.8.6.111-3.fc9 should be somewhat
improved.

For 1.8.6.111-3.fc9:
(I maybe missing something else. I will recheck this)

? License
  - I always wonder about this.
    What does "Ruby" license mean? My understanding is that
    when we talk "this is licensed under the same license of
    Ruby", this means GPL+ and (Ruby original license), is this
    wrong?

    From http://www.gnu.org/licenses/license-list.html :
-----------------------------------------------------------
 License of Ruby
    This is a free software license, compatible with the GPL via an explicit
dual-licensing clause.
-----------------------------------------------------------

  - For ext/openssl:
    ext/openssl uses OpenSSL,
-----------------------------------------------------------
   978  gcc -shared -o ../../.ext/i386-linux/openssl.so ossl_x509name.o
ossl_pkey_dsa.o ossl_pkey_rsa.o ossl_x509store
.o ossl_x509revoked.o ossl_engine.o ossl_pkey_dh.o ossl_config.o ossl_pkcs7.o
ossl_bn.o openssl_missing.o ossl_pkcs12.
o ossl_cipher.o ossl_asn1.o ossl_bio.o ossl_ssl.o ossl.o ossl_hmac.o
ossl_x509crl.o ossl_x509ext.o ossl_x509req.o ossl
_ocsp.o ossl_pkey.o ossl_rand.o ossl_x509.o ossl_x509attr.o ossl_x509cert.o
ossl_digest.o ossl_ns_spki.o -L"." -L"../.
." -L.  -rdynamic -Wl,-export-dynamic    -lruby -lssl -lcrypto  -lpthread -ldl
-lcrypt -lm   -lc
   979  make[1]: Leaving directory
`/builddir/build/BUILD/ruby-1.8.6.111/ruby-1.8.6-p111/ext/openssl'
-----------------------------------------------------------
    However OpenSSL license is GPLv2 incompatible (I don't know
    for GPLv3). Is this part legally okay?

* SourceURL
  - Please don't comment out Source[1-3] full URL (rpmbuild
    support full URLs for not only Source0).
    (Or you just uncompressed and then recompressed again?
     If so, we want to use what is given from upstream directly
     as much as possible)

* Requires
  - Does not ruby-tcltk require ruby? At least
    /usr/lib/ruby/1.8/tkextlib/pkg_checker.rb (on i386) contains
-----------------------------------------------------------
     1  #!/usr/bin/env ruby
-----------------------------------------------------------

  ! For ruby-rdoc, "Requires: %{name} = %{version}-%{release}" is
    redundant as ruby-irb already requires it.
    (Same for ruby-ri).

* Consistent usage of macros
  - If you want to use %__mkdir_p, please also use %__rm, %__make,
    etc for consistency.

* Timestamps
  We want to keep timestamps as much as possible.
  A. Please use "-p" option when using "install" or "cp" commands.
  B. Please change:
-----------------------------------------------------------
@@ -181,7 +178,7 @@
   --disable-rpath \
   --with-ruby-prefix=%{_prefix}/lib
 
-make RUBY_INSTALL_NAME=ruby %{?_smp_mflags}
+make RUBY_INSTALL_NAME=ruby COPY="cp -p" %{?_smp_mflags}
 %ifarch ia64
 # Miscompilation? Buggy code?
 rm -f parse.o
-----------------------------------------------------------

! rpm -bi --short-circuit
  ! (Some reviewers perhaps don't mind this, however)
    rpm -bi --short-circuit fails when done more than twice:

    To pass multiple time short-circuit installation, the following
    seem to be needed.
-----------------------------------------------------------
@@ -206,6 +203,7 @@
 %endif
 
 # installing documents and exapmles...
+rm -rf tmp-ruby-docs
 mkdir tmp-ruby-docs
 cd tmp-ruby-docs
 
@@ -303,19 +297,20 @@
 
 # generate ri doc
 rubybuilddir=$RPM_BUILD_DIR/%{name}-%{version}/%{name}-%{arcver}
+rm -rf %{name}-%{arcver}/.ext/rdoc
 LD_LIBRARY_PATH=$RPM_BUILD_ROOT%{_libdir}
RUBYLIB=$RPM_BUILD_ROOT%{_libdir}/ruby/%{rubyxver}:$RPM_BUILD_ROO
T%{_libdir}/ruby/%{rubyxver}/%{_normalized_cpu}-%{_target_os} make -C
$rubybuilddir DESTDIR=$RPM_BUILD_ROOT 
install-doc
 #DESTDIR=$RPM_BUILD_ROOT LD_LIBRARY_PATH=$RPM_BUILD_ROOT%{_libdir}
$RPM_BUILD_ROOT%{_bindir}/ruby -I $rubyb
uilddir -I
$RPM_BUILD_ROOT%{_libdir}/ruby/%{rubyxver}/%{_normalized_cpu}-%{_target_os}/ -I
$rubybuilddir/lib
 $RPM_BUILD_ROOT%{_bindir}/rdoc --all --ri-system $rubybuilddir
-----------------------------------------------------------

! Argument list too long.
  - When I try normal rpmbuild as my default user, it stops with
    "Argument list too long" error.
-----------------------------------------------------------
@@ -278,7 +276,7 @@
 done
 
 # make sure that all doc files are the world-readable
-find -type f | xargs chmod 0644
+find -type f -exec chmod 0644 {} ';'
 
 # convert to utf-8
 for i in `find -type f`; do
-----------------------------------------------------------

* Some seemingly redundant scripts
  - Are the following parts needed?
------------------------------------------------------------
@@ -286,10 +284,6 @@
        if [ $? != 0 ]; then
                iconv -f iso8859-1 -t utf-8 $i > $.new && mv $i.new $i || exit 1
        fi
-       if [ -f $i.new ]; then
-               echo "Failed to convert with iconv."
-               exit 1
-       fi
 done
@@ -338,7 +333,6 @@
 
 %clean
 rm -rf $RPM_BUILD_ROOT
-rm -rf tmp-ruby-docs
 
 %post libs -p /sbin/ldconfig
 
-------------------------------------------------------------
  - Also, are there any reasons you prefer tar x"v"f? (for %setup
    we usually suppress verbose output).

* Duplicate directories:
  - On i386, build.log says:
--------------------------------------------------------------
 24078  + cp -pr ruby-1.8.6-p111/LEGAL
/var/tmp/ruby-1.8.6.111-3.fc9-root-mockbuild/usr/share/doc/ruby-libs-1.8.6.111
 24079  + cp -pr ruby-1.8.6-p111/LGPL
/var/tmp/ruby-1.8.6.111-3.fc9-root-mockbuild/usr/share/doc/ruby-libs-1.8.6.111
 24080  + exit 0
 24081  warning: File listed twice: /usr/lib/ruby
 24082  warning: File listed twice: /usr/lib/ruby/1.8
 24083  warning: File listed twice: /usr/lib/ruby/site_ruby
 24084  warning: File listed twice: /usr/lib/ruby/site_ruby/1.8
 24085  warning: File listed twice: /usr/lib/ruby/site_ruby/1.8/i386-linux
 24086  Provides: bigdecimal.so bubblebabble.so cparse.so curses.so dbm.so
digest.so dl.so enumerator.so etc.so fcntl.so gdbm.so iconv.so libruby =
1.8.6.111-3.fc9 libruby.so.1.8 md5.so nkf.so openssl.so pty.so readline.so
rmd160.so ruby(abi) = 1.8 sdbm.so sha1.so sha2.so socket.so stringio.so
strscan.so syck.so syslog.so thread.so wait.so zlib.so
 24087  Requires(interp): /sbin/ldconfig /sbin/ldconfig
--------------------------------------------------------------
     IMO the following is better.
--------------------------------------------------------------
@@ -384,12 +378,14 @@
 %doc %{name}-%{arcver}/GPL
 %doc %{name}-%{arcver}/LEGAL
 %doc %{name}-%{arcver}/LGPL
-%dir %{_libdir}/ruby
 %dir %{_prefix}/lib/ruby
-%dir %{_libdir}/ruby/%{rubyxver}
 %dir %{_prefix}/lib/ruby/%{rubyxver}
-%dir %{_libdir}/ruby/%{rubyxver}/%{_normalized_cpu}-%{_target_os}
+%ifnarch i386 ppc
+%dir %{_libdir}/ruby
+%dir %{_libdir}/ruby/%{rubyxver}
 %{sitedir}
+%endif
+%dir %{_libdir}/ruby/%{rubyxver}/%{_normalized_cpu}-%{_target_os}
 %{sitedir2}
 ## the following files should goes into ruby-tcltk package.
 %exclude %{_prefix}/lib/ruby/%{rubyxver}/*tk.rb
--------------------------------------------------------------

By the way, is it possible to remove config.h or at least rename
it?

(In reply to comment #8)
> For 1.8.6.111-3.fc9:
> (I maybe missing something else. I will recheck this)
> 
> ? License
>   - I always wonder about this.
>     What does "Ruby" license mean? My understanding is that

See http://fedoraproject.org/wiki/Licensing for what we should describe in .spec.

>     when we talk "this is licensed under the same license of
>     Ruby", this means GPL+ and (Ruby original license), is this
>     wrong?

No, Ruby License is actually different one to GPL. Ruby is just licensed under
Ruby License or GPL. they just can choose one of them.

>     However OpenSSL license is GPLv2 incompatible (I don't know
>     for GPLv3). Is this part legally okay?

AFAIUC if someone who wants to use OpenSSL on Ruby, they need to apply Ruby
License instead of GPL. if necessary, I'll package it separately.

> * SourceURL
>   - Please don't comment out Source[1-3] full URL (rpmbuild
>     support full URLs for not only Source0).
>     (Or you just uncompressed and then recompressed again?
>      If so, we want to use what is given from upstream directly
>      as much as possible)

That was odd to reduce the package size. Source1 is already dead link. I'll be
back to the upstream one if it's preferred.

> * Requires
>   - Does not ruby-tcltk require ruby? At least
>     /usr/lib/ruby/1.8/tkextlib/pkg_checker.rb (on i386) contains

Thanks for catching this up. apparently it doesn't need to be shipped. and
basically any libraries there isn't supposed to be run directly. otherwise those
should be moved under /usr/bin. the main reason that not requiring ruby in
general is, they could be used without ruby, but with mod_ruby and another ruby
implementation etc.

> ! Argument list too long.
>   - When I try normal rpmbuild as my default user, it stops with
>     "Argument list too long" error.

I don't see this one. xargs is supposed to do that. isn't it your shell bug?

Otherwise will be fixed in next build.

(In reply to comment #9)
> By the way, is it possible to remove config.h or at least rename
> it?

No, it isn't, as I said before.

Well, I have not checked 1.8.6.111-4.fc9 yet, however

- (at least on i386) using have_header like
----------------------------------------------------------
unless have_header("magick/MagickCore.h")
  exit_failure "Can't install RMagick #{RMAGICK_VERS}. Can't find MagickCore.h.\n"
else
  headers << "magick/MagickCore.h"
end
----------------------------------------------------------

returns
-----------------------------------------------------------
have_library: checking for InitializeMagick() in -lMagick... -------------------- no

"gcc -o conftest -I. -I/usr/lib/ruby/1.8/i386-linux
-I/home/tasaka1/rpmbuild/BUILD/RMagick-2.0.0/ext/RMagick -fopenmp   -O2 -g -pipe -W
all -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unw
ind-tables -fopenmp   conftest.c  -L"." -L"/usr/lib" -lWand -lMagick     -lWand
-lMagick    -lruby-static -lMagick  -lpthread -ldl -lcr
ypt -lm   -lc"
/usr/bin/ld: cannot find -lruby-static
collect2: ld returned 1 exit status
checked program was:
/* begin */
 1: #include <assert.h>
 2: #include <ctype.h>
 3: #include <stdint.h>
 4: #include <stdio.h>
 5: #include <stdlib.h>
 6: #include <math.h>
 7: #include <time.h>
 8: #include <sys/types.h>
 9: #include <magick/MagickCore.h>
10: 
11: /*top*/
12: int main() { return 0; }
13: int t() { void ((*volatile p)()); p = (void ((*)()))InitializeMagick; return
0; }
/* end */
------------------------------------------------------------

It seems that this is because /usr/lib/ruby/1.8/i386-linux/rbconfig.rb
(on i386) contains
-------------------------------------------------------------
   126    CONFIG["LIBRUBY_A"] = "lib$(RUBY_SO_NAME)-static.a"
   131    CONFIG["LIBRUBYARG_STATIC"] = "-l$(RUBY_SO_NAME)-static"
-------------------------------------------------------------

Any ideas?

Why does RMagick link to the static library? the above config is to see the
static library file name. I don't see the strong reason if it's linking the
static library. -- or does it fall back to the shared library if the above
config is gone? anyway, the priority should be changed to look for the shared
library first, though.

Well, it was not 'have_header' but 'have_library'. The actual example is:

When I write TEMP.rb as:
--------------------------------------------------------------
require 'mkmf'

have_library('z')
---------------------------------------------------------------
and try "ruby TEMP.rb", it returns
---------------------------------------------------------------
$ LANG=C ruby TEMP.rb
checking for main() in -lz... no
$ cat mkmf.log 
have_library: checking for main() in -lz... -------------------- no

"gcc -o conftest -I. -I/usr/lib/ruby/1.8/i386-linux -I.  -O2 -g -pipe -Wall
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4
-m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -Wall  -fPIC
conftest.c  -L"." -L"/usr/lib" -L.  -rdynamic -Wl,-export-dynamic    
-lruby-static -lz  -lpthread -ldl -lcrypt -lm   -lc"
/usr/bin/ld: cannot find -lruby-static
collect2: ld returned 1 exit status
checked program was:
/* begin */
1: /*top*/
2: int main() { return 0; }
3: int t() { void ((*volatile p)()); p = (void ((*)()))main; return 0; }
/* end */

"gcc -o conftest -I. -I/usr/lib/ruby/1.8/i386-linux -I.  -O2 -g -pipe -Wall
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4
-m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -Wall  -fPIC
conftest.c  -L"." -L"/usr/lib" -L.  -rdynamic -Wl,-export-dynamic    
-lruby-static -lz  -lpthread -ldl -lcrypt -lm   -lc"
/usr/bin/ld: cannot find -lruby-static
collect2: ld returned 1 exit status
checked program was:
/* begin */
1: /*top*/
2: int main() { return 0; }
3: int t() { main(); return 0; }
/* end */

--------------------

--------------------------------------------------------------

I guess some fix is needed on ruby rpm side.

libruby-static.a is back now btw.

Sorry for delay... I will recheck the newest ruby this weekend.

Created attachment 292286 [details]
Directory check list of ruby HEAD

Created attachment 292287 [details]
License check list of ruby HEAD

For ruby-1.8.6.111-7.fc9:

* License:
  Please check the license check list attached.
  SUMMARY:
  - From file "COPYING", the license of ruby is "GPLv2 or Ruby",
    not "GPL+ or Ruby".

  - Maybe ruby-rdoc license should be "(GPLv2 or Ruby) and CC-BY"?

  - I guess that mdoc2man.rb (problematic, BSD with advertising)
    is not used anywhere, however would you confirm that?

* Timestamps
  - Please fix the following line to use "cp -p".
--------------------------------------------------------------------------
   314	cp misc/*.el $RPM_BUILD_ROOT%{_datadir}/emacs/site-lisp/ruby-mode
--------------------------------------------------------------------------

* Directory ownership issue
  (Check the list if the result is as you desire).
  - The directory
    %{_libdir}/ruby/%{rubyxver}/%{_normalized_cpu}-%{_target_os}
    should be owned by also non-64-bits archs.

Other thing seems okay.

ping

Can I recheck the newest rawhide ruby again?

requested by Jens Petersen (#27995)

I have taken over this package.

Created attachment 335385 [details]
License check list again

For 1.8.6.287-6:

* License
  - As attached, please change the license tag as below:
    ruby-libs -> (GPLv2 or Ruby) and (GPL+ or ASL 1.0)
    ruby-tcltk -> (GPLv2 or Ruby) and TCL
    ruby-rdoc -> (GPLv2 or Ruby) and CC-BY
    Others -> GPLv2 or Ruby

* Source2
  - is found at:
    http://elbereth-hp.hp.infoseek.co.jp/files/ruby/refm/old/2005/list.html

* 64 bits specification
-------------------------------------------------------
%ifarch ppc64 s390x sparc64 x86_64
%patch22 -p1
%patch23 -p1
-------------------------------------------------------
  - Now the following may be better:
-------------------------------------------------------
%if %{__isa_bits} == 64
-------------------------------------------------------

* %check
  - "make test" seems to succeed even on ppc64...
    http://koji.fedoraproject.org/koji/taskinfo?taskID=1244350
    

* Emacs related directory
  - From emacs-22.3-3 /etc/rpm/macros.emacs (in emacs-common) defines
    %_emacs_sitelispdir %_emacs_sitestartdir. So from F-11
    using these macros is preferred, like:
-------------------------------------------------------
%{!?_emacs_sitelispdir: %global _emacs_sitelispdir /usr/share/emacs/site-lisp}
-------------------------------------------------------
    or so (use %global)

* Convering files into UTF-8
  - There are lots of files named .new left so these should be removed
    (the last " || exit 1" should be "|| rm -f $i.new")

  ! Note that when changing the encoding of HTML files: 
    - usually just changing the encoding to UTF-8 is not enough and
      also changing "charset=foo" item in the HTML files is needed.

(In reply to comment #23)
> For 1.8.6.287-6:
> 
> * License
>   - As attached, please change the license tag as below:
>     ruby-libs -> (GPLv2 or Ruby) and (GPL+ or ASL 1.0)

The correct one is: (GPLv2 or Ruby) and (GPL+ or Artistic)

Jeroen, ping?

Jeroen, would you also update this package? At least
lots of .new files must be removed.

How does such a snippet look to you?

==
for i in `find -type f ! -name "*.gif"`; do
    sh -c "
        iconv -f utf-8 -t utf-8 $i > /dev/null 2>&1 || \
            (
                iconv -f euc-jp -t utf-8 $i > $i.new \
                    && mv $i.new $i \
                    || rm -f $i.new
            )

        if [ $? != 0 ]; then
            iconv -f iso8859-1 -t utf-8 $i > $.new \
                && mv $i.new $i \
                || rm -f $i.new
        fi
    "
done
==

Sorry for delay.
With the scriptlets in your comment 27:

------------------------------------------------------
            (
                iconv -f euc-jp -t utf-8 $i > $i.new \
                    && mv $i.new $i \
                    || rm -f $i.new
            )
------------------------------------------------------
will always exit with 0 (unless "rm -f" fails with some _very_
strange reason) and the latter 
------------------------------------------------------
        if [ $? != 0 ]; then
            iconv -f iso8859-1 -t utf-8 $i > $.new \
                && mv $i.new $i \
                || rm -f $i.new
        fi
------------------------------------------------------
will never be executed. i.e. the first "exit 1" before )
need not be changed, the latter "|| exit 1" has to be
changed to "|| rm -f $i.new" as you wrote.

Well, my comment 28 is wrong, and the current ruby.spec is
wrong in a different way. Now I am doing some trial for this part.

In the previous (-7) spec file:

---------------------------------------------------------
for i in `find -type f ! -name "*.gif"`; do
    sh -c "iconv -f utf-8 -t utf-8 $i > /dev/null 2>&1 || (iconv -f euc-jp -t utf-8 $i > $i.new && mv $i.new 
$i || exit 1)
    if [ $? != 0 ]; then
        iconv -f iso8859-1 -t utf-8 $i > $.new && mv $i.new $i || exit 1
    fi"
done
---------------------------------------------------------

Here $? is always 0 like below:
---------------------------------------------------------
$ sh -c "LANG=C ; foo bar ; echo $?"
sh: foo: command not found
0
$ sh -c 'LANG=C ; foo bar ; echo $?'
sh: foo: command not found
127
---------------------------------------------------------
With double quotation shell first evaluates the value of $?
and expands it before actually executing sub-shell. However
as we also want to use $i, we cannot easily change " to '.

So I changed
- not to use subshell
- from ( to {
---------------------------------------------------------
$ sh -c 'status=0 ; { status=1 ; } ; echo $status'
1
$ sh -c 'status=0 ; ( status=1 ) ; echo $status'
0
---------------------------------------------------------
- and not to "exit 1" but to use "status=1"

And I addressed all I mentioned in my comment 23 (except for
64 bits handling)

Closing this Merge Review

Setting the fedora-review flag to '+' as that seems to have been missed.