On Mon, Sep 23, 2024 at 9:25 AM Jun Omae <jun6...@gmail.com> wrote:
>
> On 2024/09/22 22:55, Timofei Zhakov wrote:
> > On Sun, Sep 22, 2024 at 2:02 AM Jun Omae <jun6...@gmail.com> wrote:
> >>
> >> On 2024/09/21 5:50, Timofei Zhakov wrote:
> >>> On Fri, Sep 20, 2024 at 11:30 AM Jun Omae <jun6...@gmail.com> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> On 2024/09/16 21:41, rin...@apache.org wrote:
> >>>>> Author: rinrab
> >>>>> Date: Mon Sep 16 12:41:13 2024
> >>>>> New Revision: 1920717
> >>>>>
> >>>>> URL: http://svn.apache.org/viewvc?rev=1920717&view=rev
> >>>>> Log:
> >>>>> Merge the `cmake` branch to trunk.
> >>>>
> >>>> I encounter the following error on all of cmdline.* tests with Visual
> >>>> Studio
> >>>> as cmake generator.
> >>>>
> >>>> [[[
> >>> [...]
> >>>> ]]]
> >>>>
> >>>> The exception is raised when launching <build-dir>/svnadmin.exe because
> >>>> the
> >>>> file is actually located in <build-dir>/Release/svnadmin.exe. It is not
> >>>> raised
> >>>> when cmake with Ninja is used because <build-dir>/svnadmin.exe is
> >>>> located.
> >>>>
> >>>> I think we could use $<TARGET_FILE_DIR:target> instead of
> >>>> ${CMAKE_CURRENT_BINARY_DIR} as a workaround.
> >>>>
> >>>> [[[
> >>>> Index: CMakeLists.txt
> >>>> ===================================================================
> >>>> --- CMakeLists.txt (revision 1920797)
> >>>> +++ CMakeLists.txt (working copy)
> >>>> @@ -671,13 +671,13 @@
> >>>> "cmdline.${py_test_name}"
> >>>> COMMAND
> >>>> "${Python3_EXECUTABLE}"
> >>>> "${CMAKE_CURRENT_SOURCE_DIR}/build/run_tests.py"
> >>>> - --bin ${CMAKE_CURRENT_BINARY_DIR}
> >>>> - --tools-bin ${CMAKE_CURRENT_BINARY_DIR}
> >>>> + --bin $<TARGET_FILE_DIR:svnadmin>
> >>>> + --tools-bin $<TARGET_FILE_DIR:entries-dump>
> >>>> --verbose
> >>>> --log-to-stdout
> >>>> --set-log-level=WARNING
> >>>> ${CMAKE_CURRENT_SOURCE_DIR}
> >>>> - ${CMAKE_CURRENT_BINARY_DIR}
> >>>> + $<TARGET_FILE_DIR:svnadmin>
> >>>> ${py_test_path}
> >>>> )
> >>>> endif()
> >>>> ]]]
> >>>
> >>> Oh, I didn't notice this problem, because I am using a single config
> >>> generator. I think the suggested code is great for resolving the
> >>> issue, because I think there is no generator expression to get the
> >>> target dir in the current config without a specific target. However, I
> >>> think it's better to use svn's target dir since it's the main
> >>> executable and maybe the same for tools (?). It actually should be the
> >>> same for all targets, so it doesn't meter.
> >>>
> >>> Also, I think we should add some multi-config generator tests to the
> >>> GitHub Actions workflow, to prevent similar problems in future.
> >>>
> >>
> >> I use "svnadmin" for target directory because we use "svnadmin" to create a
> >> test repository before using "svn" to check out the repository. Well, using
> >> "svn" is okay to me.
> >
> > Okay.
> >
> >> Revised the proposed patch, in addition, added use of VS2022 to the matrix:
> >> cmake-test-build-dir.v2.patch.txt (attached).
> >
> > Would this be better to separate this patch by two different commits,
> > with tests and GitHub actions?
>
> Yeah. Separated the commit to two commits:
>
> -
> https://github.com/apache/subversion/commit/5dff28422740659ea9031d7287ce33785cd08a4b
> -
> https://github.com/apache/subversion/commit/c456dfd0311201baab3f3eaf94573ed8b1ba408a
Great!
> >> However, the same issue occurs on ra-test. I think the issue should be
> >> occurred
> >> also when Ninja is used with build directory being outside of source tree
> >> (e.g.
> >> %TMP%\svn-out). Because "../../svnserve/svnserve.exe" and "svnserve.exe"
> >> are
> >> searched.
> >>
> >> [[[
> >> 40: Test command: D:\a\subversion\subversion\out\Release\ra-test.exe
> >> "--srcdir" "D:/a/subversion/subversion/subversion/tests/libsvn_ra"
> >> 40: Working Directory: D:/a/subversion/subversion/out
> >> 40: Test timeout computed to be: 10000000
> >> 40: PASS: ra-test 1: test svn_ra_get_location_segments
> >> 40: PASS: ra-test 2: test ra_svn tunnel callback check
> >> 40: svn_tests: E170013: Unable to connect to a repository at URL
> >> 'svn+test://localhost/test-repo-tunnel'
> >> 40: svn_tests: E200006: Could not find svnserve. Checked paths:
> >> 40: D:\a\subversion\svnserve\svnserve.exe
> >> 40: D:\a\subversion\subversion\out\svnserve.exe
> >> 40: FAIL: ra-test 3: test ra_svn tunnel creation callbacks
> >> 40: PASS: ra-test 4: lock multiple paths
> >> 40: PASS: ra-test 5: test ra_get_dir2
> >> ...
> >> ]]]
> >
> > I think changing the working directory of where the tests are running
> > to the build directory in the current config.
> >
> > See 'svn-cmake-ctest-working-directory.patch.txt' in the attachments.
>
> Thanks for the patch. However, after the patch, tests with both ninja and
> vs2022 failed. See
> https://github.com/jun66j5/subversion/actions/runs/10987698238
>
> I don't think `--srcdir` option should be changed. Reverting the changes
> for the option, jobs except ubuntu successed.
>
> -
> https://github.com/apache/subversion/compare/trunk...jun66j5:subversion:cmake-pytests-build-dir
> - https://github.com/jun66j5/subversion/actions/runs/10988627279
Oups, I see. It is a copy-paste mistake, because this code has been
duplicated from targets.cmake to target.cmake.ezt, and this variable
got expanded.
Attached a new version of the patch as
'svn-cmake-ctest-working-directory-v2.patch.txt' with reverted
--srcdir change. Log message:
[[[
cmake: Fix C-Tests failing in multi-config generators.
* build/generator/templates/targets.cmake.ezt
(tests): Use `$<TARGET_FILE_DIR:...>` generator expression instead of using
`CMAKE_CURRENT_BINARY_DIR` variable for determining the path to a directory
with built Subversion binaries
]]]
Succeeded GitHub Actions (except tests, failing on Linux):
https://github.com/rinrab/subversion/actions/runs/10993560311
Comparison of all these commits after doing some Git-magic:
https://github.com/apache/subversion/compare/trunk...rinrab:subversion:cmake-test-build-dir
Finally, I think we can start committing those changes into trunk.
What do you think?
Thanks!
--
Timofei Zhakov
Index: build/generator/templates/targets.cmake.ezt
===================================================================
--- build/generator/templates/targets.cmake.ezt (revision 1920842)
+++ build/generator/templates/targets.cmake.ezt (working copy)
@@ -62,7 +62,11 @@
add_executable([targets.name][for targets.sources]
[targets.sources][end]
)
- add_test([targets.namespace].[targets.name] [targets.name] --srcdir
${CMAKE_SOURCE_DIR}/[targets.srcdir])
+ add_test(
+ NAME [targets.namespace].[targets.name]
+ COMMAND [targets.name] --srcdir ${CMAKE_SOURCE_DIR}/[targets.srcdir]
+ WORKING_DIRECTORY $<TARGET_FILE_DIR:[targets.name]>
+ )
[end]target_link_libraries([targets.name] PRIVATE[for targets.libs]
[targets.libs][end]
)[if-any targets.msvc_libs]
