| Message ID | 20241108-pks-clar-build-improvements-v1-0-25c1fe65ce37@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | Build improvements for clar | expand |
Hi Patrick On 08/11/2024 13:16, Patrick Steinhardt wrote: > Hi, > > Dscho has reported in [1] that the CMake build instructions for clar do > not work well on Windows/MSVC because we execute the shell scripts > directly instead of using the discovered `SH_EXE`. This small patch > series fixes the issue. I've been using the CMake build in Visual Studio the last couple of days as my hard drive with linux on it died. I ended up with a slightly different fix using "sh -c" rather than putting the awk script inside a shell script. See the diff below. I don't have a strong preference either way but it would be nice to fix the line wrapping and add VERBATIM so that paths containing special characters are quoted correctly Best Wishes Phillip ---- >8 ---- diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index f0a1a75382a..b8a37b3870d 100644 --- a/contrib/buildsystems/CMakeLists.txt +++ b/contrib/buildsystems/CMakeLists.txt @@ -989,11 +989,21 @@ parse_makefile_for_scripts(clar_test_SUITES "CLAR_TEST_SUITES" "") list(TRANSFORM clar_test_SUITES PREPEND "${CMAKE_SOURCE_DIR}/t/unit-tests/") list(TRANSFORM clar_test_SUITES APPEND ".c") add_custom_command(OUTPUT "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h" - COMMAND ${CMAKE_SOURCE_DIR}/t/unit-tests/generate-clar-decls.sh "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h" ${clar_test_SUITES} - DEPENDS ${CMAKE_SOURCE_DIR}/t/unit-tests/generate-clar-decls.sh ${clar_test_SUITES}) + COMMAND ${SH_EXE} + "${CMAKE_SOURCE_DIR}/t/unit-tests/generate-clar-decls.sh" + "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h" + ${clar_test_SUITES} + DEPENDS "${CMAKE_SOURCE_DIR}/t/unit-tests/generate-clar-decls.sh" + ${clar_test_SUITES} + VERBATIM) add_custom_command(OUTPUT "${CMAKE_BINARY_DIR}/t/unit-tests/clar.suite" - COMMAND awk -f "${CMAKE_SOURCE_DIR}/t/unit-tests/clar-generate.awk" "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h" > "${CMAKE_BINARY_DIR}/t/unit-tests/clar.suite" - DEPENDS "${CMAKE_SOURCE_DIR}/t/unit-tests/clar-generate.awk" "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h") + COMMAND ${SH_EXE} "-c" [[awk -f "$1" "$2" >"$3"]] awk + "${CMAKE_SOURCE_DIR}/t/unit-tests/clar-generate.awk" + "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h" + "${CMAKE_BINARY_DIR}/t/unit-tests/clar.suite" + DEPENDS "${CMAKE_SOURCE_DIR}/t/unit-tests/clar-generate.awk" + "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h" + VERBATIM) add_library(unit-tests-lib ${clar_test_SUITES} "${CMAKE_SOURCE_DIR}/t/unit-tests/clar/clar.c"
Phillip Wood <phillip.wood123@gmail.com> writes: > I've been using the CMake build in Visual Studio the last couple of days > as my hard drive with linux on it died. I ended up with a slightly > different fix using "sh -c" rather than putting the awk script inside > a shell script. See the diff below. I don't have a strong preference > either way but it would be nice to fix the line wrapping and add > VERBATIM so that paths containing special characters are quoted correctly Thanks for comments. I've committed the same sin number of times, but a scriptlet written in a third language as a string literal in a shell script is somewhat awkward to maintain, so I may have slight preference for your variant. Either way, we are now letting the shell, and not CMake, to spawn "awk", so if that was the reason why the file needs to be changed (i.e. CMake perhaps failed to or found a wrong awk), either of your two approaches would solve that by delegating the task to the shell. > > Best Wishes > > Phillip > > ---- >8 ---- > > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt > index f0a1a75382a..b8a37b3870d 100644 > --- a/contrib/buildsystems/CMakeLists.txt > +++ b/contrib/buildsystems/CMakeLists.txt > @@ -989,11 +989,21 @@ parse_makefile_for_scripts(clar_test_SUITES "CLAR_TEST_SUITES" "") > list(TRANSFORM clar_test_SUITES PREPEND "${CMAKE_SOURCE_DIR}/t/unit-tests/") > list(TRANSFORM clar_test_SUITES APPEND ".c") > add_custom_command(OUTPUT "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h" > - COMMAND ${CMAKE_SOURCE_DIR}/t/unit-tests/generate-clar-decls.sh "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h" ${clar_test_SUITES} > - DEPENDS ${CMAKE_SOURCE_DIR}/t/unit-tests/generate-clar-decls.sh ${clar_test_SUITES}) > + COMMAND ${SH_EXE} > + "${CMAKE_SOURCE_DIR}/t/unit-tests/generate-clar-decls.sh" > + "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h" > + ${clar_test_SUITES} > + DEPENDS "${CMAKE_SOURCE_DIR}/t/unit-tests/generate-clar-decls.sh" > + ${clar_test_SUITES} > + VERBATIM) > add_custom_command(OUTPUT "${CMAKE_BINARY_DIR}/t/unit-tests/clar.suite" > - COMMAND awk -f "${CMAKE_SOURCE_DIR}/t/unit-tests/clar-generate.awk" "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h" > "${CMAKE_BINARY_DIR}/t/unit-tests/clar.suite" > - DEPENDS "${CMAKE_SOURCE_DIR}/t/unit-tests/clar-generate.awk" "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h") > + COMMAND ${SH_EXE} "-c" [[awk -f "$1" "$2" >"$3"]] awk > + "${CMAKE_SOURCE_DIR}/t/unit-tests/clar-generate.awk" > + "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h" > + "${CMAKE_BINARY_DIR}/t/unit-tests/clar.suite" > + DEPENDS "${CMAKE_SOURCE_DIR}/t/unit-tests/clar-generate.awk" > + "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h" > + VERBATIM) > add_library(unit-tests-lib ${clar_test_SUITES} > "${CMAKE_SOURCE_DIR}/t/unit-tests/clar/clar.c"
On Mon, Nov 11, 2024 at 10:34:27AM +0900, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > > > I've been using the CMake build in Visual Studio the last couple of days > > as my hard drive with linux on it died. I ended up with a slightly > > different fix using "sh -c" rather than putting the awk script inside > > a shell script. See the diff below. I don't have a strong preference > > either way but it would be nice to fix the line wrapping and add > > VERBATIM so that paths containing special characters are quoted correctly > > Thanks for comments. > > I've committed the same sin number of times, but a scriptlet written > in a third language as a string literal in a shell script is > somewhat awkward to maintain, so I may have slight preference for > your variant. Either way, we are now letting the shell, and not > CMake, to spawn "awk", so if that was the reason why the file needs > to be changed (i.e. CMake perhaps failed to or found a wrong awk), > either of your two approaches would solve that by delegating the > task to the shell. Yeah, I don't think it's particularly beautiful either. Personally, I'd still lean towards my solution, mostly because it allows us to iterate more readily in the future in case we ever need more logic in this context, and it avoids having to handle the redirect in the build system. So I'll take the VERBATIM and line wrapping suggestions, but for now keep the shell script. I'll adapt though if you think that this is too much of an abomination. Patrick
Patrick Steinhardt <ps@pks.im> writes: > So I'll take the VERBATIM and line wrapping suggestions, but for now > keep the shell script. I'll adapt though if you think that this is too > much of an abomination. I do not expect me to be editing the awk script buried inside a shell string literal myself, so it is more or less a "meh" to me, but once people start iterating, they may find it irritating that their favourite editor do not help them with its "awk" mode while editing the script. Once that happens, they may split the script out themselves. Before that, I do not deeply care ;-) Thanks.
Hi, Dscho has reported in [1] that the CMake build instructions for clar do not work well on Windows/MSVC because we execute the shell scripts directly instead of using the discovered `SH_EXE`. This small patch series fixes the issue. [1]: <3b2cb360-297a-915c-ae27-c45f38fa49b9@gmx.de> Thanks! Patrick Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Patrick Steinhardt (3): t/unit-tests: convert "clar-generate.awk" into a shell script cmake: use SH_EXE to execute clar scripts Makefile: let clar header targets depend on their scripts Makefile | 6 ++-- contrib/buildsystems/CMakeLists.txt | 6 ++-- t/unit-tests/clar-generate.awk | 50 ---------------------------- t/unit-tests/generate-clar-suites.sh | 63 ++++++++++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 56 deletions(-) --- base-commit: facbe4f633e4ad31e641f64617bc88074c659959 change-id: 20241108-pks-clar-build-improvements-1c3962a9a79f Best regards,