[Bug 486302] Review Request: parrot - Parrot Virtual Machine

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=486302





--- Comment #32 from Lubomir Rintel <lkundrak at v3.sk>  2009-04-19 09:16:21 EDT ---
Uff, this seems to need a lot of work to be accepted. Sorry the review took so
long.

1.) Don't require perl modules by package name:

BuildRequires:  perl-Test-Harness
Requires:       perl-Pod-Simple
...

This would be better:

BuildRequires:  perl(Test::Harness)
...

2.) Source1 is useless:

BuildRequires:  ed
Source1:        reduce_rpmlint_err.tar.gz

Do what the scripts do inline. It might be a lot easier to use sed instead of
ed.

3.) Subpackage buildrequire

%package docs
...
BuildRequires:  perl

%package tools
...
BuildRequires:  ed

Makes no sense. Please don't define BRs in subpackages.
Furthermore, perl is in default buildgroup, thus can be ommited.

4.) Why would tools require pkgconfig?

%package tools
...
Requires:       pkgconfig

5.) This always evaluates to false

if test "%{_vendor}" = "suse"

please remove it

6.) No architecture independence

%ifarch %{ix86} x86_64
...
%else
# PowerPC
...
%endif

Remove this, or explain.

7.) %{_smp_mflags} not used

make
make parrot_utils
make installable
make html

Either use them or explain why you don't.

8.) No useless comments please

#make install DESTDIR=$RPM_BUILD_ROOT
...
#rm -rf $RPM_BUILD_ROOT/%{_docdir}/parrot         # for Solaris?

9.)

10.) Don't strip anything

%{__strip} %{RPM_PAR_LIB_DIR}dynext/*.so

Remove this

11.) Bad comment

rm -rf $RPM_BUILD_ROOT%{_usr}/share/doc/parrot    # necessary for SuSE

Well, eh, SUSE?

12.) Use macros for directories where appropriate

    --lex=/usr/bin/flex \
    --yacc=/usr/bin/yacc \
...
rm -rf $RPM_BUILD_ROOT%{_usr}/share/doc/parrot    # necessary for SuSE

%{_bindir}, %{_datadir}, etc...

13.) Compiler generates .so-s executable

# Force permissions on shared versioned libs so they get stripped.
find $RPM_BUILD_ROOT%{_libdir} -type f -name '*.so.*' -exec chmod 755 {} \;

Is this necessary? Explain if yes.

14.) Enable test suite

# make test < /dev/null
# %{?_with_fulltest:make fulltest < /dev/null}
# make test || :
# %{?_with_fulltest:make fulltest || :}

15.) Comment patches and submit them upstream?

Patch0:         parrot-install_files.patch

What does this do? Was it submitted upstream?

16.) Provides not sane

parrot-tools:

perl(DB)
Perl debugger? I guess not.

perl(File::Which) = 0.05
You embed File::Which in /usr/lib/parrot/1.0.0/tools/lib/File/Which.pm
You should not. Depend on perl-File-Which package instead.

parrot-docs:

perl(A)  
perl(B)  

You should not provide these.

17.) Doc shouldn't depend on perl:
parrot-docs requires these:

perl(strict)  
perl(warnings)

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.