Re: Close SVN-1778
Den tors 5 okt. 2023 kl 07:10 skrev Yasuhito FUTATSUKI < futat...@yf.bsdclub.org>: > On 2023/10/05 5:28, Nathan Hartman wrote: > > On Wed, Oct 4, 2023 at 10:59 AM Yasuhito FUTATSUKI < > futat...@yf.bsdclub.org> > > wrote: > > >> Nothing but I don't want that we actively drop Python 2 support, > >> at least SWIG Python bindings even in trunk, at least now. > > > > > > I agree we shouldn't break Python 2 support if it's currently working. At > > least, let's delay breaking it until 1.14.x is EOL, to avoid breakage on > > very long term support OS. (I think that if we start removing Py2 compat > on > > trunk now, we risk mistakenly backporting the changes to 1.14.x at some > > later time.) > > More over, I know and perhaps we all know one large site that is still > using > Python 2 bindings through ViewVC 1.1.x, although I don't know what version > of Subversion it uses. > > > However, I see the point that it would be nice to make the code clear and > > not require us to remember that OSError needs to change to > > FileNotFoundError in the future. > > > > We could add a Python2 check and handle it differently. In the future > when > > we decide to actively remove Python2 support, we can grep for all the > > Python2 checks and remove that code. I know it makes the code > long-winded. > > :-/ > > Or just note it in a comment, then it does not need extra cost in > execution. > > Clean up of Python2 support code is like a conversion from pure Python 2 > code > to Python 2/Python 3 dual support code, especially in SWIG Python bindings, > because it uses py3c, Python C API wrapper to absorb difference between for > Python 2 and for Python 3. To remove py3c dependency, we should use native > Python 3 C APIs. Anyways it need a whole code review, not just grep the > markers, although the markers will help us greatly. > Agreed that it is a big undertaking. I do think it should be done but we should probably announce it well ahead to let anyone be prepared to update. When I get some time, I would like to start a new thread about the future development (release schedule, features for each release) and then we can publish something of what should go into each release. Anyway, thanks for the review and feedback. I've commtted r1912743 catching OSError and checking ENOENT, with a comment to change it when we remove Python 2 support. Kind regards, Daniel Sahlberg
Re: Close SVN-1778
On 2023/10/05 5:28, Nathan Hartman wrote: On Wed, Oct 4, 2023 at 10:59 AM Yasuhito FUTATSUKI wrote: Nothing but I don't want that we actively drop Python 2 support, at least SWIG Python bindings even in trunk, at least now. I agree we shouldn't break Python 2 support if it's currently working. At least, let's delay breaking it until 1.14.x is EOL, to avoid breakage on very long term support OS. (I think that if we start removing Py2 compat on trunk now, we risk mistakenly backporting the changes to 1.14.x at some later time.) More over, I know and perhaps we all know one large site that is still using Python 2 bindings through ViewVC 1.1.x, although I don't know what version of Subversion it uses. However, I see the point that it would be nice to make the code clear and not require us to remember that OSError needs to change to FileNotFoundError in the future. We could add a Python2 check and handle it differently. In the future when we decide to actively remove Python2 support, we can grep for all the Python2 checks and remove that code. I know it makes the code long-winded. :-/ Or just note it in a comment, then it does not need extra cost in execution. Clean up of Python2 support code is like a conversion from pure Python 2 code to Python 2/Python 3 dual support code, especially in SWIG Python bindings, because it uses py3c, Python C API wrapper to absorb difference between for Python 2 and for Python 3. To remove py3c dependency, we should use native Python 3 C APIs. Anyways it need a whole code review, not just grep the markers, although the markers will help us greatly. Cheers, -- Yasuhito FUTATSUKI
Re: Close SVN-1778
On Wed, Oct 4, 2023 at 10:59 AM Yasuhito FUTATSUKI wrote: > Hello, > > On 2023/10/04 18:02, Daniel Sahlberg wrote: > > Den ons 4 okt. 2023 kl 06:58 skrev Nathan Hartman < > hartman.nat...@gmail.com > >> : > > > >> Thanks for the review! Committed in r1912724. More below: > >> > > > > Great! > > > > > >> I see only one issue: FileNotFoundError is new in Python 3, so Python 2 > >> will fail with a NameError when it sees that. However: On Python 3, > >> FileNotFoundError inherits from OSError, OSError exists in Python 2, and > >> OSError in both Pythons has the strerror attribute. So (unless I'm > missing > >> something) we should catch OSError instead of FileNotFoundError here for > >> compatibility. > >> > > > > Good point. If we catch OSError we should check for err.errno = > > errno.ENOENT (as is done in subversion/bindings/swig/python/tests/fs.py). > > > > I don't think anything has formally been decided regarding Python 2 > > support, we have tried hard to keep Python 2 compatibility in 1.14 but > for > > /trunk (and a coming 1.15 release) my opinion is that we should remove > it. > > This should probably be broken out to a separate thread and documented > > somewhere. > > > > I think catching FileNotFoundError is cleaner than OSError + check for > > ENOENT. Also there is no immediate need to backport to 1.14 (this is > just a > > better error message). With that in mind, I'm leaning towards keeping > > FileNotFoundError (we should probably update tests/fs.py to follow the > same > > pattern). > > > > Yasuhito / Jun: Since you are Python experts, do you have any comments? > > Nothing but I don't want that we actively drop Python 2 support, > at least SWIG Python bindings even in trunk, at least now. I agree we shouldn't break Python 2 support if it's currently working. At least, let's delay breaking it until 1.14.x is EOL, to avoid breakage on very long term support OS. (I think that if we start removing Py2 compat on trunk now, we risk mistakenly backporting the changes to 1.14.x at some later time.) However, I see the point that it would be nice to make the code clear and not require us to remember that OSError needs to change to FileNotFoundError in the future. We could add a Python2 check and handle it differently. In the future when we decide to actively remove Python2 support, we can grep for all the Python2 checks and remove that code. I know it makes the code long-winded. :-/ Yasuhito and Jun know much more than me about Python so maybe there is a better way. Cheers Nathan
Re: Close SVN-1778
Hello, On 2023/10/04 18:02, Daniel Sahlberg wrote: Den ons 4 okt. 2023 kl 06:58 skrev Nathan Hartman : Thanks for the review! Committed in r1912724. More below: Great! I see only one issue: FileNotFoundError is new in Python 3, so Python 2 will fail with a NameError when it sees that. However: On Python 3, FileNotFoundError inherits from OSError, OSError exists in Python 2, and OSError in both Pythons has the strerror attribute. So (unless I'm missing something) we should catch OSError instead of FileNotFoundError here for compatibility. Good point. If we catch OSError we should check for err.errno = errno.ENOENT (as is done in subversion/bindings/swig/python/tests/fs.py). I don't think anything has formally been decided regarding Python 2 support, we have tried hard to keep Python 2 compatibility in 1.14 but for /trunk (and a coming 1.15 release) my opinion is that we should remove it. This should probably be broken out to a separate thread and documented somewhere. I think catching FileNotFoundError is cleaner than OSError + check for ENOENT. Also there is no immediate need to backport to 1.14 (this is just a better error message). With that in mind, I'm leaning towards keeping FileNotFoundError (we should probably update tests/fs.py to follow the same pattern). Yasuhito / Jun: Since you are Python experts, do you have any comments? Nothing but I don't want that we actively drop Python 2 support, at least SWIG Python bindings even in trunk, at least now. Cheers, -- Yasuhito FUTATSUKI
Re: Close SVN-1778
Den ons 4 okt. 2023 kl 06:58 skrev Nathan Hartman : > Thanks for the review! Committed in r1912724. More below: > Great! > I see only one issue: FileNotFoundError is new in Python 3, so Python 2 > will fail with a NameError when it sees that. However: On Python 3, > FileNotFoundError inherits from OSError, OSError exists in Python 2, and > OSError in both Pythons has the strerror attribute. So (unless I'm missing > something) we should catch OSError instead of FileNotFoundError here for > compatibility. > Good point. If we catch OSError we should check for err.errno = errno.ENOENT (as is done in subversion/bindings/swig/python/tests/fs.py). I don't think anything has formally been decided regarding Python 2 support, we have tried hard to keep Python 2 compatibility in 1.14 but for /trunk (and a coming 1.15 release) my opinion is that we should remove it. This should probably be broken out to a separate thread and documented somewhere. I think catching FileNotFoundError is cleaner than OSError + check for ENOENT. Also there is no immediate need to backport to 1.14 (this is just a better error message). With that in mind, I'm leaning towards keeping FileNotFoundError (we should probably update tests/fs.py to follow the same pattern). Yasuhito / Jun: Since you are Python experts, do you have any comments? Kind regards, Daniel >
Re: Close SVN-1778
On Tue, Oct 3, 2023 at 2:01 AM Daniel Sahlberg wrote: > Den tis 3 okt. 2023 kl 02:44 skrev Nathan Hartman < > hartman.nat...@gmail.com>: > >> On Mon, Oct 2, 2023 at 5:39 AM Daniel Sahlberg >> wrote: >> >> Suggested docstring (also attached as a patch): >> >> """Perform diff and return a file object from which the output can >> be read. >> >> When DIFFOPTIONS is None (the default), use svn's internal diff. >> >> With any other DIFFOPTIONS, exec the external diff found on PATH, >> passing it DIFFOPTIONS. On Windows, exec diff.exe rather than >> diff. If a diff utility is not installed or found on PATH, throws >> FileNotFoundError. Caveat: On some systems, including Windows, an >> external diff may not be available unless installed and added to >> PATH manually. >> """ >> >> More below: >> > > This looks good to me! > Thanks for the review! Committed in r1912724. More below: Agreed! Being fancy, how about: > > [[[ > Index: subversion/bindings/swig/python/svn/fs.py > === > --- subversion/bindings/swig/python/svn/fs.py (revision 1912696) > +++ subversion/bindings/swig/python/svn/fs.py (working copy) > @@ -170,8 +170,13 @@ > + [self.tempfile1, self.tempfile2] > ># open the pipe, and return the file object for reading from the > child. > - p = _subprocess.Popen(cmd, stdout=_subprocess.PIPE, bufsize=-1, > -close_fds=_sys.platform != "win32") > + try: > +p = _subprocess.Popen(cmd, stdout=_subprocess.PIPE, bufsize=-1, > + close_fds=_sys.platform != "win32") > + except FileNotFoundError as e: > +e.strerror = "External diff command not found in PATH" > +raise > + >return _PopenStdoutWrapper(p) > > else: > ]]] > > In my testing that gave me a FileNotFoundError with a custom error message > instead of the generic "No such file or directory". > I see only one issue: FileNotFoundError is new in Python 3, so Python 2 will fail with a NameError when it sees that. However: On Python 3, FileNotFoundError inherits from OSError, OSError exists in Python 2, and OSError in both Pythons has the strerror attribute. So (unless I'm missing something) we should catch OSError instead of FileNotFoundError here for compatibility. (Excuse my C++ terminology, "throw".."catch" :-) Cheers, Nathan
Re: Close SVN-1778
Den tis 3 okt. 2023 kl 02:44 skrev Nathan Hartman : > On Mon, Oct 2, 2023 at 5:39 AM Daniel Sahlberg > wrote: > > Den sön 1 okt. 2023 kl 21:50 skrev Nathan Hartman < > hartman.nat...@gmail.com>: > (snip) > >> This (somewhat platform-specific) "gotcha" isn't completely obvious > from a first glance at the code. > >> > >> Your explanation above makes the failure mode and its reasons more > clear. > >> > >> So, to close SVN-1778, I would suggest only to add some documentation > of the above fact to the function. I'll be happy to compose a suggested > docstring. > > > > > > Please do! > > Suggested docstring (also attached as a patch): > > """Perform diff and return a file object from which the output can > be read. > > When DIFFOPTIONS is None (the default), use svn's internal diff. > > With any other DIFFOPTIONS, exec the external diff found on PATH, > passing it DIFFOPTIONS. On Windows, exec diff.exe rather than > diff. If a diff utility is not installed or found on PATH, throws > FileNotFoundError. Caveat: On some systems, including Windows, an > external diff may not be available unless installed and added to > PATH manually. > """ > > More below: > This looks good to me! > > >> I don't know how it fails (with a cryptic traceback message?) but if we > want to get fancy, perhaps a failure could give some user-friendly hint > about things to check for (such as whether a diff tool is available and > diffoptions are correct). > > > > > > It fails with a FileNotFoundError error so it should be trivial to > catch. I don't know the proper conventions on error handling in the > bindings (or in Python in general), I guess we should still throw an error, > hoping that whoever use the FileDiff class will catch it and handle in an > intelligent way. > > > > Another way would be to use the internal diff in case of an exception, > but it would be less apparent (I would rather raise an error, even if it > just means ignoring the FileNotFoundError and documenting the cause). > > In this case, I would rather leave the implementation as it is > currently. I think that calling the internal diff when an external > diff is expected (with some unknown options) would be worse than > failing with FileNotFoundError here. Even if the exception is not > caught, the docstring should provide the useful hint. :-) > Agreed! Being fancy, how about: [[[ Index: subversion/bindings/swig/python/svn/fs.py === --- subversion/bindings/swig/python/svn/fs.py (revision 1912696) +++ subversion/bindings/swig/python/svn/fs.py (working copy) @@ -170,8 +170,13 @@ + [self.tempfile1, self.tempfile2] # open the pipe, and return the file object for reading from the child. - p = _subprocess.Popen(cmd, stdout=_subprocess.PIPE, bufsize=-1, -close_fds=_sys.platform != "win32") + try: +p = _subprocess.Popen(cmd, stdout=_subprocess.PIPE, bufsize=-1, + close_fds=_sys.platform != "win32") + except FileNotFoundError as e: +e.strerror = "External diff command not found in PATH" +raise + return _PopenStdoutWrapper(p) else: ]]] In my testing that gave me a FileNotFoundError with a custom error message instead of the generic "No such file or directory". Kind regards, Daniel
Re: Close SVN-1778
On Mon, Oct 2, 2023 at 5:39 AM Daniel Sahlberg wrote: > Den sön 1 okt. 2023 kl 21:50 skrev Nathan Hartman : (snip) >> This (somewhat platform-specific) "gotcha" isn't completely obvious from a >> first glance at the code. >> >> Your explanation above makes the failure mode and its reasons more clear. >> >> So, to close SVN-1778, I would suggest only to add some documentation of the >> above fact to the function. I'll be happy to compose a suggested docstring. > > > Please do! Suggested docstring (also attached as a patch): """Perform diff and return a file object from which the output can be read. When DIFFOPTIONS is None (the default), use svn's internal diff. With any other DIFFOPTIONS, exec the external diff found on PATH, passing it DIFFOPTIONS. On Windows, exec diff.exe rather than diff. If a diff utility is not installed or found on PATH, throws FileNotFoundError. Caveat: On some systems, including Windows, an external diff may not be available unless installed and added to PATH manually. """ More below: >> I don't know how it fails (with a cryptic traceback message?) but if we want >> to get fancy, perhaps a failure could give some user-friendly hint about >> things to check for (such as whether a diff tool is available and >> diffoptions are correct). > > > It fails with a FileNotFoundError error so it should be trivial to catch. I > don't know the proper conventions on error handling in the bindings (or in > Python in general), I guess we should still throw an error, hoping that > whoever use the FileDiff class will catch it and handle in an intelligent way. > > Another way would be to use the internal diff in case of an exception, but it > would be less apparent (I would rather raise an error, even if it just means > ignoring the FileNotFoundError and documenting the cause). In this case, I would rather leave the implementation as it is currently. I think that calling the internal diff when an external diff is expected (with some unknown options) would be worse than failing with FileNotFoundError here. Even if the exception is not caught, the docstring should provide the useful hint. :-) Cheers, Nathan SVN-1778-patch1.patch Description: Binary data
Re: Close SVN-1778
Den sön 1 okt. 2023 kl 21:50 skrev Nathan Hartman : > On Sun, Oct 1, 2023 at 9:58 AM Daniel Sahlberg < > daniel.l.sahlb...@gmail.com> wrote: > > > > Hi, > > > > Issue SVN-1778 [1] is about the Python bindings, class FileDiff, where > get_pipe() called an external program diff (under Windows: diff.exe). Since > diff.exe has to be installed (and added to PATH) manually on Windows the > function would fail. The same is true on other os:es but diff is probably > more commonly present by default. > > > > r1824410 added support for running the internal Subversion diff > functions (svn.diff.file_diff_2 and svn.diff.file_output_unified4) as long > as the argument diffoptions is None (the default). Since diffoptions is > passed as arguments to the diff command, it seems reasonable to require the > presence of diff (or diff.exe) if one uses diffoptions. > > > > I'm suggesting to close this issue - it is not completely solved but > when using diffoptions,it is probably expected that this arguments is > passed on to whatever diff/diff.exe executable is present on the system and > thus it is not unreasonable to require the end user to install diff.exe > > > > Kind regards, > > > > Daniel Sahlberg > > > > [1] https://issues.apache.org/jira/browse/SVN-1778 > > > This (somewhat platform-specific) "gotcha" isn't completely obvious from a > first glance at the code. > > Your explanation above makes the failure mode and its reasons more clear. > > So, to close SVN-1778, I would suggest only to add some documentation of > the above fact to the function. I'll be happy to compose a suggested > docstring. > Please do! > > I don't know how it fails (with a cryptic traceback message?) but if we > want to get fancy, perhaps a failure could give some user-friendly hint > about things to check for (such as whether a diff tool is available and > diffoptions are correct). > It fails with a FileNotFoundError error so it should be trivial to catch. I don't know the proper conventions on error handling in the bindings (or in Python in general), I guess we should still throw an error, hoping that whoever use the FileDiff class will catch it and handle in an intelligent way. Another way would be to use the internal diff in case of an exception, but it would be less apparent (I would rather raise an error, even if it just means ignoring the FileNotFoundError and documenting the cause). > > I noticed that there aren't docstrings on most of the functions in > subversion/bindings/swig/python/svn/fs.py, and I don't know whether that's > deliberate. If it is, then the suggested docstring could be a comment > instead. > There are docstrings in the other py files so I suppose this was just overlooked. Kind regards, Daniel
Re: Close SVN-1778
On Sun, Oct 1, 2023 at 9:58 AM Daniel Sahlberg wrote: > > Hi, > > Issue SVN-1778 [1] is about the Python bindings, class FileDiff, where get_pipe() called an external program diff (under Windows: diff.exe). Since diff.exe has to be installed (and added to PATH) manually on Windows the function would fail. The same is true on other os:es but diff is probably more commonly present by default. > > r1824410 added support for running the internal Subversion diff functions (svn.diff.file_diff_2 and svn.diff.file_output_unified4) as long as the argument diffoptions is None (the default). Since diffoptions is passed as arguments to the diff command, it seems reasonable to require the presence of diff (or diff.exe) if one uses diffoptions. > > I'm suggesting to close this issue - it is not completely solved but when using diffoptions,it is probably expected that this arguments is passed on to whatever diff/diff.exe executable is present on the system and thus it is not unreasonable to require the end user to install diff.exe > > Kind regards, > > Daniel Sahlberg > > [1] https://issues.apache.org/jira/browse/SVN-1778 This (somewhat platform-specific) "gotcha" isn't completely obvious from a first glance at the code. Your explanation above makes the failure mode and its reasons more clear. So, to close SVN-1778, I would suggest only to add some documentation of the above fact to the function. I'll be happy to compose a suggested docstring. I don't know how it fails (with a cryptic traceback message?) but if we want to get fancy, perhaps a failure could give some user-friendly hint about things to check for (such as whether a diff tool is available and diffoptions are correct). I noticed that there aren't docstrings on most of the functions in subversion/bindings/swig/python/svn/fs.py, and I don't know whether that's deliberate. If it is, then the suggested docstring could be a comment instead. Cheers Nathan
