Re: configure --without-swig warns when it shouldn't

Yasuhito FUTATSUKI wrote on Sat, 30 May 2020 19:24 +0900:
> On 2020/05/12 23:17, Branko Čibej wrote:
> > Can we just say that it was a mistake to look at the environment (e.g.,
> > PERL=none) instead of configure arguments (--without-swig-perl)? I agree
> > that adding the proposed flags to configure is the correct solution. We
> > could also add a warning for usage of PERL=none and RUBY=none, pointing
> > users to the new options instead.
>
> Then I make a patch as a starting point. Could anyone please refine
> or rewrite this?

Thanks for the patch!

I've tested it and confirm it behaves as expected:

    % ~/src/svn-config.nice ~/src/svn/t1 --without-apxs --without-swig-python
    % grep PYTHON Makefile | vipe
    PYTHON = python
    SWIG_PY_PYTHON = none
    %

Just one question; see below.

Yasuhito FUTATSUKI wrote on Sat, 30 May 2020 19:24 +0900:
> +++ configure.ac (working copy)
> @@ -1272,19 +1272,19 @@
>
> # Scripting and Bindings languages
>
> -# Python: Used for testsuite, and bindings
> +# Python: Used for testsuite
> AC_ARG_VAR([PYTHON], [Python interpreter command])
>
> PYTHON="`$abs_srcdir/build/find_python.sh`"
> if test -z "$PYTHON"; then
> - AC_MSG_WARN([Python 2.7 or later is required to run the testsuite])
> - AC_MSG_WARN([or to use the Subversion Python bindings])
> + AC_MSG_WARN([Python 2.7 or later is required to run the testsuite.])
> AC_MSG_WARN([])
> AC_MSG_WARN([If you have a suitable Python installed, but not on the])
> AC_MSG_WARN([PATH, set the environment variable PYTHON to the full path])
> AC_MSG_WARN([to the Python executable, and re-run configure])
> + PYTHON=none
> fi
> -AC_PATH_PROGS(PYTHON, "$PYTHON", none)
> +AC_SUBST(PYTHON)
>

Here configure sets PYTHON=none…

> @@ -1291,16 +1291,95 @@
> +# Python: as a target of SWIG Python bindings
> +AC_ARG_WITH(swig_python,
> +[AS_HELP_STRING([[--with-swig-python[=PATH|auto|no]|--without-swig-python]],
> + [specify path to SWIG bindings target Python [default=auto]])],
> +[],
> +[
> +if test "$PYTHON" = "no" -o "$PYTHON" = "none"; then
> + with_swig_python=no
> + AC_MSG_WARN([Disabling build of SWIG Python bindings by setting "none" to])
> + AC_MSG_WARN([PYTHON envirionment variable is deprecated.])
> + AC_MSG_WARN([])
> + AC_MSG_WARN([Please use --without-swig-python instead.])

…and here configure warns when $PYTHON is set to «none». Would this
error message be printed when the value «none» was set by configure,
above, rather than by the user?

---
Some suggested changes to the English:
Yasuhito FUTATSUKI wrote on Sat, 30 May 2020 19:24 +0900:
> configure: Add new option to specify path to swig bindings targets
> 
> Before this commit, we could not distinct Python processor for using
> as a part of build/test system and for a target of language bindings.
"distinct" is an adjective, not a verb, and "processor" as a term of
art has specific meanings which don't fit this context, so how about:
    Before this commit, it was not possible to use distinct Python
    installations for the build system and test suite on the one hand,
    and for building language bindings against on the other hand.
That's a little wordy because of the "and" in "the build system and test
suite".
I've changed the use of "target" because, in this context, I'd say the
"target" of, say, the SWIG Python bindings is the range of Python
versions they are intended to support, rather than any particular
Python installation.
> So we've introduced new variable "SWIG_PY_PYTHON" for a target of
> the SWIG Python bindings and configure option to specify it. 
> Also, for the symmetricalness we've introduced "SWIG_PL_PERL" and
> "SWIG_RB_RUBY" for the Perl and Ruby SWIG bindings, and options
> to specify them.
By convention, log messages should use the imperative mood to describe
the changes their patches make; thus, s/we've introduced/introduce/.
Some projects follow another convention and use the past tense (e.g.,
"Fixed bug #42").  In contrast, as written (in present perfect) it
could be understood as describing for the reader the historical context
of the patch, as opposed to describing the changes made by the patch.
Furthermore:
s/configure option/a configure option/
s/symmetricalness/symmetry/
> * configure.ac ():
>   - Use variables "SWIG_PL_PERL", "SWIG_PY_PYTHON", "SWIG_RB_RUBY" for
>     target of SWIG Perl, Python, Ruby bindings instead of "PERL",
>     "PYTHON", "RUBY".
>   - introduce --with-swig-perl, --with-swig-python, --with-swig-ruby
>     options for setting variable "SWIG_PL_PERL", "SWIG_PY_PYTHON",
>     "SWIG_RB_RUBY".
"Introduce" should be capitalized like "Use" is: each of those words
is at the start of a sentence.  There's another case of this later in
the log message.
> * Makefile.in, build/ac-macros/swig.m4 ():
>   Use variables "SWIG_PL_PERL", "SWIG_PY_PYTHON", "SWIG_RB_RUBY" for
>   target of SWIG Perl, Python, Ruby bindings instead of "PERL",
>   "PYTHON", "RUBY".
> 
> * subversion/bindings/swig/INSTALL
>   (BUILDING SWIG BINDINGS FOR SVN ON UNIX step 2):
>   - Describe how to specify the path to the target language interpreters
>     with new introduced options, instead of using environment variable.
"with new introduced options" lacks a definite article (the options are
already known to the reader).  Furthermore, "new introduced" is
a sequence of two adjectives with no intervening comma, which isn't
necessarily appropriate in this context.  All in all, I'd suggest
"with the new options" or "with the newly-introduced options".
"environment variables" should be plural.
>   - fix the name of glue libraries.
> +++ configure.ac	(working copy)
> @@ -1291,16 +1291,95 @@
>  # SVN_CHECK_JDK sets $JAVA_CLASSPATH
>  SVN_CHECK_JDK($JAVA_OLDEST_WORKING_VER)
>  
> -AC_PATH_PROG(PERL, perl, none)
> +AC_ARG_WITH(swig_perl,
> +[AS_HELP_STRING([[--with-swig-perl[=PATH|auto|no]|--without-swig-perl]],
> +                [specify path to SWIG bindings target Perl [default=auto]])],
How about s/Perl/Perl interpreter/, so it's clear which path should be
specified as the argument to this option?
Likewise for the other new options' help strings.
> +[],
> +[
> +if test "$PERL" = "no" -o "$PERL" = "none"; then 
> +  with_swig_perl=no
> +  AC_MSG_WARN([Disabling build of SWIG Perl bindings by setting "none" to])
> +  AC_MSG_WARN([PERL envirionment variable is deprecated.])
«by setting the PERL environment variable to "none"».  (Change the word
order, add missing definite article, and correct the spelling of "environment".)
s/build/building/ [or "Disabling the SWIG Perl bindings' build"]
> +  AC_MSG_WARN([])
> +  AC_MSG_WARN([Please use --without-swig-perl instead.])
Thanks again,
Daniel