I didn't put a S-o-b line on behalf of Ben, since I wasn't sure if I
should be doing that for patches originally submitted via Redmine.

The only real comment I have on Ben's patch is that we should also
remove conf/suse/ruby-env.patch, since it's not being used anymore, but
I can submit a follow-up patch to do that.

Couple of very minor questions/comments inline.

-- 
Jacob Helwig

On Fri, 24 Sep 2010 11:36:48 -0700, Jacob Helwig wrote:
> 
> From: Ben Kevan <[email protected]>
> 
> ---
> 
> This was originally a patch attached to #4772 [1]
> 
> [1] http://projects.puppetlabs.com/issues/4772
> 
>  conf/suse/puppet.spec |   27 +++++++++++++++------------
>  1 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/conf/suse/puppet.spec b/conf/suse/puppet.spec
> index bd4b358..941a650 100644
> --- a/conf/suse/puppet.spec
> +++ b/conf/suse/puppet.spec
> @@ -4,23 +4,20 @@
>  
>  Summary: A network tool for managing many disparate systems
>  Name: puppet
> -Version: 2.6.0
> +Version: 2.6.1
>  Release: 1%{?dist}
>  License: GPL
> -Group:    Productivity/Networking/System
> +Group:       Productivity/Networking/System

Necessary, or stylistic?  The other fields don't have a hard-tab here,
so I'm curious why Group is the only one that does, since that seems to
be the only change on this line?

>  
>  URL: http://puppetlabs.com/projects/puppet/
>  Source0: http://puppetlabs.com/downloads/puppet/%{name}-%{version}.tar.gz
> -Source1: client.init
> -Source2: server.init
> -Patch0: ruby-env.patch
>  
>  PreReq: %{insserv_prereq} %{fillup_prereq}
> -Requires: ruby >= 1.8.2
> +Requires: ruby >= 1.8.1
>  Requires: facter >= 1.5
>  Requires: cron
>  BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
> -BuildRequires: ruby >= 1.8.7
> +BuildRequires: ruby >= 1.8.1
>  
>  %description
>  Puppet lets you centrally manage every important aspect of your system using 
> a 
> @@ -29,7 +26,7 @@ normally aggregated in different files, like users, cron 
> jobs, and hosts,
>  along with obviously discrete elements like packages, services, and files.
>  
>  %package server
> -Group:    Productivity/Networking/System
> +Group:       Productivity/Networking/System

Same as above.

>  Summary: Server for the puppet system management tool
>  Requires: puppet = %{version}-%{release}
>  
> @@ -39,10 +36,9 @@ The server can also function as a certificate authority 
> and file server.
>  
>  %prep
>  %setup -q
> -%patch0 -p0
>  
>  %build
> -for f in bin/* ; do
> +for f in bin/* sbin/*; do
>   sed -i -e '1c#!/usr/bin/ruby' $f
>  done

I wouldn't expect this be changed as part of this patch, and is just me
voicing a general concern, but unconditionally replacing the first line
of any file under bin/, and sbin/ seems...dangerous.  This is probably
just be being paranoid.

All of the files currently under bin/ and sbin/ start with
"#!/usr/bin/env ruby", but I'd be much more comfortable with a
replacement that wouldn't end up changing something like "#!/bin/sh" to
"#!/usr/bin/ruby" if a non-Ruby script were to make it into bin/ or
sbin/ for some reason.

>  
> @@ -63,16 +59,16 @@ done
>  find %{buildroot}%{ruby_sitelibdir} -type f -perm +ugo+x -exec chmod a-x 
> '{}' \;
>  %{__cp} -a %{pbuild}/conf/redhat/client.sysconfig 
> %{buildroot}%{_confdir}/client.sysconfig
>  %{__install} -Dp -m0644 %{buildroot}%{_confdir}/client.sysconfig 
> %{buildroot}/var/adm/fillup-templates/sysconfig.puppet
> -%{__install} -Dp -m0755 %SOURCE1 %{buildroot}%{_initrddir}/puppet
>  %{__cp} -a %{pbuild}/conf/redhat/server.sysconfig 
> %{buildroot}%{_confdir}/server.sysconfig
>  %{__install} -Dp -m0644 %{buildroot}%{_confdir}/server.sysconfig 
> %{buildroot}/var/adm/fillup-templates/sysconfig.puppetmaster
> -%{__install} -Dp -m0755 %SOURCE2 %{buildroot}%{_initrddir}/puppetmaster
>  %{__cp} -a %{pbuild}/conf/redhat/fileserver.conf 
> %{buildroot}%{_confdir}/fileserver.conf
>  %{__install} -Dp -m0644 %{buildroot}%{_confdir}/fileserver.conf 
> %{buildroot}%{_sysconfdir}/puppet/fileserver.conf
>  %{__cp} -a %{pbuild}/conf/redhat/puppet.conf 
> %{buildroot}%{_confdir}/puppet.conf
>  %{__install} -Dp -m0644 %{buildroot}%{_confdir}/puppet.conf 
> %{buildroot}%{_sysconfdir}/puppet/puppet.conf
>  %{__cp} -a %{pbuild}/conf/redhat/logrotate %{buildroot}%{_confdir}/logrotate
>  %{__install} -Dp -m0644 %{buildroot}%{_confdir}/logrotate 
> %{buildroot}%{_sysconfdir}/logrotate.d/puppet
> +%{__install} -Dp -m0755 %{confdir}/client.init 
> %{buildroot}%{_initrddir}/puppet
> +%{__install} -Dp -m0755 %{confdir}/server.init 
> %{buildroot}%{_initrddir}/puppetmaster
>  %{__ln_s}  %{_initrddir}/puppet %{buildroot}%{_sbindir}/rcpuppet
>  %{__ln_s}  %{_initrddir}/puppetmaster %{buildroot}%{_sbindir}/rcpuppetmaster
>  
> @@ -140,6 +136,13 @@ find %{buildroot}%{ruby_sitelibdir} -type f -perm +ugo+x 
> -exec chmod a-x '{}' \;
>  %{__rm} -rf %{buildroot}
>  
>  %changelog
> +* Tue Sep 14 2010 Ben Kevan <[email protected]> - 2.6.1
> +- New version to 2.6.1
> +- Add client.init and server.init from source since it's now included in the 
> packages
> +- Bumped ruby version back down, as issue #4333 is resolved
> +- Removed ruby-env patch, replaced with sed in prep
> +- Update urls to puppetlabs.com
> +
>  * Wed Jul 21 2010 Ben Kevan <[email protected]> - 2.6.0
>  - New version and ruby version bump
>  - Add puppetdoc to %_bindir (unknown why original suse package, excluded or 
> forgot to add)
> -- 
> 1.7.3
> 

Attachment: signature.asc
Description: Digital signature

Reply via email to