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...@poem.co.jp> on 2019/11/08 14:58:56 UTC

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

Hi, I updated the patch to restrict supported SWIG version
posted before, <4e...@poem.co.jp>.

In build system:
* Make symbolic link for *.so files in build tree on the copy-swig-py
  target, for the check-swig-py target (for SWIG >= 4.0.0). this symbolic
  links are cleared on the clean-swig-py target
* Stop build with Python 3 + SWIG < 3.0.10 (or SWIG >= 4.0.0, yet)
* Stop build with Python 2 + SWIG >= 4.0.0

In subversion/bindings/swig/INSTALL:
* Add description that SWIG is optional for using distribution tarball.
* Update description of supported SWIG version.

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

Thanks,
-- 
Yasuhito FUTATSUKI <fu...@poem.co.jp>

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

Posted by Branko Čibej <br...@apache.org>.
On 12.11.2019 16:19, Nathan Hartman wrote:
> On Tue, Nov 12, 2019 at 7:26 AM Branko Čibej <brane@apache.org
> <ma...@apache.org>> wrote:
>
>     On 12.11.2019 13:08, Julian Foad wrote:
>     > Daniel Shahaf wrote:
>     >> There should be a comma after "e.g.".
>     >
>     > In my experience (in England)[,] a comma can be used[,] but is not
>     > often used.  I appreciate [that] we like to get grammar
>     > {right|"Right"}, but ... :-)
>
>
>     In MY experience, my English teacher (i.e., someone from England, not
>     just someone who taught me English, in a British school) hammered the
>     comma after 'e.g.' and 'i.e.' into my brainstem in no uncertain
>     terms. :)
>
>
> Thank you for that. :-)
>
> What about my "SWIG version 2.0.0 and later" singular / plural question
> and mix of treating it as singular in some places and plural in others?


In the specific example you quote, I believe the singular is correct
since you only use one version at a time, but have a choice of which
version that may be.


> may I suggest:
>
>     * SWIG installation is optional if you are using a Subversion
>       distribution tarball, because it contains the source files generated
>       by SWIG.

Rule number one: active voice, not passive voice.

    You do not have to install SWIG if you are using a Subversion
    distribution tarball, etc.


> may I suggest:
>
>     * Currently, SWIG versions 2.0.0 and later are supported, with the
>       following notes:

See rule number one.

    We currently support SWIG version 2.0.0 and later, etc.


Or:

    We currently support SWIG versions from 2.0.0 onwards, etc.


> I suggest adding "one of the":
>
>     * To verify you have SWIG installed correctly, run "swig -version"
>       from the command line.

English relies on prepositions, not on verb conjugations (since has
none). Also see rule number one.

    To verify that you installed SWIG correctly, etc.



--  Brane


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

Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Nov 13, 2019 at 1:20 AM Yasuhito FUTATSUKI <fu...@poem.co.jp>
wrote:

> On 2019/11/13 14:16, Nathan Hartman wrote:
> > Yasuhito, if possible could you please commit your patch?
> > Then, I will work on the description in the INSTALL file and I'll
> > incorporate the good input from Daniel, Julian, and Brane... :-)
>
> Thanks a lot, for your reviews, helps, and advices.
>
> I've committed my patch, with update for messages in  build/ac-macros
> by Nathan's suggestion and URLs update for ViewVC and Trac in INSTALL
> file only, in r1869722.


Thank you!

I have followed up with r1869745, to improve the text in
subversion/bindings/swig/INSTALL.

If any mistakes are found, let me know.

Note: If the patch contributed by Jun Omae a little earlier is
committed, the text regarding SWIG 4.0.0+ in INSTALL will need to be
adjusted accordingly. I have not changed it because the patch has not
been committed yet. See the thread "[Patch] Support building with
SWIG 4 on Python 3.x" --
https://lists.apache.org/thread.html/49766e2f6b78b6a89cbb398526b202d248ea90aa2b7c7e73cb8b22c0@%3Cdev.subversion.apache.org%3E

Cheers,
Nathan

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

Posted by Yasuhito FUTATSUKI <fu...@poem.co.jp>.
On 2019/11/13 14:16, Nathan Hartman wrote:
> On Tue, Nov 12, 2019 at 10:39 AM Julian Foad <ju...@apache.org> wrote:
> 
>> Nathan Hartman wrote:
>>> What about my "SWIG version 2.0.0 and later" singular / plural question
>>> and mix of treating it as singular in some places and plural in others?
>>
>> "SWIG versions 2 and later are ..."
>> "SWIG version 2 or later is ..."
>>
>> If there are no concerns about the substance of the patch, I suggest a
>> good course of action is to commit it ASAP, then just make your edits in
>> follow-up commits, and optionally mention in email that you did so.
>> Even when some of the language is a bit unclear, most of this sort of
>> thing doesn't need pre-commit review.
> 
> 
> I agree.
> 
> Yasuhito, if possible could you please commit your patch?
> Then, I will work on the description in the INSTALL file and I'll
> incorporate the good input from Daniel, Julian, and Brane... :-)

Thanks a lot, for your reviews, helps, and advices.

I've committed my patch, with update for messages in  build/ac-macros
by Nathan's suggestion and URLs update for ViewVC and Trac in INSTALL
file only, in r1869722.

Thanks,
-- 
Yasuhito FUTATSUKI <fu...@poem.co.jp>

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

Posted by Nathan Hartman <ha...@gmail.com>.
On Tue, Nov 12, 2019 at 10:39 AM Julian Foad <ju...@apache.org> wrote:

> Nathan Hartman wrote:
> > What about my "SWIG version 2.0.0 and later" singular / plural question
> > and mix of treating it as singular in some places and plural in others?
>
> "SWIG versions 2 and later are ..."
> "SWIG version 2 or later is ..."
>
> If there are no concerns about the substance of the patch, I suggest a
> good course of action is to commit it ASAP, then just make your edits in
> follow-up commits, and optionally mention in email that you did so.
> Even when some of the language is a bit unclear, most of this sort of
> thing doesn't need pre-commit review.


I agree.

Yasuhito, if possible could you please commit your patch?
Then, I will work on the description in the INSTALL file and I'll
incorporate the good input from Daniel, Julian, and Brane... :-)

Thanks!

Nathan

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

Posted by Julian Foad <ju...@apache.org>.
Nathan Hartman wrote:
> What about my "SWIG version 2.0.0 and later" singular / plural question
> and mix of treating it as singular in some places and plural in others?

"SWIG versions 2 and later are ..."
"SWIG version 2 or later is ..."

If there are no concerns about the substance of the patch, I suggest a 
good course of action is to commit it ASAP, then just make your edits in 
follow-up commits, and optionally mention in email that you did so. 
Even when some of the language is a bit unclear, most of this sort of 
thing doesn't need pre-commit review.

- Julian

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

Posted by Nathan Hartman <ha...@gmail.com>.
On Tue, Nov 12, 2019 at 7:26 AM Branko Čibej <br...@apache.org> wrote:

> On 12.11.2019 13:08, Julian Foad wrote:
> > Daniel Shahaf wrote:
> >> There should be a comma after "e.g.".
> >
> > In my experience (in England)[,] a comma can be used[,] but is not
> > often used.  I appreciate [that] we like to get grammar
> > {right|"Right"}, but ... :-)
>
>
> In MY experience, my English teacher (i.e., someone from England, not
> just someone who taught me English, in a British school) hammered the
> comma after 'e.g.' and 'i.e.' into my brainstem in no uncertain terms. :)
>

Thank you for that. :-)

What about my "SWIG version 2.0.0 and later" singular / plural question
and mix of treating it as singular in some places and plural in others?

Nathan

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

Posted by Branko Čibej <br...@apache.org>.
On 12.11.2019 13:08, Julian Foad wrote:
> Daniel Shahaf wrote:
>> There should be a comma after "e.g.".
>
> In my experience (in England)[,] a comma can be used[,] but is not
> often used.  I appreciate [that] we like to get grammar
> {right|"Right"}, but ... :-)


In MY experience, my English teacher (i.e., someone from England, not
just someone who taught me English, in a British school) hammered the
comma after 'e.g.' and 'i.e.' into my brainstem in no uncertain terms. :)

-- Brane


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

Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
> There should be a comma after "e.g.".

In my experience (in England)[,] a comma can be used[,] but is not often 
used.  I appreciate [that] we like to get grammar {right|"Right"}, but 
... :-)

- Julian

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

Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Nathan Hartman wrote on Mon, Nov 11, 2019 at 21:37:44 -0500:
>     * In the swig-x.y.z, directory, run ./configure (where x.y.z is
>       the SWIG version, e.g. 3.0.12).

There should be a comma after "e.g.".

Cheers,

Daniel

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

Posted by Nathan Hartman <ha...@gmail.com>.
On Fri, Nov 8, 2019 at 9:59 AM Yasuhito FUTATSUKI <fu...@poem.co.jp>
wrote:

> Hi, I updated the patch to restrict supported SWIG version
> posted before, <4e...@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