[Bug 478931] Review Request: globus-rls-server - Globus Toolkit - Replica Location Service Server

Fri Jun 5 12:27:08 UTC 2009

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=478931

--- Comment #4 from Mattias Ellert <mattias.ellert at fysast.uu.se>  2009-06-05 08:27:07 EDT ---
(In reply to comment #3)
> This one was quite different from the other globus packages. Here is my review:
> 
> - package builds in koji rawhide
>    http://koji.fedoraproject.org/koji/taskinfo?taskID=1394353
> 
> ? I don't know if this
>    BuildRequires:  globus-gssapi-gsi-devel >= 4
> is really required, since
>    $ grep -rl gssapi *
>    pkgdata/pkg_data_src.gpt.in
> Is it an upstream error or am I missing something?

Yes, this is a bogus requirement stated in the upstream package description
file - fixed (patch updated too)

> * Afaik %setup is supposed to be used only once in a specfile. So
>    %setup -q -n %{_name}-%{version}
>    %setup -D -T -q -n %{_name}-%{version} -a 1
> should be replaced by
>    %setup -q -n %{_name}-%{version} -a 1

You can have as many %setup lines you want as long as all except the first one
has a -D flag in order not the trigger the deletion of already unpacked sources

Quoting http://www.rpm.org/max-rpm/s1-rpm-specref-macros.html

"The -D option is used to direct %setup to not delete the build directory prior
to unpacking the sources. This option is used when more than one source archive
is to be unpacked into the build directory, normally with the -b or -a
options."

> ? Actually, why are you not making globus_rls_server_setup a package on its
> own?

See:

https://fedoraproject.org/wiki/PackagingDrafts/Globus#Setup_packages

and

https://fedoraproject.org/wiki/PackagingDrafts/Globus#Globus_package_that_provides_both_a_library_and_programs_and_that_has_a_corresponding_setup_package

The example above is from globus-common which already does the same (packaging
globus-common and globus-common-setup in the same SRPM).

> ! It would be good to briefly explain what Source{1,2,3} are for where you
> declare them.

Comments added.

> * rpmlint complains
>    globus-rls-server.x86_64: E: wrong-script-interpreter
> /usr/share/globus/setup/SXXrls.in "@SHELL@"
>    globus-rls-server.x86_64: E: non-executable-script
> /usr/share/globus/setup/SXXrls.in 0644
>    globus-rls-server.x86_64: E: non-readable /etc/globus-rls-server.conf 0600
>    globus-rls-server.x86_64: W: no-reload-entry
> /etc/rc.d/init.d/globus-rls-server
> Any words for these?

I removed the SXXrls.in file from the package - it is not useful for the RPM
package anyway since the RPM uses a %{SOURCE2} as the init.d script instead.

The configuration file is intentionally non-readable by non-root users since it
contains information about database username and password.

reload entry added to init.d script. It is empty since reloading is not
supported - as mandated by the guidelines.

> ! Please preserve the timestamp of %{SOURCE3} in %install

OK - Done for %{SOURCE2} as well.

> ! There is some html documentation under ./Doc that can be packaged. Also there
> is an INSTALL.html

The INSTALL.html file would be confusing to users since it contains information
about GPT and how to build from source which is not relevant when installing
from the RPM. The relevant part of the post-installation instructions are
available in %{SOURCE3} which is installed.

> ! I don't know how serious they are but 
>    Doc/man/man8/globus-rls-server.8
>    Doc/html/globus-rls-server.html
>    server.c
> contain references to /usr/local. You might want to fix them.

This is fixed by these lines already present in the spec file:

# Fix hardcoded default locations
sed 's!/usr/local/etc!/etc!g' -i server.c
sed 's!/usr/local/etc!/etc!g' -i Doc/man/man8/globus-rls-server.8

> ? /usr/share/globus/setup/setup-globus-common.pl defines perl location as 
>    /usr/lib/perl/
> Will this be a problem?

When using GPT as installed from the Fedora RPM the GPT perl module is already
in the default module path - so it will be found. The additions to the perl
include path of the $GPT_LOCATION/lib/perl directory (where they would be by
default in a configure, make, make install type installation) does not break
anything when using the RPM version.

> ? Are the tests under ./test worth running in %check ? 

The test requires a postgres server where you are allowed to create and modify
tables - not something you want to do during an RPM build.

> ! The package owns the directory
>    /usr/share/globus/packages/setup/
> I wasn't sure if this is intentional (or if its contents should actually go to
> /usr/share/globus/packages/globus_rls_server_setup/) and just wanted to bring
> it into your attention.

This is intentional.

> * The scriplets are different than the ones in the guidelines:
>   
> http://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets
> (See the Requires(*) stuff)

The scriptlet requires are added.

> * %{_initrddir}/%{name} does not contain all the required actions
>    http://fedoraproject.org/wiki/Packaging/SysVInitScript#Required_Actions

The missing actions added (see also the rpmlint section above).

> ? The "status" action in %{_initrddir}/%{name} has the port number hard-coded.
> Shouldn't the port number be taken from %{_sysconfdir}/%{name}.conf ?  

Fixed.

New version:

http://www.grid.tsl.uu.se/repos/globus/info/new/globus-rls-server-4.7-2.fc10.src.rpm
http://www.grid.tsl.uu.se/repos/globus/info/new/globus-rls-server.spec

-- 
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.