2011/3/12 Brian Bockelman <[log in to unmask]>:
> Took the chance to review Lukasz's new RPMs.  Here are my items of concern:
> - We probably want to make sub-RPMs for the different security plugins (ssl, gsi, krb5, etc)

   I am not really convinced but don't have strong opinions about this.

> - It's bad form to have multiple sources if the items in the .spec file if they're included in the tarball.  I.e., instead of:
>  install -m 755 %{SOURCE2} $RPM_BUILD_ROOT%{_initrddir}/cmsd
> You could have done:
>  install -m 755 packaging/rhel/cmsd.init $RPM_BUILD_ROOT%{_initrddir}/cmsd
> This cuts down on the number of sources and hence the ability for folks to introduce errors.

   Good point. Will fix.

> - It is kind and traditional to include in comments where the sources can be taken from.  I.e., from my Xrootd RPM:
> # git clone http://xrootd.org/repo/xrootd.git xrootd
> # cd xrootd
> # git-archive master | gzip -9 > ~/rpmbuild/SOURCES/xrootd.tgz
> Source0:        xrootd.tgz

   Well, why not.

> - I strongly suggest a sample configuration file so the user can do a "yum install xrootd; service xrootd start" and have something working immediately.

   I agree. The best would be the "Apache style":
https://github.com/apache/httpd/blob/trunk/docs/conf/httpd.conf.in
https://github.com/apache/cassandra/blob/trunk/conf/cassandra.yaml

> - I notice you have three xrootd config files ({xrootd,cmsd,frmd}-main.cf) - I thought it was the general style of the project to have one config file?

   The init scrips can support both: one big config file for everyone
or one config file per daemon. I have done it this way because this is
the only way I could make daemons working (the config file installed
by default didn't work). If you give me a common config file that
*works* I will merge them.

> - Couldn't get the makesrpm.sh script to work.  Failed with:
> [brian@brian-test packaging]$ ./makesrpm.sh
> [i] Working on: ../
> [i] Stoing the output to: .
> [i] Working with version: unknown
> [i] RPM compliant version: unknown
> [i] Woring in: /tmp/tmp.pEERkq7868
> fatal: unrecognized argument: --format=%H
> [!] Unable to figure out the git commit hash
> Poking around a little, it seems to be due to me not being on an exact tag.  I think instead of:
> git log -1 --format='%ai %h'
> you want:
> git log -1 --pretty=format:'%ai %H'

   No, what I try to extract is a long hash of the latest commit on
the current branch. It looks like the format option differs between
the git versions. I will look into it.

> Fixing this, the version number appears something like this:
> 20110312.73b5e2f
> Could we instead use something like "git describe"'s output?
> v3.0.2-90-g73b5e2f

   There was a discussion about that at some point. I would prefer to
keep it consistent with the stuff that goes into XrdVersion.hh.

> This is something we can more easily mangle into the RPM.
> - makesrpm.sh doesn't work because I have local macros set.  I removed your "dirty hack" and, with the patch below, it works.
> - I strongly recommend adding an xrootd user rather than having it run as 'daemon'.

   Adding a user account on RPM install is too intrusive and
unacceptable for most of our clients.

> - What about the "cnsd" daemon?  No started up script is given for that.

  This is because it didn't have proper daemonization code and
inconsistent commandline options, needs to be resolved in the future.

> - I'm not sure it's really plausible to have multiple xrootds run in this setup, but I think that's a larger issue

   It is, look at the other email.

> - You probably want to add at least "Epoch: 1" here in order to match the EPEL packaging.

  OK.

> I have tried to include as many of these comments in a patch below; some require input or decision making from your side.

   Thanks, I will massage it and apply.

>
> Overall, a great improvement!  Thanks Lukasz!  With a bit more finessing, I can put my separate RPMs to rest.

   Thanks for the constructive comments!

Cheers,
   Lukasz