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

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

[Mark Rada]

This will allow users to set a JavaScript/CSS minifier when/if they run
the autoconfigure script while building git. This is much more
convenient than editing Makefile and gitweb/Makefile manually.

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(-)

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 =
-
 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);
+ ])
+
 ## Site configuration (override autodetection)
 ## --with-PACKAGE[=ARG] and --without-PACKAGE
 AC_MSG_NOTICE([CHECKS for site configuration])
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 ?=
-
 # 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)
 
-- 
1.7.0.3.436.g45b2d

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

[Jakub Narebski]

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.

> 
> 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.

> 
> 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.

>  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?

[...]
> 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.

>  # 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

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

[Mark Rada]

>> 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.
>
 
I don't understand what you mean by this. Are you saying that the
patch is good as is?
 
 
>> 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?
 
I think this is what I am really not understanding here. Are you saying
that you think it would be better to use —with-OPT=PATH instead of
—enable-OPT=PATH? 

Is this just a preference? I'm not seeing the problem with —enable-OPT..
This is confusing me a bit...

Thank you for the input.

—
Mark Rada
marada@uwaterloo.ca

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

[Jakub Narebski]

Mark Rada wrote:

>>> 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.
>  
> I don't understand what you mean by this. Are you saying that the
> patch is good as is?

Yes, I think that while it could be improved a bit, it is good as
it is now.

I'm sorry for causing confusion: at first I was wondering why you do 
not use JSMIN=@JSMIN@ etc. in config.mak.in, but then I noticed that
other existing configuration uses "add to config.mak.append" trick.

[...]
>>> +# 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).

This is about alternate solution; please discard this question.

>> 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?
>  
> I think this is what I am really not understanding here. Are you saying
> that you think it would be better to use —with-OPT=PATH instead of
> —enable-OPT=PATH? 
> 
> Is this just a preference? I'm not seeing the problem with —enable-OPT..
> This is confusing me a bit...

Well, I haven't noticed that GIT_PARSE_WITH_SET_MAKE_VAR uses 
—with-OPT=PATH and not —enable-OPT=PATH.

What would be nice to have is GIT_PARSE_ENABLE_SET_MAKE_VAR macro... but
for only two callsites it might be overkill.

-- 
Jakub Narebski
Poland