On Tue, Nov 26, 2024 at 4:25 AM Jun Omae <jun6...@gmail.com> wrote:
>
> On 2024/11/25 21:00, Timofei Zhakov wrote:
> > Hi Daniel!
> >
> > I am happy to see your feedback.
> >
> > On Sun, Nov 24, 2024 at 6:17 PM Daniel Shahaf <d...@daniel.shahaf.name>
> > wrote:
> >>
> >> rin...@apache.org wrote on Thu, 26 Sep 2024 13:22 +00:00:
> >>> Fix command-line tests on Linux by removing configuration of svneditor.sh
> >>> script.
> >>>
> >>> See also 'Removing configuration of svneditor.sh script?' thread on dev@
> >>> [1].
> >>
> >> The log message says "Fix", but neither the log message nor the thread
> >> explains what wasn't working.
> >
> > This log message explains in context of the CMake build-system that wasn't
> > supporting it previously. Probably, I didn't explain it enough in the
> > log-message.
> >
> >> More importantly, using trunk@HEAD:
> >>
> > [...]
> >>
> >> Reverting r1920955 makes that test PASS.
> >>
> >> Please fix.
>
> Ah, I have tested in tree build but haven't tested in out-of-tree build on the
> reviewing.
>
> > I've just checked the configuration you are talking about, and the problem
> > has
> > been reproduced. Thanks for noticing and reporting it!
> >
> > After some research, I found the source of it. The path to svneditor_script
> > was previously being determined using process' current directory on Unix,
> > while Windows reads it from something like the current script path (I am not
> > exploring the details, just quoting an answer from stackoverflow).
> >
> > If we use the same approach as Windows on all platforms, we first will
> > minimize
> > the differences between platforms, and secondly resolve the issue you've
> > just
> > explained. I would like to suggest the following patch:
> >
> > [[[
> > Index: subversion/tests/cmdline/svntest/main.py
> > ===================================================================
> > --- subversion/tests/cmdline/svntest/main.py (revision 1922052)
> > +++ subversion/tests/cmdline/svntest/main.py (working copy)
> > @@ -141,9 +141,7 @@
> > svneditor_script = os.path.join(sys.path[0], 'svneditor.bat')
> > else:
> > # This script is in the build tree, not in the source tree.
> > - svneditor_script = os.path.join(os.path.dirname(
> > -
> > os.path.dirname(os.path.abspath('.'))),
> > - 'tests/cmdline/svneditor.sh')
> > + svneditor_script = os.path.join(sys.path[0], 'svneditor.sh')
> >
> > # Username and password used by the working copies
> > wc_author = 'jrandom'
> > ]]]
> >
> > What do you think? Did it work for you?
>
> The patch looks good to me (tested in tree and out-of-tree builds). However, I
> think "# This script is in the build tree, ..." comment should be removed
> also.
Oh, I didn't notice this comment. I agree that it should be removed as well.
Thanks for reviewing!
--
Timofei Zhakov
