Bug 283081

ADDITIONAL NOTE: You will need F7 or RHEL 3 to compile this code. I recommend
compiling with mock and a f7-i386 config. Support for RHEL 4+5 is forthcoming.

In i386, you should be building with fedora-devel-i386.cfg, as this is what will
go into the next Fedora release (F8).

Please do the review with f7-i386 while I work with upstream to get an F8 port.
The main issues should be in the spec and once they are ironed out rev'ing the
source should be straightforward (and necessary anyway).

I've done a review of the package as-is, here are my notes:

- tarfile matches upstream with the following md5 sum:

$ md5sum condor-BUILD-V6_9-trunk-2007-9-7.tar.gz 
d6fb0aa4bb1aba0617d09436b6dae8e4  condor-BUILD-V6_9-trunk-2007-9-7.tar.gz

- couldn't build in mock under fedora-devel

- as expected, rpmlint gives warning about license:

$ rpmlint condor-6.9.5-0.1.20070907cvs.src.rpm
W: condor invalid-license Condor Public License

- some lines in the specfile are longer than 80 characters

- license text (LICENSE.TXT) is missing from the package (the %doc line is
commented out)


The last two items can be fixed independently of the other (major) issues of
building under fedora-devel and getting a compatible license.

Nuno

I've uploaded a new SRPM and spec:

 spec: http://grid.et.redhat.com/files/condor.spec
 SRPM: http://grid.et.redhat.com/files/condor-6.9.5-0.2.20070907cvs.src.rpm

This addresses the issue of no documentation, most lines longer than 80
characters have been fixed, and the package builds under fedora-devel-i368.
However, building is tied to the current Rawhide (7.91). I will provide a patch
for 7.92 when it is released.

Reviewed the revised package/spec:

- confirmed that the missing %doc and the long lines issues have been fixed

- managed to build in mock under fedora-devel-i368:

$ mock -r fedora-devel-i386.cfg condor-6.9.5-0.2.20070907cvs.src.rpm 
init
clean
prep
This may take a while
unpack cache
setup
build
ending
done
Results and/or logs in: /var/lib/mock/fedora-development-i386/result

$ cd /var/lib/mock/fedora-development-i386/result
$ ls -alF
total 37968
drwxr-sr-x 2 nsantos mock     4096 Sep 11 11:53 ./
drwxr-sr-x 5 nsantos mock     4096 Sep 11 11:41 ../
-rw-r--r-- 1 nsantos mock   881599 Sep 11 11:53 build.log
-rw-rw-r-- 1 nsantos mock 22186503 Sep 11 11:53
condor-6.9.5-0.2.20070907cvs.i386.rpm
-rw-rw-r-- 1 nsantos mock 14988562 Sep 11 11:45 condor-6.9.5-0.2.20070907cvs.src.rpm
-rw-rw-r-- 1 nsantos mock     2506 Sep 11 11:53
condor-debuginfo-6.9.5-0.2.20070907cvs.i386.rpm
-rw-rw-r-- 1 nsantos mock   719184 Sep 11 11:53
condor-devel-6.9.5-0.2.20070907cvs.i386.rpm
-rw-r--r-- 1 nsantos mock      165 Sep 11 11:41 mockconfig.log
-rw-r--r-- 1 nsantos mock    17190 Sep 11 11:53 root.log


Confirmation of requires/provides:

$ for i in `ls *.rpm`; do echo; echo $i; echo "Provides:" ; rpm -qp $i
--provides; echo "Requires:"; rpm -qp $i --requires; done

condor-6.9.5-0.2.20070907cvs.i386.rpm
Provides:
config(condor) = 6.9.5-0.2.20070907cvs
perl(Condor)  
perl(Execute) = 1.00
perl(FileLock) = 1.01
condor = 6.9.5-0.2.20070907cvs
Requires:
/bin/bash  
/bin/sh  
/bin/sh  
/sbin/ldconfig  
/sbin/ldconfig  
/usr/bin/env  
/usr/bin/perl  
config(condor) = 6.9.5-0.2.20070907cvs
krb5-libs  
libc.so.6  
libc.so.6(GLIBC_2.0)  
libc.so.6(GLIBC_2.1)  
libc.so.6(GLIBC_2.1.3)  
libc.so.6(GLIBC_2.2)  
libc.so.6(GLIBC_2.3)  
libc.so.6(GLIBC_2.3.4)  
libc.so.6(GLIBC_2.4)  
libcom_err.so.2  
libcrypt.so.1  
libcrypto.so.6  
libdl.so.2  
libdl.so.2(GLIBC_2.0)  
libdl.so.2(GLIBC_2.1)  
libgcc_s.so.1  
libgcc_s.so.1(GCC_3.0)  
libgcc_s.so.1(GLIBC_2.0)  
libk5crypto.so.3  
libk5crypto.so.3(k5crypto_3_MIT)  
libkrb5.so.3  
libkrb5.so.3(krb5_3_MIT)  
libm.so.6  
libm.so.6(GLIBC_2.0)  
libm.so.6(GLIBC_2.1)  
libpcre.so.0  
libpq.so.5  
libresolv.so.2  
libssl.so.6  
libstdc++.so.6  
libstdc++.so.6(CXXABI_1.3)  
libstdc++.so.6(GLIBCXX_3.4)  
mailx  
openssl  
pcre  
perl >= 0:5.003
perl >= 1:5.0
perl(Carp)  
perl(Cwd)  
perl(English)  
perl(Execute)  
perl(Exporter)  
perl(Fcntl)  
perl(File::Basename)  
perl(File::Copy)  
perl(File::Find)  
perl(File::Path)  
perl(File::Spec)  
perl(File::Spec::Functions)  
perl(File::Temp)  
perl(FileHandle)  
perl(FileLock)  
perl(FindBin)  
perl(Getopt::Long)  
perl(POSIX)  
perl(Socket)  
perl(Sys::Hostname)  
perl(Text::Wrap)  
perl(lib)  
perl(strict)  
perl(warnings)  
postgresql-libs  
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PartialHardlinkSets) <= 4.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(VersionedDependencies) <= 3.0.3-1
rtld(GNU_HASH)  
shadow-utils  

condor-6.9.5-0.2.20070907cvs.src.rpm
Provides:
(none)
Requires:
imake  
flex  
byacc  
tcsh  
pcre-devel  
postgresql-devel  
openssl-devel  
krb5-devel  
bind-utils  
rpmlib(CompressedFileNames) <= 3.0.4-1

condor-debuginfo-6.9.5-0.2.20070907cvs.i386.rpm
Provides:
condor-debuginfo = 6.9.5-0.2.20070907cvs
Requires:
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(CompressedFileNames) <= 3.0.4-1

condor-devel-6.9.5-0.2.20070907cvs.i386.rpm
Provides:
libcondorapi.so  
condor-devel = 6.9.5-0.2.20070907cvs
Requires:
condor = 6.9.5-0.2.20070907cvs
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rtld(GNU_HASH)  



Here are the rpmlint results, please fix or justify as appropriate (ignoring the
license-related ones for now):

$ for i in `ls *.rpm`; do echo; echo $i; rpmlint $i; done

condor-6.9.5-0.2.20070907cvs.i386.rpm
E: condor non-standard-uid /var/lib/condor/execute condor
E: condor non-standard-gid /var/lib/condor/execute condor
E: condor non-standard-dir-perm /var/lib/condor/execute 01777
E: condor non-standard-uid /var/lib/condor/log condor
E: condor non-standard-gid /var/lib/condor/log condor
E: condor non-standard-uid /var/lib/condor/spool condor
E: condor non-standard-gid /var/lib/condor/spool condor
W: condor invalid-license Condor Public License

condor-6.9.5-0.2.20070907cvs.src.rpm
W: condor invalid-license Condor Public License

condor-debuginfo-6.9.5-0.2.20070907cvs.i386.rpm
E: condor-debuginfo empty-debuginfo-package
W: condor-debuginfo invalid-license Condor Public License

condor-devel-6.9.5-0.2.20070907cvs.i386.rpm
W: condor-devel invalid-license Condor Public License
W: condor-devel unstripped-binary-or-object /usr/lib/libcondorapi.so
W: condor-devel no-soname /usr/lib/libcondorapi.so

A new SRPM and spec (same url as before) are available:

 SRPM: http://grid.et.redhat.com/files/condor-6.9.5-0.3.20070907cvs.src.rpm

The unstripped-binary-or-object and no-soname warnings have been removed by
removing the libcondorapi.so in favor of the libcondorapi.a, which I've been
told by upstream is what people use anyway. This means the -devel package has
actually been replaced with a -static package.

After looking through the maze of pages about users and groups (including the
user registry) it appears that user creation is acceptable when accompanied by a
good reason. For this case, the condor user owns all the daemons run in the
condor system and the files under /var/lib/condor, which includes files
associated with jobs that are sent to a machine for execution. It is desirable
to have a known owner for all such files.

The permissions on /var/lib/condor/execute are intentional as it is a world
writeable directory and it is desirable to take advantage of restricted deletion
(sticky bit).

Also, the empty debuginfo package has been fixed by using unstripped binaries
for the primary package.

Rebuilt in mock, here's the rpmlint output:

$ for i in `ls *.rpm`; do echo; echo $i; rpmlint $i; done

condor-6.9.5-0.3.20070907cvs.i386.rpm
E: condor non-standard-uid /var/lib/condor/execute condor
E: condor non-standard-dir-perm /var/lib/condor/execute 01777
E: condor non-standard-uid /var/lib/condor/log condor
E: condor non-standard-uid /var/lib/condor/spool condor
W: condor invalid-license Condor Public License

condor-6.9.5-0.3.20070907cvs.src.rpm
W: condor invalid-license Condor Public License

condor-debuginfo-6.9.5-0.3.20070907cvs.i386.rpm
W: condor-debuginfo invalid-license Condor Public License

condor-static-6.9.5-0.3.20070907cvs.i386.rpm
W: condor-static invalid-license Condor Public License


So everything looks good (your reasoning for the condor userid is valid, same
with the dir permissions), let's just wait for the license issue to be solved.

The license issue has been resolved.

Spec URL: http://grid.et.redhat.com/files/condor.spec
SRPM URL: http://grid.et.redhat.com/files/condor-7.0.0-3.el5.src.rpm

Build issues on F9 (fedora-devel) have been resolved.

Spec URL: http://grid.et.redhat.com/files/condor.spec
SRPM URL: http://grid.et.redhat.com/files/condor-7.0.0-4.el5.src.rpm

Successfully built on F9:

$mock -r fedora-devel-i386.cfg condor-7.0.0-4.el5.src.rpm
init
clean
prep
This may take a while
unpack cache
setup
build
ending
done
Results and/or logs in: /var/lib/mock/fedora-development-i386/result

$ cd /var/lib/mock/fedora-development-i386/result
$ ls -alF
total 80348
drwxr-sr-x 2 nsantos mock     4096 Feb  8 17:48 ./
drwxr-sr-x 5 nsantos mock     4096 Feb  8 17:35 ../
-rw-r--r-- 1 nsantos mock  2432332 Feb  8 17:48 build.log
-rw-r--r-- 1 nsantos mock 23756936 Feb  8 17:47 condor-7.0.0-4.fc9.i386.rpm
-rw-r--r-- 1 nsantos mock 12415405 Feb  8 17:36 condor-7.0.0-4.fc9.src.rpm
-rw-r--r-- 1 nsantos mock 43146428 Feb  8 17:48
condor-debuginfo-7.0.0-4.fc9.i386.rpm
-rw-r--r-- 1 nsantos mock   342602 Feb  8 17:47 condor-static-7.0.0-4.fc9.i386.rpm
-rw-r--r-- 1 nsantos mock      165 Feb  8 17:35 mockconfig.log
-rw-r--r-- 1 nsantos mock    16530 Feb  8 17:48 root.log


Confirmation of requires/provides:

$ for i in `ls *.rpm`; do echo; echo $i; echo "Provides:" ; rpm -qp $i
--provides; echo "Requires:"; rpm -qp $i --requires; done

condor-7.0.0-4.fc9.i386.rpm
Provides:
config(condor) = 7.0.0-4.fc9
perl(Condor)  
perl(Execute) = 1.00
perl(FileLock) = 1.01
condor = 7.0.0-4.fc9
Requires:
/bin/bash  
/bin/sh  
/bin/sh  
/bin/sh  
/bin/sh  
/bin/sh  
/sbin/chkconfig  
/sbin/chkconfig  
/sbin/service  
/sbin/service  
/usr/bin/env  
/usr/bin/perl  
config(condor) = 7.0.0-4.fc9
gsoap  
krb5-libs  
libc.so.6  
libc.so.6(GLIBC_2.0)  
libc.so.6(GLIBC_2.1)  
libc.so.6(GLIBC_2.1.3)  
libc.so.6(GLIBC_2.2)  
libc.so.6(GLIBC_2.3)  
libc.so.6(GLIBC_2.3.3)  
libc.so.6(GLIBC_2.3.4)  
libc.so.6(GLIBC_2.4)  
libcom_err.so.2  
libcrypt.so.1  
libcrypto.so.7  
libdl.so.2  
libdl.so.2(GLIBC_2.0)  
libdl.so.2(GLIBC_2.1)  
libgcc_s.so.1  
libgcc_s.so.1(GCC_3.0)  
libgcc_s.so.1(GLIBC_2.0)  
libgsoapssl++.so.0  
libk5crypto.so.3  
libk5crypto.so.3(k5crypto_3_MIT)  
libkrb5.so.3  
libkrb5.so.3(krb5_3_MIT)  
libm.so.6  
libm.so.6(GLIBC_2.0)  
libpcre.so.0  
libpq.so.5  
libresolv.so.2  
libssl.so.7  
libstdc++.so.6  
libstdc++.so.6(CXXABI_1.3)  
libstdc++.so.6(GLIBCXX_3.4)  
mailx  
openssl  
pcre  
perl >= 0:5.006
perl >= 1:5.0
perl(Carp)  
perl(Cwd)  
perl(English)  
perl(Execute)  
perl(Exporter)  
perl(Fcntl)  
perl(File::Basename)  
perl(File::Copy)  
perl(File::Find)  
perl(File::Path)  
perl(File::Spec)  
perl(File::Spec::Functions)  
perl(File::Temp)  
perl(FileHandle)  
perl(FileLock)  
perl(FindBin)  
perl(Getopt::Long)  
perl(POSIX)  
perl(Socket)  
perl(Sys::Hostname)  
perl(Text::Wrap)  
perl(lib)  
perl(strict)  
perl(warnings)  
postgresql-libs  
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PartialHardlinkSets) <= 4.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(VersionedDependencies) <= 3.0.3-1
rtld(GNU_HASH)  
shadow-utils  

condor-7.0.0-4.fc9.src.rpm
Provides:
(none)Requires:
imake  
flex  
byacc  
tcsh  
pcre-devel  
postgresql-devel  
openssl-devel  
krb5-devel  
gsoap-devel  
bind-utils  
m4  
autoconf  
rpmlib(CompressedFileNames) <= 3.0.4-1

condor-debuginfo-7.0.0-4.fc9.i386.rpm
Provides:
condor-debuginfo = 7.0.0-4.fc9
Requires:
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PartialHardlinkSets) <= 4.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1

condor-static-7.0.0-4.fc9.i386.rpm
Provides:
condor-static = 7.0.0-4.fc9
Requires:
condor = 7.0.0-4.fc9
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1


Rpmlint output:

condor-7.0.0-4.fc9.i386.rpm
E: condor non-standard-uid /var/lib/condor condor
E: condor non-standard-gid /var/lib/condor condor
E: condor non-standard-uid /var/lib/condor/execute condor
E: condor non-standard-gid /var/lib/condor/execute condor
E: condor non-standard-dir-perm /var/lib/condor/execute 01777
E: condor non-standard-uid /var/lib/condor/log condor
E: condor non-standard-gid /var/lib/condor/log condor
E: condor non-standard-uid /var/lib/condor/spool condor
E: condor non-standard-gid /var/lib/condor/spool condor
W: condor service-default-enabled /etc/rc.d/init.d/condor
W: condor incoherent-subsys /etc/rc.d/init.d/condor $prog
W: condor service-default-enabled /etc/rc.d/init.d/condor

condor-7.0.0-4.fc9.src.rpm

condor-debuginfo-7.0.0-4.fc9.i386.rpm

condor-static-7.0.0-4.fc9.i386.rpm



Matt: you had addressed the user and perms issues before, the service warnings
are expected, can you look into the "incoherent-subsys" warning?

That warning appears because the condor init script actually controls a program
called "condor_master". I would like to keep the init script's name "condor" as
that is the service that is being controlled.

Sounds good, thanks for clarifying.

Looks like all issues have been resolved, I'm marking this as fedora-review+

First of all. let me say, as a former LSF suer, that I am more then happy to see
this program finally arriving in our land. Kudos!

Second, a couple of comments
- please add a note before the Source0 tag, explaining that upstream explicitly
prohibits direct download, hence the missing full URL in this field.
For verification, one can provide the required personal info data at
http://parrot.cs.wisc.edu/v7.0.license.html and download the sources from
http://www.cs.wisc.edu/condor/cgi-bin/downloads/download.pl/v7.0#7.0.0-source-code
- there is no need to create a "patch" in order to add the init.d script. Just
provide it as Source1 and use it as such (not that what you have done is not
correct; it's just unnecessary complicated)
- the build log says:
checking for javac... no
configure: WARNING: javac not found
checking for jar... no
configure: WARNING: jar not found
Would condor be happier with icedtea / gcj as BR ?
-there is a "checking for zlib... no" line in the build log; would a BR
zlib-devel be nbeneficial ?
- parallel compiling seems to be disabled (and commented in the spec). Is it
still intended ?
- rpmlint says
condor.x86_64: W: service-default-enabled /etc/rc.d/init.d/condor
We usually require services to not be enabled by default

s.suer.user. !!!

Re: no source URL, there is no URL because I process the source tarball before
including it in the SRPM. The tarball that UW distributes is 100MB+ and includes
copies of what they call "externals", such as Globus and PVM, that this package
does not use.

Yes, the init script is overly complicated, and will hopefully be part of a
future source release anyway.

Re: java & jar, if you inspect the source, you'll discover that these tests do
not actually change anything.

Re: zlib, you'll notice %configure ... --without-zlib, because the features of
Condor that require zlib are not included in this package.

Re: make -j, yes, dependencies are not setup correctly for parallel make.

Re: enabled by default, if this is a requirement, I can change the init script.

Thank you for your comments. I hope my clarifications are helpful.

New Package CVS Request
=======================
Package Name: condor
Short Description: Batch system for High Throughput Computing
Owners: matt
Branches: 
InitialCC: 
Cvsextras Commits: Yes

In reply to comment #16 and #14. 

Is the source that you remove from upstream Patented or Trademarked such that it
can't be shipped in Fedora? If so, please see: 
http://fedoraproject.org/wiki/Packaging/SourceURL

If it's just a case of it not being needed/used, but not prohibited code, it
would be better to simply just use the upstream source package. 

If you can add a script and note about how you modify the source before checkin,
that would be great. 

cvs done.

There are three issues: 1) there is source that cannot be distributed; 2) there
is about 100MB of source that can be distributed but is never used; 3) there is
no link to the source directly, it is behind a registration page

I'll add instructions for downloading the source and provide a script to strip
out portions we do not want included in the package.

cvs was done here... resetting fedora-cvs flag.

> Requires: pcre
> Requires: postgresql-libs
> Requires: openssl
> Requires: krb5-libs
> Requires: gsoap

All these ought to be removed in favour of the automatic dependencies on the library SONAMEs. It's not very obvious, but the review guidelines mention it in the packaging guidelines:

    https://fedoraproject.org/wiki/Packaging/Guidelines#Requires

I've pruned some of the Requires for 7.2.1-1. Doing so assumes that all the dependencies do proper SO versioning, which may be a dangerous assumption.

Please file new bugs instead of commenting on closed ones.

Your explicit Requires are version-less. They are no better than the automatic SONAME deps. They are fragile even, since they would break if a library were moved to another package or if a package were renamed.

The point of adding the comment here is to make the reviewer aware of this, too, as this ought to have caught during the review.

> Doing so assumes that all the dependencies do proper SO versioning,
> which may be a dangerous assumption.

It doesn't take much to verify that the SONAMEs include major versions. Multiple libraries using the same SONAME would be a bad conflict in the package collection [unless it's a special-case of alternative installations that is being treated appropriately].