You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jun Omae <ju...@gmail.com> on 2024/11/26 03:24:13 UTC
Re: autoconf out-of-tree builds broken
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:
>>
>> rinrab@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.
--
Jun Omae <ju...@gmail.com> (大前 潤)
Re: autoconf out-of-tree builds broken
Posted by Timofei Zhakov <ri...@apache.org>.
On Tue, Nov 26, 2024 at 4:25 AM Jun Omae <ju...@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:
> >>
> >> rinrab@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
Re: autoconf out-of-tree builds broken
Posted by Daniel Sahlberg <da...@gmail.com>.
Den tis 26 nov. 2024 kl 04:25 skrev Jun Omae <ju...@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:
> >>
> >> rinrab@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