You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org> on 2020/12/01 16:34:37 UTC
[Patch] Introduce release mode configure script (Re: [Patch] Update
the INSTALL file for SWIG bindings)
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.
Thanks,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: [Patch] Introduce release mode configure script (Re: [Patch]
Update the INSTALL file for SWIG bindings)
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/12/04 6:56, Yasuhito FUTATSUKI wrote:
> On 2020/12/04 6:40, Yasuhito FUTATSUKI wrote:
>> Many thanks for the reviw. Then I've amended the patch.
>> (attached file, introduce-release-mode-configure-patch-v2.txt)
>
> I'm very sorry the patch attached previous mail is a bit old.
> It contains an extra tweak on style in release mode block of
> SVN_DETERMINE_SWIG_OPTS in build/ac-macros/swig.m4.
>
> Inline diff was not corresponded with the patch.
>
> The correct patch is the one attached in this message.
>
> Thanks,
I've just committed the patch in r1884610, to go ahead.
Thanks,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: [Patch] Introduce release mode configure script (Re: [Patch]
Update the INSTALL file for SWIG bindings)
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/12/04 6:40, Yasuhito FUTATSUKI wrote:
> Many thanks for the reviw. Then I've amended the patch.
> (attached file, introduce-release-mode-configure-patch-v2.txt)
I'm very sorry the patch attached previous mail is a bit old.
It contains an extra tweak on style in release mode block of
SVN_DETERMINE_SWIG_OPTS in build/ac-macros/swig.m4.
Inline diff was not corresponded with the patch.
The correct patch is the one attached in this message.
Thanks,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: [Patch] Introduce release mode configure script (Re: [Patch]
Update the INSTALL file for SWIG bindings)
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/12/02 4:24, Branko Čibej wrote:
> 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.
Many thanks for the reviw. Then I've amended the patch.
(attached file, introduce-release-mode-configure-patch-v2.txt)
In addition to the items pointed out, I also added two modification:
* Clean up aclocal.m4 on local-extraclean target in Makefile.in.
* Move the task to generate aclocal.m4 earlier in autogen.sh, because
aclocal.m4 file is also (at least) used by autoheader.
Also, I make diffs for release mode block and non-release mode block
of SVN_DETERMINE_SWIG_OPTS from corresponding block before this patch
in build/ac-macros/swig.m4, to show how they are changed.
Non release mode:
[[[
diff -ubB ./SVN_DETERMINE_SWIG_OPTS_original_block.txt ./SVN_DETERMINE_SWIG_OPTS_non_release_block.txt
--- ./SVN_DETERMINE_SWIG_OPTS_original_block.txt 2020-12-04 01:55:39.289774000 +0900
+++ ./SVN_DETERMINE_SWIG_OPTS_non_release_block.txt 2020-12-04 06:21:30.416610000 +0900
@@ -1,8 +1,16 @@
+ # not in release mode
SWIG_PY_COMPILE="none"
SWIG_PY_LINK="none"
SWIG_PY_OPTS="none"
SWIG_PY_ERRMSG="check config.log for details"
- if test "$SWIG_PY_PYTHON" != "none"; then
+ if test "$SWIG_PY_PYTHON" = "none"; then
+ SWIG_PY_ERRMSG="You specfied not to build Python bindings or \
+suitable Python interpreter is not found."
+ else
+ if test "$SWIG" = "none"; then
+ AC_MSG_WARN([You specified to build SWIG Python bindings, but SWIG is not found.])
+ SWIG_PY_ERRMSG="SWIG is need to build SWIG Python bindings, but it is not found."
+ else
AC_MSG_NOTICE([Configuring python swig binding])
AC_CACHE_CHECK([for Python includes], [ac_cv_python_includes],[
@@ -49,9 +57,6 @@
])
SWIG_PY_LIBS="`SVN_REMOVE_STANDARD_LIB_DIRS($ac_cv_python_libs)`"
- if test "$SWIG" = "none"; then
- SWIG_PY_ERRMSG=""
- else
# Look more closely at the SWIG and Python versions to
# determine SWIG_PY_OPTS. We can skip this if we already
# have the SWIG-generated files.
@@ -93,7 +97,14 @@
fi
SWIG_PL_ERRMSG="check config.log for details"
- if test "$SWIG_PL_PERL" != "none"; then
+ if test "$SWIG_PL_PERL" = "none"; then
+ SWIG_PL_ERRMSG="You specfied not to build Perl bindings or \
+suitable Perl interpreter is not found."
+ else
+ if test "$SWIG" = "none"; then
+ AC_MSG_WARN([You specified to build SWIG Perl bindings, but SWIG is not found.])
+ SWIG_PL_ERRMSG="SWIG is need to build SWIG Perl bindings, but it is not found."
+ else
AC_MSG_CHECKING([perl version])
dnl Note that the q() bit is there to avoid unbalanced brackets
dnl which m4 really doesn't like.
@@ -110,11 +121,19 @@
AC_MSG_WARN([perl bindings require perl 5.8.0 or newer.])
fi
fi
+ fi
SWIG_RB_COMPILE="none"
SWIG_RB_LINK="none"
SWIG_RB_ERRMSG="check config.log for details"
- if test "$SWIG_RB_RUBY" != "none"; then
+ if test "$SWIG_RB_RUBY" = "none"; then
+ SWIG_RB_ERRMSG="You specfied not to build Ruby bindings or \
+suitable Ruby interpreter is not found."
+ else
+ if test "$SWIG" = "none"; then
+ AC_MSG_WARN([You specified to build SWIG Ruby bindings, but SWIG is not found.])
+ SWIG_RB_ERRMSG="SWIG is need to build SWIG Ruby bindings, but it is not found."
+ else
if test x"$SWIG_VERSION" = x"3""00""008"; then
# Use a local variable to escape the '#' sign.
ruby_swig_issue_602='https://subversion.apache.org/docs/release-notes/1.11#ruby-swig-issue-602'
@@ -234,4 +253,5 @@
dnl SWIG Ruby bindings successfully configured, clear the error message
SWIG_RB_ERRMSG=""
+ fi
fi
]]]
Release mode:
[[[
diff -ubB ./SVN_DETERMINE_SWIG_OPTS_original_block.txt ./SVN_DETERMINE_SWIG_OPTS_release_block.txt
--- ./SVN_DETERMINE_SWIG_OPTS_original_block.txt 2020-12-04 01:55:39.289774000 +0900
+++ ./SVN_DETERMINE_SWIG_OPTS_release_block.txt 2020-12-04 06:21:56.977529000 +0900
@@ -1,8 +1,12 @@
+ # in release mode
SWIG_PY_COMPILE="none"
SWIG_PY_LINK="none"
SWIG_PY_OPTS="none"
SWIG_PY_ERRMSG="check config.log for details"
- if test "$SWIG_PY_PYTHON" != "none"; then
+ if test "$SWIG_PY_PYTHON" = "none"; then
+ SWIG_PY_ERRMSG="You specfied not to build Python bindings or \
+suitable Python interpreter is not found."
+ else
AC_MSG_NOTICE([Configuring python swig binding])
AC_CACHE_CHECK([for Python includes], [ac_cv_python_includes],[
@@ -49,51 +53,17 @@
])
SWIG_PY_LIBS="`SVN_REMOVE_STANDARD_LIB_DIRS($ac_cv_python_libs)`"
- if test "$SWIG" = "none"; then
SWIG_PY_ERRMSG=""
- else
- # Look more closely at the SWIG and Python versions to
- # determine SWIG_PY_OPTS. We can skip this if we already
- # have the SWIG-generated files.
- AC_CACHE_CHECK([for Python >= 3], [ac_cv_python_is_py3],[
- ac_cv_python_is_py3="no"
- $SWIG_PY_PYTHON -c 'import sys; sys.exit(0x3000000 > sys.hexversion)' && \
- ac_cv_python_is_py3="yes"
- ])
-
- if test "$ac_cv_python_is_py3" = "yes"; then
- if test "$SWIG_VERSION" -ge "300010"; then
- dnl SWIG Python bindings successfully configured, clear the error message dnl
- SWIG_PY_ERRMSG=""
- else
- SWIG_PY_ERRMSG="SWIG version is not suitable"
- AC_MSG_WARN([Subversion Python bindings for Python 3 require SWIG 3.0.10 or newer])
fi
- if test "$SWIG_VERSION" -lt "400000"; then
- SWIG_PY_OPTS="-python -py3 -nofastunpack -modern"
- else
- SWIG_PY_OPTS="-python -py3 -nofastunpack"
fi
- else
- if test "$SWIG_VERSION" -lt "400000"; then
- SWIG_PY_OPTS="-python -classic"
- dnl SWIG Python bindings successfully configured, clear the error message dnl
- SWIG_PY_ERRMSG=""
- else
- SWIG_PY_OPTS="-python -nofastunpack"
- SWIG_PY_ERRMSG="SWIG version is not suitable"
- AC_MSG_WARN([Subversion Python bindings for Python 2 require 1.3.24 <= SWIG < 4.0.0])
fi
fi
- fi
- fi
- fi
- fi
- fi
-
SWIG_PL_ERRMSG="check config.log for details"
- if test "$SWIG_PL_PERL" != "none"; then
+ if test "$SWIG_PL_PERL" = "none"; then
+ SWIG_PL_ERRMSG="You specfied not to build Perl bindings or \
+suitable Perl interpreter is not found."
+ else
AC_MSG_CHECKING([perl version])
dnl Note that the q() bit is there to avoid unbalanced brackets
dnl which m4 really doesn't like.
@@ -114,12 +84,10 @@
SWIG_RB_COMPILE="none"
SWIG_RB_LINK="none"
SWIG_RB_ERRMSG="check config.log for details"
- if test "$SWIG_RB_RUBY" != "none"; then
- if test x"$SWIG_VERSION" = x"3""00""008"; then
- # Use a local variable to escape the '#' sign.
- ruby_swig_issue_602='https://subversion.apache.org/docs/release-notes/1.11#ruby-swig-issue-602'
- AC_MSG_WARN([Ruby bindings are known not to support swig 3.0.8; see $ruby_swig_issue_602])
- fi
+ if test "$SWIG_RB_RUBY" = "none"; then
+ SWIG_RB_ERRMSG="You specfied not to build Ruby bindings or \
+suitable Ruby interpreter is not found."
+ else
rbconfig="$SWIG_RB_RUBY -rrbconfig -e "
for var_name in arch archdir CC LDSHARED DLEXT LIBS LIBRUBYARG \
]]]
Thanks,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: [Patch] Introduce release mode configure script (Re: [Patch]
Update the INSTALL file for SWIG bindings)
Posted by Branko Čibej <br...@apache.org>.
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.
[...]