Hi Garrick, I started to do a torque review and came across the following
in the PBS_License.txt file:

  2. Redistribution in any form is only permitted for non-commercial,
     non-profit purposes.  There can be no charge for the Software or any
     software incorporating the Software.  Further, there can be no
     expectation of revenue generated as a consequence of redistributing
     the Software.

So I'm afraid the above isn't acceptable within FE since the Fedora 
Project aims to provide FOSS packages that are "free for anyone to use, 
modify and distribute" -- and the "anyone" may include both non- and 
for-profit entities.  So the above license disqualifies this submission:

  http://fedoraproject.org/wiki/Packaging/Guidelines#Legal

Sorry...

Fortunately we don't have to worry about that.  A few lines up is, "After
December 31, 2001, only conditions 3-6 must be met."

Btw, I just posted torque-2.1.0p0-0.2.200604060235.src.rpm which has better
.desktop files, some menu icons, and matching upstream source (the first srpm
was just pulled out of CVS).

I'm sorry!  I've re-opened the bug and will continue with the review.

An attempt to build this RPM:

  http://www-rcf.usc.edu/~garrick/torque-2.1.0p0-0.3.200604071240.src.rpm

in mock (FC4 i386) resulted in this error:

  RPM build errors:
    File must begin with "/": %{_desktopdir}/*.desktop

Bah!  _desktopdir is defined by jpackage-utils.

Updated http://www-rcf.usc.edu/~garrick/torque.spec changes that to
%{_datadir}/applications

Created attachment 127513 [details]
remove hard-coded rpaths

This tiny patch removes the hard-coded rpaths from the configure script and
here are the lines to add to the torque.spec:

  Patch0: torque_configure_remove_rpaths.patch

  %patch0 -p1

Hi Garrick, using your latest spec and the patch above I put together:

http://mitgcm.org/eh3/fedora_misc/torque-2.1.0p0-0.4.200604071240.src.rpm
md5sum:
2470a40280b9b65d6b49c9fffe5f46bb  torque-2.1.0p0-0.4.200604071240.src.rpm

which builds locally (and probably in mock since it worked with a 
very similar previous version).  In any case, heres the rpmlint output:

  W: torque invalid-license Freely redistributable (See PBS_License.txt)

[repeated many times] -- probably OK to ignore since the license does appear to 
satisfy the Fedora requirements

  W: torque no-documentation

[repeated many times] -- OK to ignore

  W: torque incoherent-version-in-changelog 2.1.0p0-0.3.200604071240
     2.1.0p0-0.4.200604071240

the naming does not exactly follow the pattern in 
  http://fedoraproject.org/wiki/Packaging/NamingGuidelines 
but it is pretty close -- could you please use the "YYYYMMDDcvs" 
pattern per the guidelines?

  E: torque no-binary
  E: torque non-standard-dir-perm /var/torque/spool 01777
  W: torque non-standard-dir-in-var torque

probably OK to ignore

  W: torque-client unstripped-binary-or-object /usr/sbin/pbs_iff
  E: torque-client setuid-binary /usr/sbin/pbs_iff root 04755
  E: torque-client non-standard-executable-perm /usr/sbin/pbs_iff 04755

These are worrisome.  The unstripped binary can probably be fixed
in the rpm build somehow.  And does the /usr/sbin/pbs_iff really 
need to be suid?

  E: torque-mom non-standard-dir-perm /var/torque/checkpoint 0700
  E: torque-mom non-standard-dir-perm /var/torque/undelivered 01777
  E: torque-mom non-standard-dir-perm /var/torque/mom_priv/jobs 0751
  W: torque-mom non-standard-dir-in-var torque
  W: torque-mom incoherent-init-script-name pbs_mom
  E: torque-scheduler non-standard-dir-perm /var/torque/sched_priv 0750
  W: torque-scheduler non-standard-dir-in-var torque
  W: torque-scheduler incoherent-init-script-name pbs_sched
  E: torque-server non-standard-dir-perm /var/torque/server_priv/acl_groups 0750
  E: torque-server non-standard-dir-perm /var/torque/server_priv/queues 0750
  E: torque-server non-standard-dir-perm /var/torque/server_priv 0750
  E: torque-server non-standard-dir-perm /var/torque/server_priv/acl_hosts 0750
  E: torque-server non-standard-dir-perm /var/torque/server_priv/acl_users 0750
  E: torque-server non-standard-dir-perm /var/torque/server_priv/acl_svr 0750
  E: torque-server non-standard-dir-perm /var/torque/server_priv/jobs 0750
  W: torque-server non-standard-dir-in-var torque
  W: torque-server incoherent-init-script-name pbs_server

The above can probably (?) be ignored.

(In reply to comment #6)
> Created an attachment (id=127513) [edit]
> remove hard-coded rpaths
> 
> This tiny patch removes the hard-coded rpaths from the configure script and
> here are the lines to add to the torque.spec:
> 
>   Patch0: torque_configure_remove_rpaths.patch
> 
>   %patch0 -p1


Is this necessary?  TORQUE is just doing standard autoconf+automake+libtool stuff.

(In reply to comment #7)
>   W: torque incoherent-version-in-changelog 2.1.0p0-0.3.200604071240
>      2.1.0p0-0.4.200604071240
> 
> the naming does not exactly follow the pattern in 
>   http://fedoraproject.org/wiki/Packaging/NamingGuidelines 
> but it is pretty close -- could you please use the "YYYYMMDDcvs" 
> pattern per the guidelines?

I'll add a "cvs" on the end.


>   W: torque-client unstripped-binary-or-object /usr/sbin/pbs_iff
>   E: torque-client setuid-binary /usr/sbin/pbs_iff root 04755
>   E: torque-client non-standard-executable-perm /usr/sbin/pbs_iff 04755
> 
> These are worrisome.  The unstripped binary can probably be fixed
> in the rpm build somehow.  And does the /usr/sbin/pbs_iff really 
> need to be suid?

Yes, pbs_iff needs to get a priv port as part of TORQUE's internal authn system.
 
>   E: torque-mom non-standard-dir-perm /var/torque/checkpoint 0700
>   E: torque-mom non-standard-dir-perm /var/torque/undelivered 01777
>   E: torque-mom non-standard-dir-perm /var/torque/mom_priv/jobs 0751
>   W: torque-mom non-standard-dir-in-var torque
>   W: torque-mom incoherent-init-script-name pbs_mom
>   E: torque-scheduler non-standard-dir-perm /var/torque/sched_priv 0750
>   W: torque-scheduler non-standard-dir-in-var torque
>   W: torque-scheduler incoherent-init-script-name pbs_sched
>   E: torque-server non-standard-dir-perm /var/torque/server_priv/acl_groups 0750
>   E: torque-server non-standard-dir-perm /var/torque/server_priv/queues 0750
>   E: torque-server non-standard-dir-perm /var/torque/server_priv 0750
>   E: torque-server non-standard-dir-perm /var/torque/server_priv/acl_hosts 0750
>   E: torque-server non-standard-dir-perm /var/torque/server_priv/acl_users 0750
>   E: torque-server non-standard-dir-perm /var/torque/server_priv/acl_svr 0750
>   E: torque-server non-standard-dir-perm /var/torque/server_priv/jobs 0750
>   W: torque-server non-standard-dir-in-var torque
>   W: torque-server incoherent-init-script-name pbs_server
> 
> The above can probably (?) be ignored.
> 

Yup, that's all correct.

(in reply to comment #8)

Its my understanding that rpaths can be problematic (or at best just 
redundant) as described at:

  http://fedoraproject.org/wiki/Extras/Schedule/RpathCheckBuildsys

But if you can convince me that they're harmless (even on multilib) then
we can ignore the rpath rpmlint warnings that happen without the patch.  
I think its safer/easier to remove them but I could be wrong.

http://www-rcf.usc.edu/~garrick/torque-2.1.0p0-0.5.200604071240cvs.src.rpm
http://www-rcf.usc.edu/~garrick/torque.spec

These fix the package release and include your rpath patch.

I've done mock builds on FC4 and FC5 on i386.

Hi Garrick, this isn't a full review but I'm hoping to find more time to 
look at it later this week.

good:
 rpmlint output basically unchanged from previous comments
 OK - follows naming guidelines
 OK - license seems acceptable and included
 OK - spec file is not as simple as it could be and it contains a 
    number of conditional options that are a little time-consuming to 
    read and (try to) understand -- but having looked at them I don't
    see any actual blockers
 OK - builds in mock on FC5 i386
 OK - dir ownership and permissions look fine
 OK - libs seem fine and no *.la files
 OK - code not content

nits:
 - the [ "$RPM_BUILD_ROOT" != "/" ] is not necessary for FE
 - If you'd like to have the same version of torque in, say, 
     FE4, FE5, and devel then you'll probably want to add
     %{?dist} per http://fedoraproject.org/wiki/DistTag
 - perhaps the headers currently located at /usr/include/* 
     could go in a subdir such as /usr/include/torque/* since 
     some of the header files have rather unfortunately generic 
     names (eg. "tm.h")

blockers:
 - How can I verify that the source matches upstream?  I found 
   the download pages at:

     http://www.clusterresources.com/downloads/torque/snapshots/

   but I can't seem to find the same .tar.gz file or a way to 
   create an identical one from CVS -- could you please document 
   that step within the spec file as a comment so that I can repeat 
   it?  Or perhaps use one of the "official" tar files?

(In reply to comment #12)
> nits:
>  - the [ "$RPM_BUILD_ROOT" != "/" ] is not necessary for FE

Ok, will remove.

>  - If you'd like to have the same version of torque in, say, 
>      FE4, FE5, and devel then you'll probably want to add
>      %{?dist} per http://fedoraproject.org/wiki/DistTag

I was wondering about this, but I left it out because it isn't actually in the
naming guidelines.

>  - perhaps the headers currently located at /usr/include/* 
>      could go in a subdir such as /usr/include/torque/* since 
>      some of the header files have rather unfortunately generic 
>      names (eg. "tm.h")

Sure.
 
> blockers:
>  - How can I verify that the source matches upstream?  I found 
>    the download pages at:
> 
>      http://www.clusterresources.com/downloads/torque/snapshots/
> 
>    but I can't seem to find the same .tar.gz file or a way to 
>    create an identical one from CVS -- could you please document 
>    that step within the spec file as a comment so that I can repeat 
>    it?  Or perhaps use one of the "official" tar files?

I'll roll an "official" snapshot upstream and give you a new srpm in a few minutes.

http://www-rcf.usc.edu/~garrick/torque-2.1.0p0-0.5.200604171430cvs.src.rpm
http://www-rcf.usc.edu/~garrick/torque.spec

* Mon Apr 17 2006 Garrick Staples <garrick> 2.1.0p0-0.6.200604171430cvs
- add %%{dist} tag
- cleanup the cleanups in spec
- bump to matching upstream
- move headers to /usr/include/torque/

The SRPM [please note the "6" instead of "5" in release tag since the 
SRPM URL in comment #14 has a typo] at:

  http://www-rcf.usc.edu/~garrick/torque-2.1.0p0-0.6.200604171430cvs.src.rpm
  sha1sum: ba0a569763d6b91697c9a40723b392be464ab64e

does cleanup everything mentioned in comments #12--13 and heres the
remainder of the review:

very minor nit:
 + please consider adding the "-q" option to %setup so that the 
   build logs are a little shorter and more readable (just a 
   request--by no means a blocker!)

good:
 OK - source matches upstream
 OK - macro usage looks consistent although there are some 
      harmless quirks like having both %__rm and %{__rm}
 OK - proper use of -devel
 OK - desktop files appear to have correct install syntax
 OK - scriptlets look sane to me
 OK - installed and runs with out seg-faulting on a single FC4 i386
      machine (I do need to go dig up the syntax for creating default 
      queues, etc. because a quick "qsub -I" seems to wait forever and 
      I imagine its an incomplete setup and thus my fault.  Other 
      commands such as "pbsnodes -a" and "qmgr" work just fine--no 
      segfaults.)

I don't see any blockers so its APPROVED.

Congrats on the first package and please feel free to contact me if you 
want any help with FE CVS, the build system, etc.

The builds just went through so I guess we're done here.

Thanks Ed for the help!

Package Change Request
======================
Package Name: torque
New Branches: EL-4 EL-5
Owners: stevetraylen
InitialCC: garrick


I sent mails to the owner on September 23rd and more explicitly
on November 25th requesting the EPEL branches.
Also bug 479672 has been present for a while.

Steve Traylen.

cvs done.

torque-2.3.8-1.el4 has been submitted as an update for Fedora EPEL 4.
http://admin.fedoraproject.org/updates/torque-2.3.8-1.el4

torque-2.3.8-1.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/torque-2.3.8-1.el5

New Package SCM Request
=======================
Package Name: torque
Short Description: cluster job scheduler
Upstream URL: http://www.adaptivecomputing.com/products/open-source/torque/
Owners: dmlb2000 hguemar
Branches: epel7

Fedora review flag not set.

Following the https://fedoraproject.org/wiki/Package_SCM_admin_requests document for existing packages mentions nothing about setting the fedora-review flag.

This is a new branch for an existing package...

In that case please use the Package Change Request, rather than New Package.

Oh, I got the wrong title in my comment...

Package Change Request
======================
Package Name: torque
Short Description: cluster job scheduler
Upstream URL: http://www.adaptivecomputing.com/products/open-source/torque/
Owners: dmlb2000 hguemar
Branches: epel7

Package Change Request
======================
Package Name: torque
New Branches: epel7
Owners: dmlb2000 hguemar

Git done (by process-git-requests).