On Wed, Jul 20, 2005, Matthias Kurz wrote:
> OpenPKG CVS Repository
> http://cvs.openpkg.org/
> ____________________________________________________________________________
>
> Server: cvs.openpkg.org Name: Matthias Kurz
> Root: /v/openpkg/cvs Email: [EMAIL PROTECTED]
> Module: openpkg-src Date: 20-Jul-2005 13:36:10
> Branch: HEAD Handle: 2005072012360901
>
> Added files:
> openpkg-src/bacula bacula.patch bacula.spec bconsole.sh rc.bacula
>
> Log:
> new package: bacula 1.37.30 (Network backup tool)
> [...]
Cool. Thanks for this contribution, Matthias. In my role as the
OpenPKG Principal Architect I've reviewed this new package and here
is my feedback (already in the form of a quoted patch with inserted
annotations):
| Index: bacula.patch
| ===================================================================
| RCS file: /v/openpkg/cvs/openpkg-src/bacula/bacula.patch,v
| retrieving revision 1.1
| diff -u -d -u -d -u -d -r1.1 bacula.patch
| --- bacula.patch 20 Jul 2005 11:36:09 -0000 1.1
| +++ bacula.patch 20 Jul 2005 18:38:40 -0000
| @@ -1,14 +1,6 @@
| Index: configure
| --- configure.orig 2005-06-18 15:34:19.000000000 +0200
| +++ configure 2005-07-20 09:26:54.328806000 +0200
| -@@ -5932,6 +5932,7 @@
| -
| - got_readline="no"
| - READLINE_SRC=
| -+#set -x
| - if test x$support_readline = xyes; then
| -
| - # Check whether --with-readline or --without-readline was given.
| @@ -5942,15 +5943,15 @@
| no) : ;;
| yes|*)
This just removes a useless hunk of the patch to reduce its size and to
clean it up.
| @@ -69,48 +61,6 @@
| _ACEOF
| if { (eval echo "$as_me:$LINENO: \"$ac_cpp conftest.$ac_ext\"") >&5
| (eval $ac_cpp conftest.$ac_ext) 2>conftest.er1
| -@@ -6045,25 +6046,25 @@
| - # So? What about this header?
| - case $ac_header_compiler:$ac_header_preproc:$ac_c_preproc_warn_flag in
| - yes:no: )
| -- { echo "$as_me:$LINENO: WARNING: ${with_readline}/readline.h: accepted
by the compiler, rejected by the preprocessor!" >&5
| --echo "$as_me: WARNING: ${with_readline}/readline.h: accepted by the
compiler, rejected by the preprocessor!" >&2;}
| -- { echo "$as_me:$LINENO: WARNING: ${with_readline}/readline.h:
proceeding with the compiler's result" >&5
| --echo "$as_me: WARNING: ${with_readline}/readline.h: proceeding with the
compiler's result" >&2;}
| -+ { echo "$as_me:$LINENO: WARNING:
${with_readline}/include/readline/readline.h: accepted by the compiler,
rejected by the preprocessor!" >&5
| -+echo "$as_me: WARNING: ${with_readline}/include/readline/readline.h:
accepted by the compiler, rejected by the preprocessor!" >&2;}
| -+ { echo "$as_me:$LINENO: WARNING:
${with_readline}/include/readline/readline.h: proceeding with the compiler's
result" >&5
| -+echo "$as_me: WARNING: ${with_readline}/include/readline/readline.h:
proceeding with the compiler's result" >&2;}
| - ac_header_preproc=yes
| - ;;
| - no:yes:* )
| -- { echo "$as_me:$LINENO: WARNING: ${with_readline}/readline.h: present
but cannot be compiled" >&5
| --echo "$as_me: WARNING: ${with_readline}/readline.h: present but cannot be
compiled" >&2;}
| -- { echo "$as_me:$LINENO: WARNING: ${with_readline}/readline.h: check
for missing prerequisite headers?" >&5
| --echo "$as_me: WARNING: ${with_readline}/readline.h: check for missing
prerequisite headers?" >&2;}
| -- { echo "$as_me:$LINENO: WARNING: ${with_readline}/readline.h: see the
Autoconf documentation" >&5
| --echo "$as_me: WARNING: ${with_readline}/readline.h: see the Autoconf
documentation" >&2;}
| -- { echo "$as_me:$LINENO: WARNING: ${with_readline}/readline.h:
section \"Present But Cannot Be Compiled\"" >&5
| --echo "$as_me: WARNING: ${with_readline}/readline.h: section \"Present
But Cannot Be Compiled\"" >&2;}
| -- { echo "$as_me:$LINENO: WARNING: ${with_readline}/readline.h:
proceeding with the preprocessor's result" >&5
| --echo "$as_me: WARNING: ${with_readline}/readline.h: proceeding with the
preprocessor's result" >&2;}
| -- { echo "$as_me:$LINENO: WARNING: ${with_readline}/readline.h: in the
future, the compiler will take precedence" >&5
| --echo "$as_me: WARNING: ${with_readline}/readline.h: in the future, the
compiler will take precedence" >&2;}
| -+ { echo "$as_me:$LINENO: WARNING:
${with_readline}/include/readline/readline.h: present but cannot be compiled"
>&5
| -+echo "$as_me: WARNING: ${with_readline}/include/readline/readline.h:
present but cannot be compiled" >&2;}
| -+ { echo "$as_me:$LINENO: WARNING:
${with_readline}/include/readline/readline.h: check for missing
prerequisite headers?" >&5
| -+echo "$as_me: WARNING: ${with_readline}/include/readline/readline.h:
check for missing prerequisite headers?" >&2;}
| -+ { echo "$as_me:$LINENO: WARNING:
${with_readline}/include/readline/readline.h: see the Autoconf documentation"
>&5
| -+echo "$as_me: WARNING: ${with_readline}/include/readline/readline.h: see
the Autoconf documentation" >&2;}
| -+ { echo "$as_me:$LINENO: WARNING:
${with_readline}/include/readline/readline.h: section \"Present But Cannot
Be Compiled\"" >&5
| -+echo "$as_me: WARNING: ${with_readline}/include/readline/readline.h:
section \"Present But Cannot Be Compiled\"" >&2;}
| -+ { echo "$as_me:$LINENO: WARNING:
${with_readline}/include/readline/readline.h: proceeding with the
preprocessor's result" >&5
| -+echo "$as_me: WARNING: ${with_readline}/include/readline/readline.h:
proceeding with the preprocessor's result" >&2;}
| -+ { echo "$as_me:$LINENO: WARNING:
${with_readline}/include/readline/readline.h: in the future, the compiler will
take precedence" >&5
| -+echo "$as_me: WARNING: ${with_readline}/include/readline/readline.h: in the
future, the compiler will take precedence" >&2;}
| - (
| - cat <<\_ASBOX
| - ## ------------------------------------------ ##
| @@ -6074,8 +6075,8 @@
| sed "s/^/$as_me: WARNING: /" >&2
| ;;
This removes another large hunk of the patch to further reduce its
size. In general I always like to be "change complete" (what this
hunk does by patching even the error messages), but .patch files have
to be maintained over a long time and hence we _HAVE_ to reduce it
to its absolute minimum (especially here were we patch a _GENERATED_
file 'configure'!). As a result we have to intentionally break the
rule of being "change complete" because maintainance for a .patch is
more important than being 100% correct even in the error messages of a
"configure" script.
| Index: bacula.spec
| ===================================================================
| RCS file: /v/openpkg/cvs/openpkg-src/bacula/bacula.spec,v
| retrieving revision 1.2
| diff -u -d -u -d -u -d -r1.2 bacula.spec
| --- bacula.spec 20 Jul 2005 17:10:00 -0000 1.2
| +++ bacula.spec 20 Jul 2005 18:38:40 -0000
| @@ -59,18 +59,24 @@
| # build information
| Prefix: %{l_prefix}
| BuildRoot: %{l_buildroot}
| -BuildPreReq: OpenPKG, openpkg >= 20040712, gcc, readline, odoc
| -PreReq: OpenPKG, openpkg >= 20040712, readline
| +BuildPreReq: OpenPKG, openpkg >= 20040712, gcc
| +PreReq: OpenPKG, openpkg >= 20040712
| +BuildPreReq: readline, zlib
| +PreReq: readline, zlib
No offense here, but I removed "odoc" for now until have it finally
rolled out. I also added "zlib" because Bacula requires it, too. And I
placed readline and zlib onto own lines to better line up and to be more
in sync with the style of the other OpenPKG packages.
| %if "%{with_tls}" == "yes"
| +BuildPreReq: openssl >= 0.9.8
| PreReq: openssl >= 0.9.8
| %endif
| %if "%{with_db}" == "sqlite"
| +BuildPreReq: sqlite
| PreReq: sqlite
| %endif
| %if "%{with_db}" == "mysql"
| +BuildPreReq: mysql
| PreReq: mysql
| %endif
| %if "%{with_db}" == "postgres"
| +BuildPreReq: postgresql
| PreReq: postgresql
| %endif
| %if "%{with_dvd}" == "yes"
| @@ -105,13 +111,11 @@
For technical reasons (which I still horribly dislike but cannot
change easily) in OpenPKG dependencies to libraries have to be both
"BuildPreReq" (build-time) and "PreReq" (run-time). Just "PreReq" would
work only for a shared libraries only world.
| %build
| # configure
| - ./configure --help
I removed this "configure" call because I really hope that it (or some
of its side-effects) is not required and was just a left-over from you.
If it has to be there for whatever nasty reasons, A COMMENT HAS TO
DOCUMENT THIS.
| case "%{with_db}" in
| - sqlite ) cfg_db="--with-sqlite=%{l_prefix}";;
| - mysql ) cfg_db="--with-mysql=%{l_prefix}";;
| - postgres ) cfg_db="--with-postgresql=%{l_prefix}";;
| - * ) echo "with_db must be in (sqlite|mysql|postgres)" 1>&2
| - exit 1;;
| + sqlite ) cfg_db="--with-sqlite=%{l_prefix}" ;;
| + mysql ) cfg_db="--with-mysql=%{l_prefix}" ;;
| + pgsql ) cfg_db="--with-postgresql=%{l_prefix}" ;;
| + * ) echo "with_db must be in (sqlite|mysql|pgsql)" 1>&2; exit 1
;;
This is just cosmetics to align the style with our usual Shell-Script
style of all other packages: consistent leading spaces and at least one
blank before ";;". I know, this is all about subjective cosmetics only,
of course.
| esac
| CC="%{l_cc}" \
| CFLAGS="%{l_cflags -O}" \
| @@ -144,12 +148,14 @@
| --with-working-dir=%{l_prefix}/var/bacula \
| --with-pid-dir=%{l_prefix}/var/bacula/run \
| --with-subsys-dir=%{l_prefix}/var/bacula/run/subsys
| +
| # build
| %{l_make} %{l_mflags -O}
|
| %install
| rm -rf $RPM_BUILD_ROOT
|
| + # create installation hierarchy
| %{l_shtool} mkdir -f -p -m 755 \
| $RPM_BUILD_ROOT%{l_prefix}/bin \
| $RPM_BUILD_ROOT%{l_prefix}/sbin \
| @@ -165,20 +171,24 @@
| $RPM_BUILD_ROOT%{l_prefix}/share/bacula/examples/default-config \
| $RPM_BUILD_ROOT%{l_docdir}/bacula
|
| - %{l_make} %{l_mflags} DESTDIR=$RPM_BUILD_ROOT install
| + # install
| + %{l_make} %{l_mflags} install DESTDIR=$RPM_BUILD_ROOT
| +
| + # strip down installation
Again, just cosmetics to align style to other packages. You can just
ignore this, that's nothing more than the usual amount of RSE cosmetics
everybody "likes".
| strip $RPM_BUILD_ROOT%{l_prefix}/sbin/* 2>/dev/null || true
| ( cd $RPM_BUILD_ROOT%{l_prefix}/libexec/bacula &&
| for unwanted in bconsole startmysql stopmysql; do
| - rm -f $unwanted;
| + rm -f $unwanted
Just removed unnecessary semicolon.
| done
| ) || exit $?
| rm -f $RPM_BUILD_ROOT%{l_prefix}/man/man8/bacula.8.gz
|
| + # install additional files
| %{l_shtool} install -c -m 755 %{l_value -s -a} \
| %{SOURCE bconsole.sh} $RPM_BUILD_ROOT%{l_prefix}/bin/bconsole
| +
| + # install run-command script
Again, just cosmetics to make comments across whole package
more consistent.
| %{l_shtool} install -c -m 644 %{l_value -s -a} \
| - -e 's,@busr@,%{with_user},g' \
| - -e 's,@bgrp@,%{with_group},g' \
| -e 's,@with_server@,%{with_server},g' \
| %{SOURCE rc.bacula} $RPM_BUILD_ROOT%{l_prefix}/etc/rc.d/
This is because of a change below. See there...
| @@ -186,13 +196,8 @@
| %{l_shtool} install -c -m 644 \
| -e 's,/usr/share/doc/bacula-<version>,%{l_prefix}/doc/bacula,' \
| scripts/bacula.man $RPM_BUILD_ROOT%{l_prefix}/man/man8/bacula.8
| - if %{l_odoc} -F
| - then echo "WARNING: will not package auxiliary documentation" 1>&2
| - else mv ../bacula-doc-%{V_doc}/* $RPM_BUILD_ROOT%{l_docdir}/bacula
| - %{l_odoc} -ame %{_specdir}/bacula.spec
| - fi
Remove "odoc" support for now. But even with "odoc" all this doesn't
really work: the %{l_odoc} is not defined unless "odoc" becomes part of
the "openpkg" package.
| - # create file list
| + # determine installation files
Cosmetics: just align with the usual comment we have at this place in a .spec.
| %{l_rpmtool} files -v -ofiles -r$RPM_BUILD_ROOT \
| %{l_files_std} \
| '%attr(750,%{with_user},%{with_group}) %{l_prefix}/var/bacula' \
| @@ -204,19 +209,28 @@
| '%config %{l_prefix}/etc/bacula/bacula-dir.conf'
|
| %files -f files
| - %docdir %{l_docdir}
If this is really wished, it should go into the "%{l_rpmtool} files"
call and not into %files as OpenPKG doesn't use %files section content
at all.
| %clean
| rm -rf $RPM_BUILD_ROOT
|
| %post
| + # create initial database
| + if [ ! -f $RPM_INSTALL_PREFIX/var/bacula/bacula.db ]; then
| + $RPM_INSTALL_PREFIX/libexec/bacula/make_bacula_tables
| + chmod 600 $RPM_INSTALL_PREFIX/var/bacula/bacula.db
| + chown %{with_user}:%{with_group}
$RPM_INSTALL_PREFIX/var/bacula/bacula.db
| + fi
| +
This I've moved to here from rc.bacula because creating a database
is something for the install step and not of the startup, I think.
Especially, that's the way we are doing it in all other packages. We
have no package which creates such files in %start of the rc.<name>.
| # after upgrade, restart service
| [ $1 -eq 2 ] || exit 0
| - %{l_rc} -v bacula restart
| + eval `%{l_rc} bacula status 2>/dev/null`
| + [ ".$bacula_active" = .yes ] && %{l_rc} bacula restart
| + exit 0
|
| %preun
| # before erase, stop service and remove working files
| - %{l_rc} -v bacula stop
| - rm -rf $RPM_INSTALL_PREFIX/var/bacula
| + [ $1 -eq 0 ] || exit 0
| + %{l_rc} bacula stop 2>/dev/null
| + rm -rf $RPM_INSTALL_PREFIX/var/bacula/*
| exit 0
Here I aligned the code with the code of all other packages. It
especially adds additional if's and removes only the content of the
"var/bacula" dir, but not the "var/bacula" dir itself (because RPM
controls the "var/bacula" dir itself and hence removes it for us.
| Index: rc.bacula
| ===================================================================
| RCS file: /v/openpkg/cvs/openpkg-src/bacula/rc.bacula,v
| retrieving revision 1.1
| diff -u -d -u -d -u -d -r1.1 rc.bacula
| --- rc.bacula 20 Jul 2005 11:36:10 -0000 1.1
| +++ rc.bacula 20 Jul 2005 18:38:40 -0000
| @@ -9,41 +9,31 @@
| [EMAIL PROTECTED]@
| bacula_fd_enable=yes
| bacula_debug=""
| - [EMAIL PROTECTED]@;
| - [EMAIL PROTECTED]@
Remove unnecessary defines. See above.
| %status -u @l_susr@ -o
| + bacula_usable="no"
| + bacula_active="no"
| @l_prefix@/libexec/bacula/bacula status \
| - "$bacula_debug" $bacula_dir_enable $bacula_sd_enable
$bacula_fd_enable
| + "$bacula_debug" $bacula_dir_enable $bacula_sd_enable
$bacula_fd_enable && \
| + bacula_active="yes"
| + echo "bacula_enable=\"$bacula_enable\""
| + echo "bacula_usable=\"$bacula_usable\""
| + echo "bacula_active=\"$bacula_active\""
This is the only part where I'm unsure about the "bacula status" call.
But important is that OpenPKG %status sections are _NOT_ intended for
arbitrary status outputs. Instead all packages are using a standardized
output. Additionally, the providing of "bacula_active" is important for
"%start" and "%stop" below. But I guess the args to "bacula status" can
be removed, right?
| %start -u @l_susr@
| rcService bacula enable yes || exit 0
| - if [ ! -e @l_prefix@/var/bacula/bacula.db ]; then
| - # create database
| - @l_prefix@/libexec/bacula/make_bacula_tables
| - chmod 600 @l_prefix@/var/bacula/bacula.db
| - chown $bacula_user:$bacula_group @l_prefix@/var/bacula/bacula.db
| - fi
| + rcService bacula active yes && exit 0
| @l_prefix@/libexec/bacula/bacula start \
| "$bacula_debug" $bacula_dir_enable $bacula_sd_enable
$bacula_fd_enable
This code was moved to bacula.spec and the usual "active" check was added.
But this still requires that the "bacula status" call above is correct.
| %stop -u @l_susr@
| rcService bacula enable yes || exit 0
| + rcService bacula active no && exit 0
| @l_prefix@/libexec/bacula/bacula stop
|
| %restart -u @l_susr@
| rcService bacula enable yes || exit 0
| - @l_prefix@/libexec/bacula/bacula stop
| + rc bacula stop
| sleep 5
| - @l_prefix@/libexec/bacula/bacula start \
| - "$bacula_debug" $bacula_dir_enable $bacula_sd_enable
$bacula_fd_enable
| -
| -%reload -u @l_susr@
| - rcService bacula enable yes || exit 0
| - echo "bacula: reload not supported !" 1>&2
| - exit 1
| -
| -%daily -u @l_susr@
| - rcService bacula enable yes || exit 0
| - exit 0
| + rc bacula start
Here I've used the usual "rc <name>" call to remove redundancy (the
"bacula stop" and "bacula start" commands are already in %start and
%stop, so just use them in %restart!). I've also removed %reload and
%daily because they don't do anything (outputting an error is not
necessary, "rc" does it, too).
Appended is the patch again without annotations. Except for the issue
with the "bacula status" command I would like to commit this if you have
no objections from your point of view, Matthias?
Ralf S. Engelschall
[EMAIL PROTECTED]
www.engelschall.com
______________________________________________________________________
The OpenPKG Project www.openpkg.org
Developer Communication List openpkg-dev@openpkg.org
