Re: [PATCH] Update swig INSTALL document for Python 2 bindings

Thank you for the review.

On 2020/05/14 22:27, Daniel Shahaf wrote:
> Yasuhito FUTATSUKI wrote on Thu, 14 May 2020 19:51 +0900:
>> Hi,
>>
>> I overlooked that 'make clean-swig-py' doesn't remove SWIG generated
>> source files if build-output.mk is generated for release mode. It is
>> need to clean them to rebuild source files for Python 2 bindings.
>>
>> So I want to update subversion/bindings/swig/INSTALL so that users don't
>> fall into this pitfall. Could anyone please make this better?
>>
>> [[[
>> * subversion/bindings/swig/INSTALL
>> (Step 2: Build and Install Subversion.): Add description for optional
>> process needed to build Python 2 bindings.
>>
>> Index: subversion/bindings/swig/INSTALL
>> ===================================================================
>> --- subversion/bindings/swig/INSTALL (revision 1877407)
>> +++ subversion/bindings/swig/INSTALL (working copy)
>> @@ -141,6 +141,10 @@
>>
>> See Subversion's own INSTALL file for details.
>>
>> + If you are using Subversion distribution tarball and want to build
>> + Python bindings for Python 2, you should run 'sh autogen.sh' before run
>> + ./configure script to rebuild build environment as non-release mode.
>
> I suggest to add this information to trunk/INSTALL, item (12), instead.

Ah, that's good. I felt somewhat uncomfortable placing this text here.
 
> A few tweaks to the text:
>
> + If you are using a Subversion distribution tarball and want to build
> + the Python bindings for Python 2, you should run 'sh autogen.sh' before running
> + the ./configure script, to rebuild the build environment in non-release mode.
>
> Personally, I prefer to put descriptions before instructions (i.e.,
> "you should rebuild the build environment ... by running 'sh
> autogen.sh' ..."), but this is by and large a question of style.

Agree. Actually I want to know why to do so at first in such case.
 
>> Make sure that Subversion's ./configure script sees your installed SWIG!
>> It tries to detect SWIG near the very end of its output.
>
> Preëxisting text, I know, but still: how about changing this to
> recommend, say, «grep '^SWIG' config.log», or some other appropriate
> command?

Then, how about this?
[[[
* INSTALL (I.C.13): Add Note that non-release mode is requires to build
 SWIG Python 2 binding.

* subversion/bindings/swig/INSTALL
  (BUILDING SWIG BINDINGS FOR SVN ON UNIX, Step 2): Add description how to
  confirm that the ./configure could find SWIG path correctly.

Index: INSTALL
===================================================================
--- INSTALL (revision 1877936)
+++ INSTALL (working copy)
@@ -506,7 +506,13 @@
       reached end of life. All users are strongly encouraged to move
       to Python 3.
 
+ Note: If you are using a Subversion distribution tarball and want
+ to build the Python bindings for Python 2, you should rebuild
+ the build environment in non-release mode by running
+ 'sh autogen.sh' before running the ./configure script; see
+ section II.B for more about autogen.sh.
 
+
       13. Perl 5.8 or newer (Windows only) (OPTIONAL)
 
       To build Subversion under any of the MS Windows platforms, you
Index: subversion/bindings/swig/INSTALL
===================================================================
--- subversion/bindings/swig/INSTALL (revision 1877936)
+++ subversion/bindings/swig/INSTALL (working copy)
@@ -143,6 +143,7 @@
 
   Make sure that Subversion's ./configure script sees your installed SWIG!
   It tries to detect SWIG near the very end of its output.
+ You can find it by running 'grep "^SWIG=" config.log'.
 
   Also make sure that the configure script sees the paths to the perl and/or
   python executable you used to configure SWIG as above. If it does not then
]]]

Thanks,

-- 
Yasuhito FUTATSUKI <futatuki_at_poem.co.jp>/<futatuki_at_yf.bsdclub.org>