Re: [PATCHv5 3/6] Gitweb: add autoconfigure support for minifiers

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 1, 2010, 22:40 UTC
Message-ID: <201004020040.26177.jnareb@gmail.com>
In-Reply-To: <4BB430D9.1090900@mailservices.uwaterloo.ca>

On Thu, 1 Apr 2010, Mark Rada wrote:

> This will allow users to set a JavaScript/CSS minifier when/if they run
> the autoconfigure script while building git.

This is a good idea, IMHO.

>                                              This is much more 
> convenient than editing Makefile and gitweb/Makefile manually.

Actually recommended way of customizing the build if one doesn't want to use ./configure script (besides providing values in command line when invoking make) is not to edit Makefile, but to add appropriate definitions to [untracked] config.mak file.

Show 11 quoted lines

> 
> Signed-off-by: Mark Rada <marada@uwaterloo.ca>
> 
> ---
> 
> 	No changes since the previous version.
> 
>  Makefile        |    4 ----
>  configure.ac    |   20 ++++++++++++++++++++
>  gitweb/Makefile |   14 ++------------
>  3 files changed, 22 insertions(+), 16 deletions(-)

Why there is no change to config.mak.in? I would thought that it would contain JSMIN=@JSMIN@ etc.

But see also below: perhaps current version is a better version.

Show 13 quoted lines

> 
> diff --git a/Makefile b/Makefile
> index 450e4df..ef1a232 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -282,10 +282,6 @@ lib = lib
>  # DESTDIR=
>  pathsep = :
>  
> -# JavaScript/CSS minifier invocation that can function as filter
> -JSMIN =
> -CSSMIN =
> -

I think it is a good change anyway, as we unset variables in Makefile only in "Guard against environment variables" for variables such as LIB_OBJS. So even without support for JSMIN/CSSMIN in ./configure it would be a good change.

Show 31 quoted lines

>  export prefix bindir sharedir sysconfdir
>  
>  CC = gcc
> diff --git a/configure.ac b/configure.ac
> index 914ae57..bf36c72 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -179,6 +179,26 @@ fi],
>     AC_MSG_NOTICE([Will try -pthread then -lpthread to enable POSIX Threads.])
>  ])
>  
> +# Define option to enable JavaScript minification
> +AC_ARG_ENABLE([jsmin],
> + [AS_HELP_STRING([--enable-jsmin=ARG],
> +   [ARG is the value to pass to make to enable JavaScript minification.])],
> + [
> +   JSMIN=$enableval;
> +   AC_MSG_NOTICE([Setting JSMIN to '$JSMIN' to enable JavaScript minifying])
> +   GIT_CONF_APPEND_LINE(JSMIN=$enableval);
> + ])
> +
> +# Define option to enable CSS minification
> +AC_ARG_ENABLE([cssmin],
> + [AS_HELP_STRING([--enable-cssmin=ARG],
> +   [ARG is the value to pass to make to enable CSS minification.])],
> + [
> +   CSSMIN=$enableval;
> +   AC_MSG_NOTICE([Setting CSSMIN to '$CSSMIN' to enable CSS minifying])
> +   GIT_CONF_APPEND_LINE(CSSMIN=$enableval);
> + ])
> +

Why not follow the code as it was done e.g. for iconv (--without-iconv and --with-iconv=path); this would require JSMIN=@JSMIN@ in config.mak.in (and respectively for CSSMIN).

Alternatively, if you decide on appending to config.mak.autogen (by the way of config.mak.append) instead of filling config.mak.in, why not use ready macro GIT_PARSE_WITH_SET_MAKE_VAR?

[...]

Show 12 quoted lines

> diff --git a/gitweb/Makefile b/gitweb/Makefile
> index fffe700..ffee4bd 100644
> --- a/gitweb/Makefile
> +++ b/gitweb/Makefile
> @@ -14,10 +14,6 @@ prefix ?= $(HOME)
>  bindir ?= $(prefix)/bin
>  RM ?= rm -f
>  
> -# JavaScript/CSS minifier invocation that can function as filter
> -JSMIN ?=
> -CSSMIN ?=
> -

I can understand that this was not needed anyway.

Show 33 quoted lines

>  # default configuration for gitweb
>  GITWEB_CONFIG = gitweb_config.perl
>  GITWEB_CONFIG_SYSTEM = /etc/gitweb.conf
> @@ -30,18 +26,10 @@ GITWEB_STRICT_EXPORT =
>  GITWEB_BASE_URL =
>  GITWEB_LIST =
>  GITWEB_HOMETEXT = indextext.html
> -ifdef CSSMIN
> -GITWEB_CSS = gitweb.min.css
> -else
>  GITWEB_CSS = gitweb.css
> -endif
>  GITWEB_LOGO = git-logo.png
>  GITWEB_FAVICON = git-favicon.png
> -ifdef JSMIN
> -GITWEB_JS = gitweb.min.js
> -else
>  GITWEB_JS = gitweb.js
> -endif
>  GITWEB_SITE_HEADER =
>  GITWEB_SITE_FOOTER =
>  
> @@ -95,9 +83,11 @@ all:: gitweb.cgi
>  FILES = gitweb.cgi
>  ifdef JSMIN
>  FILES += gitweb.min.js
> +GITWEB_JS = gitweb.min.js
>  endif
>  ifdef CSSMIN
>  FILES += gitweb.min.css
> +GITWEB_CSS = gitweb.min.css
>  endif
>  gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)

O.K., this change means that it is two conditionals less...

-- 
Jakub Narebski
Poland