Re: Python3 work [was: The run up to Subversion 1.13.0]
On 2019/09/28 22:36, Branko Čibej wrote: On 28.09.2019 11:20, Yasuhito FUTATSUKI wrote: On 2019/09/24 16:34, Branko Čibej wrote: On 23.09.2019 22:48, Johan Corveleyn wrote: On Mon, Sep 23, 2019 at 1:53 AM Yasuhito FUTATSUKI wrote: On 2019/09/23 6:16, Johan Corveleyn wrote: Building with Python 3.7.4 still fails with the same error though (no problem, I know your patch wasn't addressing that, just mentioning it here for completeness). Consequently I haven't been able to run the swig-python tests with python 3.7 yet. [[[ c:\python37\include\pytime.h(123): error C4115: 'timeval': named type definition in parentheses [C:\research\svn\dev\swig-py3\build\win32\vcnet-vcproj\libsvn_swig_py.vcxproj ]]] Here is a patch not to treat C4115 as error, globally. If this makes it possible to build with Python 3.7 on Windows, then next step can be to limit to apply this relaxation of compile option to files which contains "#include " (most of them are generated by swig...). Yes, that makes the build succeed, thanks. I can't comment on whether or not it's good to change this error into a warning overall, or just for a limited set of files. I'll leave that discussion to others :-). It should not be an error because the source is valid C, regardless of what Microsoft's compiler thinks about it. :) However it is ourselves to decide treat it as an error, on r876281. https://svn.apache.org/viewvc?view=revision=876281 Unfortunately I can't find any reason or discussion about this decision, yet. That's why I try to treat it carefully. (Of course there might be no serious reason, though) The most likely reason is that /our/ code should not contain a use of 'struct foo*' without at least a forward declaration or a typedef. That's a coding style decision that we shouldn't enforce on generated code (but it should be a warning, not an error). If you can turn off that compiler option only for the files generated by Swig, that would be great, but just turning it into a warning is good enough, IMO. I agree your opinion. I also think a warning is enough for this purpose. And, I found that C4055 is also once treated as an error on r876281, but not to be treated as error again on 876286 for generated swig python code, just same reason :) (Though C4055 seems obsolete on Visual Studio 2017 and later) https://svn.apache.org/viewvc?view=revision=876286 (Daniel, thank you for your suggestion about revision number offset) Cheers, -- Yasuhito FUTATSUKI
Re: svn commit: r1867653 - in /subversion/branches/swig-py3/subversion/bindings/swig/python/tests: trac/versioncontrol/tests/svn_fs.py utils.py
On 2019/09/28 22:42, Branko Čibej wrote:
On 28.09.2019 15:39, Branko Čibej wrote:
On 28.09.2019 08:59, futat...@apache.org wrote:
Author: futatuki
Date: Sat Sep 28 06:59:54 2019
New Revision: 1867653
URL: http://svn.apache.org/viewvc?rev=1867653=rev
Log:
On branch swig-py3: fix test for swig-py on Python 3 on Windows
[ in subversion/bindings/swig/python/tests/]
* trac/versioncontrol/tests/svn_fs.py (REPOS_PATH, REPOS_URL),
On Python 3, pass a str object as argument to urllib.request.pathname2url()
instead of a bytes.
* util.py (file_uri_for_path):
On Python 3, pass a str object as argument to urllib.request.pathname2url()
instead of a bytes even if the argment `path' is a bytes object.
Reported by: jcorvel
==
--- subversion/branches/swig-py3/subversion/bindings/swig/python/tests/utils.py
(original)
+++ subversion/branches/swig-py3/subversion/bindings/swig/python/tests/utils.py
Sat Sep 28 06:59:54 2019
@@ -79,7 +79,10 @@ class Temper(object):
def file_uri_for_path(path):
"""Return the file: URI corresponding to the given path."""
- uri_path = pathname2url(path).encode('UTF-8')
+ if isinstance(path, str):
+uri_path = pathname2url(path).encode('UTF-8')
+ else:
+uri_path = pathname2url(path.decode('UTF-8')).encode('UTF-8')
I'd write this differently:
First, I'd not send the message to soon (facepalm).
Then, I'd write:
if isinstance(path, str):
path = path.decode('UTF-8')
uri_path = pathname2url(path).encode('UTF-8')
This way, there's only on call to pathname2url and someone who changes
the code in future doesn't have to remember to follow both branches of
the conditional.
I see. It's reasonable.
Thanks,
--
Yasuhito FUTATSUKI
Re: Python3 work [was: The run up to Subversion 1.13.0]
Yasuhito FUTATSUKI wrote on Sat, 28 Sep 2019 09:20 +00:00: > https://svn.apache.org/viewvc?view=revision=876281 > > Unfortunately I can't find any reason or discussion about this decision, yet. Are you aware of the "+ 840074" revision number offset that needs to be applied to old revision numbers (those < 5) in our history? See https://svn.apache.org/repos/asf/subversion/README (sic) for details. In this case, it means that where r876281 (= r36207) says "r36160", "r876234" should be substituted: https://svn.apache.org/r876234 However, that patch doesn't seem related to the compiler diagnostic at hand. Bert, do you remember the rationale behind including C4115 in r876281? Cheers, Daniel
Re: svn commit: r1867653 - in /subversion/branches/swig-py3/subversion/bindings/swig/python/tests: trac/versioncontrol/tests/svn_fs.py utils.py
On 28.09.2019 15:39, Branko Čibej wrote:
> On 28.09.2019 08:59, futat...@apache.org wrote:
>> Author: futatuki
>> Date: Sat Sep 28 06:59:54 2019
>> New Revision: 1867653
>>
>> URL: http://svn.apache.org/viewvc?rev=1867653=rev
>> Log:
>> On branch swig-py3: fix test for swig-py on Python 3 on Windows
>>
>> [ in subversion/bindings/swig/python/tests/]
>> * trac/versioncontrol/tests/svn_fs.py (REPOS_PATH, REPOS_URL),
>>On Python 3, pass a str object as argument to
>> urllib.request.pathname2url()
>>instead of a bytes.
>> * util.py (file_uri_for_path):
>>On Python 3, pass a str object as argument to
>> urllib.request.pathname2url()
>>instead of a bytes even if the argment `path' is a bytes object.
>>
>> Reported by: jcorvel
>> ==
>> ---
>> subversion/branches/swig-py3/subversion/bindings/swig/python/tests/utils.py
>> (original)
>> +++
>> subversion/branches/swig-py3/subversion/bindings/swig/python/tests/utils.py
>> Sat Sep 28 06:59:54 2019
>> @@ -79,7 +79,10 @@ class Temper(object):
>>
>> def file_uri_for_path(path):
>>"""Return the file: URI corresponding to the given path."""
>> - uri_path = pathname2url(path).encode('UTF-8')
>> + if isinstance(path, str):
>> +uri_path = pathname2url(path).encode('UTF-8')
>> + else:
>> +uri_path = pathname2url(path.decode('UTF-8')).encode('UTF-8')
> I'd write this differently:
First, I'd not send the message to soon (facepalm).
Then, I'd write:
if isinstance(path, str):
path = path.decode('UTF-8')
uri_path = pathname2url(path).encode('UTF-8')
This way, there's only on call to pathname2url and someone who changes
the code in future doesn't have to remember to follow both branches of
the conditional.
-- Brane
Re: svn commit: r1867653 - in /subversion/branches/swig-py3/subversion/bindings/swig/python/tests: trac/versioncontrol/tests/svn_fs.py utils.py
On 28.09.2019 08:59, futat...@apache.org wrote:
> Author: futatuki
> Date: Sat Sep 28 06:59:54 2019
> New Revision: 1867653
>
> URL: http://svn.apache.org/viewvc?rev=1867653=rev
> Log:
> On branch swig-py3: fix test for swig-py on Python 3 on Windows
>
> [ in subversion/bindings/swig/python/tests/]
> * trac/versioncontrol/tests/svn_fs.py (REPOS_PATH, REPOS_URL),
>On Python 3, pass a str object as argument to urllib.request.pathname2url()
>instead of a bytes.
> * util.py (file_uri_for_path):
>On Python 3, pass a str object as argument to urllib.request.pathname2url()
>instead of a bytes even if the argment `path' is a bytes object.
>
> Reported by: jcorvel
> ==
> ---
> subversion/branches/swig-py3/subversion/bindings/swig/python/tests/utils.py
> (original)
> +++
> subversion/branches/swig-py3/subversion/bindings/swig/python/tests/utils.py
> Sat Sep 28 06:59:54 2019
> @@ -79,7 +79,10 @@ class Temper(object):
>
> def file_uri_for_path(path):
>"""Return the file: URI corresponding to the given path."""
> - uri_path = pathname2url(path).encode('UTF-8')
> + if isinstance(path, str):
> +uri_path = pathname2url(path).encode('UTF-8')
> + else:
> +uri_path = pathname2url(path.decode('UTF-8')).encode('UTF-8')
I'd write this differently:
Re: Python3 work [was: The run up to Subversion 1.13.0]
On 28.09.2019 11:20, Yasuhito FUTATSUKI wrote: > On 2019/09/24 16:34, Branko Čibej wrote: >> On 23.09.2019 22:48, Johan Corveleyn wrote: >>> On Mon, Sep 23, 2019 at 1:53 AM Yasuhito FUTATSUKI >>> wrote: On 2019/09/23 6:16, Johan Corveleyn wrote: > > Building with Python 3.7.4 still fails with the same error though (no > problem, I know your patch wasn't addressing that, just mentioning it > here for completeness). Consequently I haven't been able to run the > swig-python tests with python 3.7 yet. > > [[[ > c:\python37\include\pytime.h(123): error C4115: 'timeval': named type > definition in parentheses > [C:\research\svn\dev\swig-py3\build\win32\vcnet-vcproj\libsvn_swig_py.vcxproj > > ]]] Here is a patch not to treat C4115 as error, globally. If this makes it possible to build with Python 3.7 on Windows, then next step can be to limit to apply this relaxation of compile option to files which contains "#include " (most of them are generated by swig...). >>> Yes, that makes the build succeed, thanks. I can't comment on whether >>> or not it's good to change this error into a warning overall, or just >>> for a limited set of files. I'll leave that discussion to others :-). >> >> It should not be an error because the source is valid C, regardless of >> what Microsoft's compiler thinks about it. :) > > However it is ourselves to decide treat it as an error, on r876281. > > https://svn.apache.org/viewvc?view=revision=876281 > > Unfortunately I can't find any reason or discussion about this > decision, yet. > That's why I try to treat it carefully. (Of course there might be no > serious reason, though) The most likely reason is that /our/ code should not contain a use of 'struct foo*' without at least a forward declaration or a typedef. That's a coding style decision that we shouldn't enforce on generated code (but it should be a warning, not an error). If you can turn off that compiler option only for the files generated by Swig, that would be great, but just turning it into a warning is good enough, IMO. -- Brane
Re: Python3 work [was: The run up to Subversion 1.13.0]
On 2019/09/24 16:34, Branko Čibej wrote: On 23.09.2019 22:48, Johan Corveleyn wrote: On Mon, Sep 23, 2019 at 1:53 AM Yasuhito FUTATSUKI wrote: On 2019/09/23 6:16, Johan Corveleyn wrote: Building with Python 3.7.4 still fails with the same error though (no problem, I know your patch wasn't addressing that, just mentioning it here for completeness). Consequently I haven't been able to run the swig-python tests with python 3.7 yet. [[[ c:\python37\include\pytime.h(123): error C4115: 'timeval': named type definition in parentheses [C:\research\svn\dev\swig-py3\build\win32\vcnet-vcproj\libsvn_swig_py.vcxproj ]]] Here is a patch not to treat C4115 as error, globally. If this makes it possible to build with Python 3.7 on Windows, then next step can be to limit to apply this relaxation of compile option to files which contains "#include " (most of them are generated by swig...). Yes, that makes the build succeed, thanks. I can't comment on whether or not it's good to change this error into a warning overall, or just for a limited set of files. I'll leave that discussion to others :-). It should not be an error because the source is valid C, regardless of what Microsoft's compiler thinks about it. :) However it is ourselves to decide treat it as an error, on r876281. https://svn.apache.org/viewvc?view=revision=876281 Unfortunately I can't find any reason or discussion about this decision, yet. That's why I try to treat it carefully. (Of course there might be no serious reason, though) -- Yasuhito FUTATSUKI
Re: Python3 work [was: The run up to Subversion 1.13.0]
On 2019/09/28 5:51, Johan Corveleyn wrote: Great! That fixes this particular problem, and the test suite now runs successfully ... almost. There seems to be one more problem: [[[ Testing Release configuration on local repository. -- Running Swig Python tests -- ..C:\Python37\lib\subprocess.py:858: ResourceWarning: subprocess 8548 is still running ResourceWarning, source=self) ResourceWarning: Enable tracemalloc to get the object allocation traceback C:\Python37\lib\unittest\case.py:628: ResourceWarning: unclosed file <_io.BufferedReader name=3> testMethod() ResourceWarning: Enable tracemalloc to get the object allocation traceback .C:\Python37\lib\unittest\case.py:628: ResourceWarning: unclosed file <_io.BufferedReader name='R:\\temp\\tmpryeb61g1'> testMethod() ResourceWarning: Enable tracemalloc to get the object allocation traceback .. -- Ran 153 tests in 66.182s OK ]]] On FreeBSD those warning are also produced. [[[ futatuki@retina-alpha[15] make check-swig-py if [ "LD_LIBRARY_PATH" = "DYLD_LIBRARY_PATH" ]; then for d in /home/futatuki/work/subversion/vwc/branches/swig-py3/subversion/bindings/swig/python/libsvn_swig_py /home/futatuki/work/subversion/vwc/branches/swig-py3/subversion/bindings/swig/python/../../../libsvn_*; do if [ -n "$DYLD_LIBRARY_PATH" ]; then LD_LIBRARY_PATH="$LD_LIBRARY_PATH:$d/.libs"; else LD_LIBRARY_PATH="$d/.libs"; fi; done; export LD_LIBRARY_PATH; fi; cd /home/futatuki/work/subversion/vwc/branches/swig-py3/subversion/bindings/swig/python; /usr/local/bin/python3.7 /home/futatuki/work/subversion/vwc/branches/swig-py3/subversion/bindings/swig/python/tests/run_all.py ../usr/local/lib/python3.7/subprocess.py:858: ResourceWarning: subprocess 81136 is still running ResourceWarning, source=self) ResourceWarning: Enable tracemalloc to get the object allocation traceback /usr/local/lib/python3.7/unittest/case.py:615: ResourceWarning: unclosed file <_io.BufferedReader name=4> testMethod() ResourceWarning: Enable tracemalloc to get the object allocation traceback ./usr/local/lib/python3.7/unittest/case.py:615: ResourceWarning: unclosed file <_io.BufferedReader name='/tmp/tmporh5038d'> testMethod() ResourceWarning: Enable tracemalloc to get the object allocation traceback .. -- Ran 153 tests in 10.773s OK ]]] I think these warnings comes from difference of warning between Python 2 and Python 3, and not affect quality of swig Python binding itself but affect quality of its test code. ResourceWarning itself appeared in Python 3.2 https://docs.python.org/3/whatsnew/3.2.html (section "Other Language Changes") If we can allow this warning and want to hide it, we can use environment variable PYTHONWARNINGS='ignore::ResourceWarning::', as the document above says. [[[ futatuki@retina-alpha[17] env PYTHONWARNINGS='ignore::ResourceWarning::' make check-swig-py if [ "LD_LIBRARY_PATH" = "DYLD_LIBRARY_PATH" ]; then for d in /home/futatuki/work/subversion/vwc/branches/swig-py3/subversion/bindings/swig/python/libsvn_swig_py /home/futatuki/work/subversion/vwc/branches/swig-py3/subversion/bindings/swig/python/../../../libsvn_*; do if [ -n "$DYLD_LIBRARY_PATH" ]; then LD_LIBRARY_PATH="$LD_LIBRARY_PATH:$d/.libs"; else LD_LIBRARY_PATH="$d/.libs"; fi; done; export LD_LIBRARY_PATH; fi; cd /home/futatuki/work/subversion/vwc/branches/swig-py3/subversion/bindings/swig/python; /usr/local/bin/python3.7 /home/futatuki/work/subversion/vwc/branches/swig-py3/subversion/bindings/swig/python/tests/run_all.py . -- Ran 153 tests in 10.757s OK ]]] Of course, we can also resolve those warnings by fixing clean up code on tests. (The warning about 'ruby' is not a big deal I suppose, but it's also something I saw when running gen-make.py with python 3.7 -- not when I'm running it with python 2.7) I think that message comes from build/generator/gen_win_dependencies.py, GenDependenciesBase._find_ruby() line 967 (message from shell program on Windows). https://svn.apache.org/viewvc/subversion/branches/swig-py3/build/generator/gen_win_dependencies.py?revision=1862754=markup#l967 This may be also caused with Python 2, if 'ruby' is not in command search path. Thanks. I'll try to take a closer look, but it's not a priority. It's still a bit strange to me that Python 2 doesn't show this warning while Python 3 does
