[m-rev.] for review: deep profiler installation

[Fergus Henderson]

On 03-Jul-2001, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Index: configure.in
...
> +AC_CHECK_HEADER(signal.h, HAVE_SIGNAL_H=1)
> +if test "$HAVE_SIGNAL_H" = 1; then
> +	AC_DEFINE(HAVE_SIGNAL)
> +fi

It's not worth doing an autoconf test for <signal.h>, since
that header file is required by the C standard (both C89 and C99).
There's already lots of places in the Mercury compiler where we
unconditionally #include <signal.h>.

>  AC_CHECK_HEADER(sys/wait.h, HAVE_SYS_WAIT_H=1)
>  if test "$HAVE_SYS_WAIT_H" = 1; then
>  	AC_DEFINE(HAVE_SYS_WAIT)
> @@ -1767,16 +1772,20 @@
>  #
>  # Add an option that disables the deep profiler.
>  #
> +AC_ARG_ENABLE(deep-profiler,
> +[  --enable-deep-profiler=...
> +                          install deep profiler CGI script in this directory],

I suggest

[  --enable-deep-profiler=<directory>
                          install the deep profiler CGI scripts in <directory>]

or

[  --enable-deep-profiler[=<directory>]
                          enable the deep profiler, and install the
                          deep profiler CGI scripts in <directory>
			  (default <directory> is /usr/lib/cgi-bin)],

(you'd probably need to prefix the inner [ and ] with backslashes).

>      # This test may need to be made more specific at a later date.
>      # Currently it only distinguishes between systems which have
>      # unistd.h or not, but at a later date we may also need to test for
>      # other posix features.
> -if test "$HAVE_UNISTD_H" = 1; then
> -    mercury_cv_enable_deep_profiler=yes
> +if test "$HAVE_UNISTD_H" = 1 -a "$HAVE_SIGNAL_H" = 1; then
> +    mercury_cv_can_enable_deep_profiler=yes
>  else
> -    mercury_cv_enable_deep_profiler=no
> +    mercury_cv_can_enable_deep_profiler=no
>  fi

No point testing for $HAVE_SIGNAL_H here (see above).

> +mercury_cv_default_cgi_dir=/usr/lib/cgi-bin
> +case $enable_deep_profiler in
> +	default)
> +		mercury_cv_enable_deep_profiler=$mercury_cv_can_enable_deep_profiler
> +		mercury_cv_cgi_dir=$mercury_cv_default_cgi_dir
> +		;;
> +	no)
> +		mercury_cv_enable_deep_profiler=no
> +		;;
> +	*)
> +		if test $enable_deep_profiler = yes; then
> +			mercury_cv_cgi_dir=$mercury_cv_default_cgi_dir
> +		else
> +			mercury_cv_cgi_dir=$enable_deep_profiler
> +		fi
> +
> +		if test $mercury_cv_can_enable_deep_profiler = no; then
> +			echo
> +			AC_MSG_ERROR(--enable-deep-profiler specified but system does not support it)
> +			exit 1
> +		fi
> +
> +		if test -d $mercury_cv_cgi_dir; then
> +			mercury_cv_enable_deep_profiler=yes
> +		else
> +			echo
> +			AC_MSG_ERROR(--enable-deep-profiler specified but $mercury_cv_cgi_dir does not exist)
> +			exit 1
> +		fi		
> +		;;
> +esac

That test that $mercury_cv_cgi_dir exists should also be done in the `default'
case, or afterwards, outside the case statement, so that you catch the case
when the user specifies `--enable-deep-profiler', but /usr/lib/cgi-bin
does not exist.

> Index: scripts/Mmakefile:
> +.PHONY: install_cgi_scripts
> +install_cgi_scripts: $(CGI_SCRIPTS)
> +       # $(INSTALL_CGI_DIR) is likely to be writeable only by root or
> +       # the www system administrator, which is why we don't try to install
> +       # it here. (If we did try, the action would have to be conditional on
> +       # $(ENABLE_DEEP_PROFILER).) Instead, the install action in ../Mmakefile
> +       # will remind the user to do the copy later.
> +       # -cp $(CGI_SCRIPTS) $(INSTALL_CGI_DIR)

It's not uncommon to do `make install' as root (many packages require that).
I think it would be nicer to handle that case automatically, and only
require the user to do the install manually if the automatic install
failed.  This can be done quite easily, by uncommenting the `-cp' line
above, and making the reminder in ../Mmakefile conditional on the
file not having already been installed.

> +++ Mmakefile   2001/07/03 03:35:02
> @@ -329,6 +329,9 @@
>         @echo "-- Don't forget to add $(INSTALL_BINDIR) to your PATH,"
>         @echo "-- $(INSTALL_MAN_DIR) to your MANPATH,"
>         @echo "-- and $(INSTALL_INFO_DIR) to your INFOPATH,"
> +       @if test $(ENABLE_DEEP_PROFILER) = yes; then \
> +       echo "-- to copy scripts/mdprof to $(INSTALL_CGI_DIR),"; \
> +       fi
>         @echo "-- and to add the following lines to the \`.emacs' file"
>         @echo "-- in your home directory:"
>         @echo " (setq load-path (cons (expand-file-name "

Change that to

	@if test $(ENABLE_DEEP_PROFILER) = yes && \
		! cmp -s scripts/mdprof $(INSTALL_CGI_DIR)/mdprof; \
	then \
		echo "-- to copy scripts/mdprof to $(INSTALL_CGI_DIR),"; \
	fi

or in fact

	@if test $(ENABLE_DEEP_PROFILER) != yes || \
		cmp -s scripts/mdprof $(INSTALL_CGI_DIR)/mdprof; \
	then : else \
		echo "-- to copy scripts/mdprof to $(INSTALL_CGI_DIR),"; \
	fi

since if I recall correctly using `!' for negation is not portable(?).

Otherwise that looks fine, but I'd like to see another diff.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------