LGTM, thanks for taking the time to do it well!
On Nov 11, 2015 2:24 AM, "Martell Malone" wrote:
> You changed from --enable to --with, but left the conditional name as
>> ENABLE_GENLIB. Change to WITH_GENLIB.
>> Makefile.am:
>> Don't test WITH_GENLIB twice. Test it once and set all the variables
>> there. Move:
>> + AM_DLLTOOLFLAGS=-o $@
>> and its twin into the respective conditional paths. Move:
>> if DELAY_IMPORT_LIBS
>>AM_DLLTOOLFLAGS += --output-delaylib $@.delayimp.a
>> endif
>> after the WITH_GENLIB conditional.
>
> Yeah I knew I done that. I thought the variable name could have stayed the
> same.
> Laziness on my part there.
>
> Anyway here is an updated patch to address your comments.
> It is in .txt format :)
>
> On Mon, Nov 9, 2015 at 11:47 PM, NightStrike
> wrote:
>
>> configure.ac:
>>
>> You missed AS_VAR_SET:
>> [GENLIB=$with_genlib]
>> [AS_VAR_SET([GENLIB], [$with_genlib])]
>>
>> This handles quoting very portably, and is more usable wrt indirection
>> for when someone copies this code elsewhere to do a similar change.
>>
>> You also forgot to remove AC_SUBST, which is redundant with AC_CHECK_TOOL.
>>
>> You changed from --enable to --with, but left the conditional name as
>> ENABLE_GENLIB. Change to WITH_GENLIB.
>>
>> Makefile.am:
>> Don't test WITH_GENLIB twice. Test it once and set all the variables
>> there. Move:
>>
>> + AM_DLLTOOLFLAGS=-o $@
>>
>> and its twin into the respective conditional paths. Move:
>>
>> if DELAY_IMPORT_LIBS
>>AM_DLLTOOLFLAGS += --output-delaylib $@.delayimp.a
>> endif
>>
>> after the WITH_GENLIB conditional.
>>
>>
>> Send patches as .txt, as google screws up the encoding / mime type.
>>
>>
>> On Tue, Nov 10, 2015 at 2:08 AM, Martell Malone
>> wrote:
>> > Thanks for your input and help on this.
>> > Please review :)
>> >
>> > On Sun, Nov 8, 2015 at 10:05 PM, NightStrike
>> wrote:
>> >>
>> >> ...and, as such, you have to provide a way to set the name.
>> >> --with-genlib=mygenlib. You can find examples of this elsewhere in
>> >> the build system. This means that you don't call AC_CHECK_TOOL until
>> >> after the with- processing, and only if with_genlib is not set to no.
>> >> AS_CASE is appropriate for that checking.
>> >>
>> >> On Mon, Nov 9, 2015 at 1:02 AM, NightStrike
>> wrote:
>> >> > Sorry, missed this the first time around... this should be a with-
>> >> > option, not enable. genlib is an external tool for the crt, not an
>> >> > internal feature that is getting compiled in. You should just have
>> to
>> >> > change to AC_ARG_WITH and from enable_ to with_.
>> >> >
>> >> > On Mon, Nov 9, 2015 at 12:46 AM, Martell Malone
>> >> > wrote:
>> >> >> Here is the final patch after running autoreconf -fi and only
>> applying
>> >> >> relevant changes.
>> >> >> I would like to apply this if there are no objections ?
>> >> >>
>> >> >> Kind Regards
>> >> >> Martell
>> >> >>
>> >> >> On Fri, Nov 6, 2015 at 3:39 PM, Martell Malone
>> >> >>
>> >> >> wrote:
>> >> >>>
>> >> >>> Updated to reflect Nightstrike's feedback
>> >> >>>
>> >> >>> On Wed, Nov 4, 2015 at 2:00 PM, NightStrike > >
>> >> >>> wrote:
>> >>
>> >> Use AM_CONDITIONAL, not DEFINE_UNQUOTED
>> >>
>> >> On Wed, Nov 4, 2015 at 1:56 PM, Martell Malone
>> >>
>> >> wrote:
>> >> > Be warned I am no autotools expert.
>> >> > A review would be very helpful. :)
>> >> >
>> >> > CC+ alexey for help on that :)
>> >> >
>> >> >
>> >> >
>> >> >
>> >> >
>> --
>> >> >
>> >> > ___
>> >> > Mingw-w64-public mailing list
>> >> > Mingw-w64-public@lists.sourceforge.net
>> >> > https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
>> >> >
>> >>
>> >>
>> >>
>> >>
>> --
>> >> ___
>> >> Mingw-w64-public mailing list
>> >> Mingw-w64-public@lists.sourceforge.net
>> >> https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
>> >> >>>
>> >> >>>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> --
>> >> >> Presto, an open source distributed SQL query engine for big data,
>> >> >> initially
>> >> >> developed by Facebook, enables you to easily query your data on
>> Hadoop
>> >> >> in a
>> >> >> more interactive manner. Teradata is also now providing full
>> enterprise
>> >> >> support for Presto. Download a free open source copy now.
>> >> >> http://pubads.g.doubleclick.net/gampad/clk?id=250295911=/4140
>> >> >>