Re: configure --without-swig warns when it shouldn't
From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Sun, 31 May 2020 03:14:28 +0000
Yasuhito FUTATSUKI wrote on Sat, 30 May 2020 19:24 +0900:
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
Just one question; see below.
Yasuhito FUTATSUKI wrote on Sat, 30 May 2020 19:24 +0900:
Here configure sets PYTHON=none…
> @@ -1291,16 +1291,95 @@
…and here configure warns when $PYTHON is set to «none». Would this
--- 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, DanielReceived on 2020-05-31 05:14:51 CEST |
This is an archived mail posted to the Subversion Dev mailing list.
This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.