On Mar 14, 2011, at 6:12 AM, Lukasz Janyst wrote:
> 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.
>
Well, I suppose it depends on how many dependencies all the security plugins pull in. If it's just OpenSSL and KRB5, then it's probably not too bad.
We should build more of the plugins if possible - I notice we don't currently ship KRB5.
>> - 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.
>
Andy?
>> - 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.
>
I use the latest SL5 git:
[brian@brian-test http]$ rpm -q git
git-1.5.5.6-4.el5
>> 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.
>
My reasoning was that it would help for pre-releases
>> 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.
This is the common practice on the OS platform: MySQL, postgres, apache HTTP, etc. It's done by the grid middleware packagers and by other CERN IT projects (CVMFS).
I would strongly suggest this to match the other packagers of xrootd and for security reasons. I know at least one US lab site that bans running processes under the "daemon" account simply because too many packages use it.
>
>> - 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
