Re: [Patch] Supported SWIG version(Re: Supported SWIG version on swig-py3)

On Fri, Nov 8, 2019 at 9:59 AM Yasuhito FUTATSUKI <futatuki_at_poem.co.jp>
wrote:

> Hi, I updated the patch to restrict supported SWIG version
> posted before, <4ea8a16b-f04b-ab8c-8794-60a244294d6f_at_poem.co.jp>.
>
...

> Please review, especially I don't have any confidence in description
> in INSTALL file.

Hello Yasuhito,

I have reviewed your patch and made some edits, which I will describe
in detail here...

I hope it is helpful. :-)

I think that the SWIG bindings should be considered plural for
linguistic purposes (i.e., bindings, rather than binding).

I am not sure whether a phrase like "SWIG version 2.0.0 and later"
should be considered singular or plural. Is it plural because we are
potentially discussing multiple SWIG versions? Or is it singular
because the subject is "SWIG version 2.0.0" with any later version(s)
being incidental? I made some edits based on what "sounds" correct to
me. This results in some cases being singular and others plural.
Please let me know if this does not seem correct.

With the above points in mind, I suggest the following changes. To
save you some work, I have updated your patch with these changes
(attached):

In build/ac-macros/swig.m4:

AC_MSG_WARN([Subversion Python bindings for Python 3 requires 3.0.10 <=
SWIG < 4.0.0])
                                                     ^^^^^^^^
                                                     (require)

AC_MSG_WARN([Subversion Python bindings for Python 2 requires 1.3.24 <=
SWIG < 4.0.0])
                                                     ^^^^^^^^
                                                     (require)

In subversion/bindings/swig/INSTALL:

This bullet point:

+ * SWIG installation is optional if you are using distribution tarball,
+ because it contains source fils generated by SWIG. If you are using
+ a source working copy checked out from Subversion source repository,
+ or you want to regenerate SWIG language bindings source files from
+ SWIG source files for some reason, you need suitable version of
+ SWIG installation.

may I suggest:

    * SWIG installation is optional if you are using a Subversion
      distribution tarball, because it contains the source files generated
      by SWIG. If you are using a working copy checked out from
      Subversion's source repository, or you want to regenerate the SWIG
      language bindings source files for some reason, you need a suitable
      version of SWIG installed.

This bullet point:

+ * Currently supported SWIG version is 2.0.0 and later, however,
+ - SWIG 1.3.24 and later in 1.3.x may work, but we didn't test on
+ latest our source code.
+ - Python bindings for Python 2 doesn't supported SWIG 4.0.0 and
later.
+ - Python 3 support for Python bindings requires SWIG 3.0.10 and
later,
+ but SWIG 4.0.0 and later is not supported (yet).
+ - Ruby bindings doesn't support SWIG 3.0.8.
+ - Make sure language specific supported version of SWIG itself, e.g.
+ - Perl 5.16 and later requires SWIG 2.0.8 and later.
+ - SWIG 3.0.9 has some trouble with Python support.
+ (see
https://sourceforge.net/p/swig/news/2016/06/swig-3010-released/)

may I suggest:

    * Currently, SWIG versions 2.0.0 and later are supported, with the
      following notes:
      - SWIG 1.3.24 and later 1.3.x may work, but we do not test these
        versions on our latest source code.
      - For Python 2 bindings, SWIG 4.0.0 or later is not supported.
      - For Python 3 bindings, SWIG 3.0.10 or later is required, but
        SWIG 4.0.0 and later is not supported (yet).
      - Note that SWIG 3.0.9 has some trouble with Python support.
        (See https://sourceforge.net/p/swig/news/2016/06/swig-3010-released/
)
      - For Perl 5.16 and later, SWIG 2.0.8 or later is required.
      - For Ruby bindings, SWIG 3.0.8 is not supported.

This was not part of your patch, but:

> * Perhaps your distribution packages a suitable version - if it does
> install it, and skip to the last bullet point in this section.

I suggest changing the punctuation:

    * Perhaps your distribution packages a suitable version. If it does,
      install it and skip to the last bullet point in this section.

This bullet point:

+ * In the swig-x.y.z, directory, run ./configure (where x.y.z is
+ SWIG version, e.g. 3.0.12).

I suggest adding "the":

    * In the swig-x.y.z, directory, run ./configure (where x.y.z is
      the SWIG version, e.g. 3.0.12).

This bullet point:

     * To verify you have SWIG installed correctly, run "swig -version"
+ from the command line. SWIG should report that it is suitable version
+ mentioned above.

I suggest adding "one of the":

    * To verify you have SWIG installed correctly, run "swig -version"
      from the command line. SWIG should report that it is one of the
      suitable versions mentioned above.

I hope this is helpful. Updated patch attached...

Kind regards,
Nathan