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

From: Mark Rada <marada@uwaterloo.ca>
Date: Apr 2, 2010, 17:28 UTC
Message-ID: <4BB6294A.7020800@mailservices.uwaterloo.ca>
In-Reply-To: <CBD7C6CF-01CB-4525-8AAB-B1E8086CA06E@mailservices.uwaterloo.ca>

Show 10 quoted lines

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

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

 
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