Den tis 26 nov. 2024 kl 04:25 skrev Jun Omae <jun6...@gmail.com>:
> 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.
>
This works for me too and I particularly like that it mirrors behaviour
between platforms. Makes it easier to review the code without having to
think about why the build system finds the script in different ways in
different platforms.
Cheers,
Daniel Sahlberg
