Daniel Kreling <krel...@linux.vnet.ibm.com> writes:
> This patch enables iprutils supportconfig plugin for SLES when the user
> specifies ./configure --enable-supportconfig. Autotools will send iprsos
> script
> to /usr/lib/supporconfig/plugins and when one issues the supportconfig
> command,
> the script will generate a report containing the plugin-iprsos.txt file.
> If one wants the iprsos be sent to another directory, s/he must specify it
> with
> --enable-supportconfig=<path>.
Hi Daniel,
I have a few comments. I hope I am reviewing the right version. :-)
So, supportconfig seems to be the counterpart of the sosreport, but for
other distros. I suppose it is going to be installed on SUSE distros
only? I recall that Jakub mentioned there is a separate repository to
hold all sosreport plugins for RHEL/Fedora. Maybe this is also the case
for supportconfig? Did you check if there is any external repository
where we could merge this?
Assuming there isn't, I still have some comments inline.
Thanks for the patch!
>
> Signed-off-by: Daniel Kreling <krel...@linux.vnet.ibm.com>
> ---
> Makefile.am | 4 ++++
> configure.ac | 19 +++++++++++++++++++
> 2 files changed, 23 insertions(+)
>
> diff --git a/Makefile.am b/Makefile.am
> index 4636ecb..121ec3f 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -53,3 +53,7 @@ if SOSREPORT
> sosreportdir = @pythondir@/sos/plugins
> sosreport_PYTHON = iprutils.py
> endif
> +
> +if SUPPORTCONFIG
> +supportconfig_SCRIPTS = iprsos
> +endif
At line 14 of this file, it already installs iprsos in ${exec_prefix}.
I see no point in duplicating installed code, so I'm not sure how to
proceed here. Did you want iprsos and supportconfig to be the same? If
so, maybe we should do some 'ln -s' magic at packaging time instead of
installing the same script twice?
> diff --git a/configure.ac b/configure.ac
> index 6e25fe3..8bee2de 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -55,6 +55,25 @@ AC_ARG_ENABLE([sosreport],
> esac], [sosreport=true])
> AM_CONDITIONAL([SOSREPORT], [test "x${sosreport}" = xtrue])
>
> +# --enable-supportconfig.
> +supportconfigdir='${exec_prefix}/usr/lib/supportconfig/plugins'
> +AC_ARG_ENABLE([supportconfig],
> + [--enable-supportconfig Install supportconfig plugin],
Since we only build this for a few distros, I suggest keeping this
option disabled by default.
> +[case "${enableval}" in
> + yes)
> + supportconfig=true
> + ;;
> + no)
> + supportconfig=false
> + ;;
> + *)
> + supportconfig=true;
> + supportconfigdir="$withval"
> + ;;
^^^^
Trailing white space here.
When I attempted to change the default installation prefix for
supportconfig it failed to install in the right path. I believe it is
because you used $withval, which is not defined for AC_ARG_ENABLE.
You should use $enableval. To reproduce, just run:
$ ./configure --enable-supportconfig=build/bla --prefix=build
$ make && make install
> + esac], [supportconfig=true])
> +AM_CONDITIONAL([SUPPORTCONFIG], [test "x${supportconfig}" = xtrue])
> +AC_SUBST(supportconfigdir, ${supportconfigdir})
> +
> # --with-systemd
> systemdunitdir='${exec_prefix}/usr/lib/systemd/system/'
> AC_ARG_WITH([systemd],
This patch depends on my last Autotools patch set that is not upstream
yet. Let's queue it to be pushed after mine, in order to avoid
unnecessary merge pain. :-)
Thanks!
--
Gabriel Krisman Bertazi
------------------------------------------------------------------------------
_______________________________________________
Iprdd-devel mailing list
Iprdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iprdd-devel
