On 01.12.2020 17:34, Yasuhito FUTATSUKI wrote:
> On 2020/11/17 11:01, Yasuhito FUTATSUKI wrote:
>> On 2020/11/17 2:16, Daniel Shahaf wrote:
>>> Yasuhito FUTATSUKI wrote on Sat, 14 Nov 2020 14:52 +0900:
>>>> +++ subversion/bindings/swig/INSTALL (working copy)
>>>> @@ -141,7 +141,15 @@
>>>> - Make sure that Subversion's ./configure script sees your installed SWIG!
>>>> + If you are using the distribution tarball and you want to use the language
>>>> + bindings C source files shipped with it, you might need to pass the
>>>> + --without-swig option to configure script to avoid detecting and checking
>>>> + SWIG on your system. A Makefile generated by configure will prevent
>>>> + building the language bindings if the configure script detect unsuitable
>>>> + version of SWIG.
>>> I don't dispute the accuracy of this paragraph, but I think this API
>>> isn't autotools-idiomatic.
>>>
>>> Generally, I'd expect --without-foo to short-circuit the probe for foo
>>> and assume foo isn't found; i.e., if my system has an unsuitable
>>> version of foo, I'd the default behaviour (given neither --with-foo nor
>>> --without-foo) and the behaviour given --without-foo to be identical:
>>> namely, behave as though foo isn't available (even if /usr/bin/foo
>>> exists and is perfectly suitable).
>>>
>>> In particular, if my system has an unsuitable version of swig,
>>> I wouldn't expect passing --without-swig to change configure's behaviour.
>> Probably what is wrong here is that the configure script accepts
>> --with-swig | --without-swig options and checks it in release mode.
>>
>> We never clean SWIG generated language bindings C source files on
>> clean-foo targets in release mode Makefile. extraclean-foo targets do it,
>> but they are only parts of the extraclean target which also removes all
>> release mode stuff. So users never use SWIG in release mode actually.
>>
>> That is, r1876662 is not correct.
> To fix it, I tweaked the configure script again.
>
> The patch attached introduce "release mode" for configure generation.
> With this patch, configure script generated by "autogen.sh --release"
> does not have --with-swig|--without-swig option and never check SWIG
> executable.
>
> Could anyone please review this?
>
> For backward compatibility to back port to 1.14.x, default for
> --with-swig-perl, --with-swig-python, --with-swig-ruby are "auto",
> i.e. search the target language, but I'd like to change them to "no" in
> trunk, because I think those are optional feature.
This makes sense, yes. More comments below.
> Distinguish configure scripts on release mode and non release mode.
> Although makefiles in the Subversion's release tarball do not support to
No "the", just "Subversion's", and "generating" instead of "to generate".
> genarate SWIG language bindings C source files using swig, the configure
> scripts shipped with release tarball had a option to specify how to find
... "shipped with THE release tarball", and "had AN option" ...
> SWIG executable, and checked it. To avoid this, we introduce "release mode"
> to the configure script and hiding an option and code to check a SWIG
... "and HIDE THE option and code" ...
> executable on it.
>
> * . (svn:ignore):
> Ignore aclocal.m4.
> * aclocal.m4 ():
> Renamed to aclocal.m4.in
> * aclocal.m4.in ():
> Renamed from aclocal.m4.in
You don't have to mention the renamed file twice in the log message. we
usually use the second form, mentioning the new name first, like this:
* aclocal.m4.in: Renamed fom aclocal.m4.
(Note that you wrote it was renamed from "aclocal.m4.in", heh. :)
> * autogen.sh ():
> Define SVN_RELEASE_MODE macro if --release is specfied from command line.
I think we should say explicitly that the macro definition is written to
aclocal.m4.
Also, "specified ON THE command line".
[...]
> (SVN_DETERMINE_SWIG_OPTS): New macro. Devided from SVN_FIND_SWIG.
"Divided", not "devided". Or better yet, "split from" or "extracted
from" or "derived from".
> - On non releasemode, warn if Perl/Python/Ruby interpreter is set but
"When not in release mode" ...
> SWIG is not found. Also it prevent build them on make swig-pl/make
"Also do not build them with 'make swig-pl'/" ...
> swig-py/make swig-rb in such case.
... "in this case."
> - Check swig version only on non release mode and it is needed
"Check swig version only when it is needed and when not in release mode."
> * configure.ac ():
> - Tweak help string for --with-swig-perl, --with-swig-python,
> --with-swig-ruby.
> - Warn if --with-swig-perl is not specified but variable 'PERL' is set.
> - Warn if --with-swig-ruby is not specified but variable 'Ruby' is set,
"Ruby" or "RUBY"?
> even the value is not 'no' nor 'none'.
> * subversion/bindings/swig/INSTALL (Step 2):
> Mension that configure and makefiles in release tarball don't
"Mention", "in THE release tarball"
> support generation of SWIG bindings C source files.
... "support generating" instead of "support generation of".
[...]
> +AC_DEFUN(SVN_DETERMINE_SWIG_OPTS,
> +[
I find the mixing of shell and m4 conditional blocks in this function
quite confusing, it's hard to see if it's done right ... why not extract
the common parts into separate helper macros then you can have something
like this:
AC_DEFUN(SVN_DETERMINE_SWIG_OPTS,
[
m4_ifndef([SVN_RELEASE_MODE],
[
# use one set of macros
],
[
# use the other set of macros
])
])
The SVN_CHECK_SWIG macro is more readable. Even if this causes some code
duplication between the m4 conditional blocks, I think it's more
important that the code is readable. m4 code is hard enough to maintain
already. :)
[...]
> AC_ARG_WITH(swig_perl,
> [AS_HELP_STRING([[--with-swig-perl[=PATH|auto|no]|--without-swig-perl]],
> - [specify path to SWIG bindings target Perl interpreter [default=auto]])],
> + [Specify path to SWIG bindings target Perl interpreter
> + [default=auto]. If the option value is 'auto' or it is not
> + specfied with this option, it searches a 'perl' program.]
... "or it is not specified, search for the 'perl' program."
I suggest to make similar wording changes for Python and Ruby, too.
[...]
Received on 2020-12-01 20:24:54 CET