Bug report

Bug report

[Ed Reel]

[-- Attachment #1: Type: text/plain, Size: 118 bytes --]

See attachment...



-- 
"God gave us two ears and one mouth to remind us we should listen
twice as much as we talk."

[-- Attachment #2: git-bugreport-2024-10-09-0244.txt --]
[-- Type: text/plain, Size: 4864 bytes --]

What did you do before the bug happened? (Steps to reproduce your issue)
Attempt to build with cmake

What did you expect to happen? (Expected behavior)
Successful build

What happened instead? (Actual behavior)
Unsuccessful build

What's different between what you expected and what actually happened?
[5*10/162] Building C object CMakeFiles/unit-tests-lib.dir/usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/strvec.c.o
FAILED: CMakeFiles/unit-tests-lib.dir/usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/strvec.c.o 
/usr/local/bin/cc -DBINDIR=\"bin\" -DDEFAULT_GIT_TEMPLATE_DIR=\"share/git-core/templates\" -DDEFAULT_HELP_FORMAT=\"html\" -DETC_GITATTRIBUTES=\"etc/gitattributes\" -DETC_GITCONFIG=\"etc/gitconfig\" -DFALLBACK_RUNTIME_PREFIX=\"/home/chronos\" -DFREAD_READS_DIRECTORIES -DGIT_BUILT_FROM_COMMIT=\"\" -DGIT_EXEC_PATH=\"libexec/git-core\" -DGIT_HOST_CPU=\"x86_64\" -DGIT_HTML_PATH=\"share/doc/git-doc\" -DGIT_INFO_PATH=\"share/info\" -DGIT_LOCALE_PATH=\"share/locale\" -DGIT_MAN_PATH=\"share/man\" -DGIT_USER_AGENT=\"git/2.47.0.GIT\" -DGIT_VERSION=\"2.47.0.GIT\" -DHAVE_ALLOCA_H -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_DEV_TTY -DHAVE_GETDELIM -DHAVE_PATHS_H -DHAVE_STRINGS_H -DHAVE_SYSINFO -DINTERNAL_QSORT -DNO_OPENSSL -DNO_STRLCPY -DPAGER_ENV="\"LESS=FRX LV=-c\"" -DPROCFS_EXECUTABLE_PATH=\"/proc/self/exe\" -DRUNTIME_PREFIX -DSHA1DC_CUSTOM_INCLUDE_SHA1_C=\"git-compat-util.h\" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C=\"git-compat-util.h\" -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1_DC -DSHA256_BLK -DSUPPORTS_SIMPLE_IPC -DUSE_CURL_FOR_IMAP_SEND -DUSE_LIBPCRE2 -I/usr/local/tmp/crew/git.20241009021715.dir/contrib/buildsystems/../.. -I/usr/local/tmp/crew/git.20241009021715.dir/contrib/buildsystems/builddir -I/usr/local/tmp/crew/git.20241009021715.dir/contrib/buildsystems/../../t/unit-tests -O2 -pipe -ffat-lto-objects -fPIC   -flto=auto -O3 -DNDEBUG -flto=auto -fno-fat-lto-objects -MD -MT CMakeFiles/unit-tests-lib.dir/usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/strvec.c.o -MF CMakeFiles/unit-tests-lib.dir/usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/strvec.c.o.d -o CMakeFiles/unit-tests-lib.dir/usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/strvec.c.o -c /usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/strvec.c
In file included from /usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/strvec.c:1:
/usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/unit-test.h:3:10: fatal error: clar-decls.h: No such file or directory
    3 | #include "clar-decls.h"
      |          ^~~~~~~~~~~~~~
compilation terminated.
[14*1/162] Linking C executable git
In function ‘strbuf_add’,
    inlined from ‘strbuf_addstr’ at /usr/local/tmp/crew/git.20241009021715.dir/contrib/buildsystems/../../strbuf.h:310:2,
    inlined from ‘verify_one_pack’ at /usr/local/tmp/crew/git.20241009021715.dir/builtin/verify-pack.c:40:3,
    inlined from ‘cmd_verify_pack’ at /usr/local/tmp/crew/git.20241009021715.dir/builtin/verify-pack.c:90:7:
/usr/local/tmp/crew/git.20241009021715.dir/strbuf.c:312:9: warning: ‘memcpy’ writing 5 bytes into a region of size 1 overflows the destination [-Wstringop-overflow=]
  312 |         memcpy(sb->buf + sb->len, data, len);
      |         ^
/usr/local/tmp/crew/git.20241009021715.dir/strbuf.c: In function ‘cmd_verify_pack’:
/usr/local/tmp/crew/git.20241009021715.dir/strbuf.c:65:6: note: destination object ‘strbuf_slopbuf’ of size 1
   65 | char strbuf_slopbuf[1];
      |      ^
In function ‘strbuf_release’,
    inlined from ‘strbuf_release’ at /usr/local/tmp/crew/git.20241009021715.dir/strbuf.c:75:6,
    inlined from ‘bundle_uri_parse_line’ at /usr/local/tmp/crew/git.20241009021715.dir/bundle-uri.c:939:2,
    inlined from ‘get_remote_bundle_uri’ at /usr/local/tmp/crew/git.20241009021715.dir/connect.c:527:8,
    inlined from ‘get_bundle_uri’ at /usr/local/tmp/crew/git.20241009021715.dir/transport.c:406:9:
/usr/local/tmp/crew/git.20241009021715.dir/strbuf.c:78:17: warning: ‘free’ called on unallocated object ‘strbuf_slopbuf’ [-Wfree-nonheap-object]
   78 |                 free(sb->buf);
      |                 ^
/usr/local/tmp/crew/git.20241009021715.dir/strbuf.c: In function ‘get_bundle_uri’:
/usr/local/tmp/crew/git.20241009021715.dir/strbuf.c:65:6: note: declared here
   65 | char strbuf_slopbuf[1];
      |      ^
ninja: build stopped: subcommand failed.
There was a build error.

[System Info]
git version:
git version 2.46.1.GIT
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
libcurl: 8.10.0
zlib: 1.3.1
uname: Linux 6.4.0-1mx-ahs-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.4.4-1~mx23+1 (2023-07-26) x86_64
compiler info: gnuc: 14.2
libc info: glibc: 2.27
$SHELL (typically, interactive shell): /bin/bash

Re: Bug report

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 5929 bytes --]

Hi Ed,

On Tue, 8 Oct 2024, Ed Reel wrote:

> See attachment...

You will find that Git mailing list readers typically frown upon having to
deal with text that was stuffed into an attachment, because it is much
harder to quote that plain text in the mail body. Maybe next time just
paste it into the mail body?

> What did you do before the bug happened? (Steps to reproduce your issue)
> Attempt to build with cmake

But which branch and/or commit?

> What did you expect to happen? (Expected behavior)
> Successful build

> What happened instead? (Actual behavior)
> Unsuccessful build

> What's different between what you expected and what actually happened?
> [5*10/162] Building C object CMakeFiles/unit-tests-lib.dir/usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/strvec.c.o
> FAILED: CMakeFiles/unit-tests-lib.dir/usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/strvec.c.o
> /usr/local/bin/cc -DBINDIR=\"bin\" -DDEFAULT_GIT_TEMPLATE_DIR=\"share/git-core/templates\" -DDEFAULT_HELP_FORMAT=\"html\" -DETC_GITATTRIBUTES=\"etc/gitattributes\" -DETC_GITCONFIG=\"etc/gitconfig\" -DFALLBACK_RUNTIME_PREFIX=\"/home/chronos\" -DFREAD_READS_DIRECTORIES -DGIT_BUILT_FROM_COMMIT=\"\" -DGIT_EXEC_PATH=\"libexec/git-core\" -DGIT_HOST_CPU=\"x86_64\" -DGIT_HTML_PATH=\"share/doc/git-doc\" -DGIT_INFO_PATH=\"share/info\" -DGIT_LOCALE_PATH=\"share/locale\" -DGIT_MAN_PATH=\"share/man\" -DGIT_USER_AGENT=\"git/2.47.0.GIT\" -DGIT_VERSION=\"2.47.0.GIT\" -DHAVE_ALLOCA_H -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_DEV_TTY -DHAVE_GETDELIM -DHAVE_PATHS_H -DHAVE_STRINGS_H -DHAVE_SYSINFO -DINTERNAL_QSORT -DNO_OPENSSL -DNO_STRLCPY -DPAGER_ENV="\"LESS=FRX LV=-c\"" -DPROCFS_EXECUTABLE_PATH=\"/proc/self/exe\" -DRUNTIME_PREFIX -DSHA1DC_CUSTOM_INCLUDE_SHA1_C=\"git-compat-util.h\" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C=\"git-compat-util.h\" -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1_DC -DSHA256_BLK -DSUPPORTS_SIMPLE_IPC -DUSE_CURL_FOR_IMAP_SEND -DUSE_LIBPCRE2 -I/usr/local/tmp/crew/git.20241009021715.dir/contrib/buildsystems/../.. -I/usr/local/tmp/crew/git.20241009021715.dir/contrib/buildsystems/builddir -I/usr/local/tmp/crew/git.20241009021715.dir/contrib/buildsystems/../../t/unit-tests -O2 -pipe -ffat-lto-objects -fPIC   -flto=auto -O3 -DNDEBUG -flto=auto -fno-fat-lto-objects -MD -MT CMakeFiles/unit-tests-lib.dir/usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/strvec.c.o -MF CMakeFiles/unit-tests-lib.dir/usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/strvec.c.o.d -o CMakeFiles/unit-tests-lib.dir/usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/strvec.c.o -c /usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/strvec.c
> In file included from /usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/strvec.c:1:
> /usr/local/tmp/crew/git.20241009021715.dir/t/unit-tests/unit-test.h:3:10: fatal error: clar-decls.h: No such file or directory
>     3 | #include "clar-decls.h"
>       |          ^~~~~~~~~~~~~~
> compilation terminated.
> [14*1/162] Linking C executable git
> In function ‘strbuf_add’,
>     inlined from ‘strbuf_addstr’ at /usr/local/tmp/crew/git.20241009021715.dir/contrib/buildsystems/../../strbuf.h:310:2,

The path starting with `/usr/` suggests that this is not Windows. And
further below, the information from a working Git suggests that you try to
build this on Linux.

At the moment, the CMake definition very much targets Windows, not any
other platform, because it provides the only convenient way to build Git
in Visual Studio without having to jump through substantial hoops.

Since you seem on a Unix-ish platform, why not try the `make`-based build,
which is how the Git maintainer builds the project?

Ciao,
Johannes

>     inlined from ‘verify_one_pack’ at /usr/local/tmp/crew/git.20241009021715.dir/builtin/verify-pack.c:40:3,
>     inlined from ‘cmd_verify_pack’ at /usr/local/tmp/crew/git.20241009021715.dir/builtin/verify-pack.c:90:7:
> /usr/local/tmp/crew/git.20241009021715.dir/strbuf.c:312:9: warning: ‘memcpy’ writing 5 bytes into a region of size 1 overflows the destination [-Wstringop-overflow=]
>   312 |         memcpy(sb->buf + sb->len, data, len);
>       |         ^
> /usr/local/tmp/crew/git.20241009021715.dir/strbuf.c: In function ‘cmd_verify_pack’:
> /usr/local/tmp/crew/git.20241009021715.dir/strbuf.c:65:6: note: destination object ‘strbuf_slopbuf’ of size 1
>    65 | char strbuf_slopbuf[1];
>       |      ^
> In function ‘strbuf_release’,
>     inlined from ‘strbuf_release’ at /usr/local/tmp/crew/git.20241009021715.dir/strbuf.c:75:6,
>     inlined from ‘bundle_uri_parse_line’ at /usr/local/tmp/crew/git.20241009021715.dir/bundle-uri.c:939:2,
>     inlined from ‘get_remote_bundle_uri’ at /usr/local/tmp/crew/git.20241009021715.dir/connect.c:527:8,
>     inlined from ‘get_bundle_uri’ at /usr/local/tmp/crew/git.20241009021715.dir/transport.c:406:9:
> /usr/local/tmp/crew/git.20241009021715.dir/strbuf.c:78:17: warning: ‘free’ called on unallocated object ‘strbuf_slopbuf’ [-Wfree-nonheap-object]
>    78 |                 free(sb->buf);
>       |                 ^
> /usr/local/tmp/crew/git.20241009021715.dir/strbuf.c: In function ‘get_bundle_uri’:
> /usr/local/tmp/crew/git.20241009021715.dir/strbuf.c:65:6: note: declared here
>    65 | char strbuf_slopbuf[1];
>       |      ^
> ninja: build stopped: subcommand failed.
> There was a build error.

> [System Info]
> git version:
> git version 2.46.1.GIT
> cpu: x86_64
> no commit associated with this build
> sizeof-long: 8
> sizeof-size_t: 8
> shell-path: /bin/sh
> libcurl: 8.10.0
> zlib: 1.3.1
> uname: Linux 6.4.0-1mx-ahs-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.4.4-1~mx23+1 (2023-07-26) x86_64
> compiler info: gnuc: 14.2
> libc info: glibc: 2.27
> $SHELL (typically, interactive shell): /bin/bash

[PATCH 0/3] cmake: fix autogenerated clar headers

[Patrick Steinhardt]

Hi,

this small patch series fixes how we set up autogenerated clar headers
in CMake such that the clar builds again. The underlying issue is that
we accidentally added the source directory, not the build directory, to
our list of include directories. I guess this went unnoticed because if
you also happen to use Makefiles in the same repo, we'd have found the
Makefile-generated headers by accident.

The other two patches unify the infra to generate "clar-decls.h", which
has basically been pulled out of my build system modernization patch
series.

Taylor: note that this conflicts with the patch series at [1]. The
conflict can be solved by accepting the Makefile of this series here and
adding the following on top of "t/unit-tests/generate-clar-decls.sh":

diff --cc Makefile
index 87b86c5be1,0101d349f3..0000000000
--- a/Makefile
+++ b/Makefile
diff --git a/t/unit-tests/generate-clar-decls.sh b/t/unit-tests/generate-clar-decls.sh
index 6646a90f71..49c61d29b3 100755
--- a/t/unit-tests/generate-clar-decls.sh
+++ b/t/unit-tests/generate-clar-decls.sh
@@ -13,6 +13,6 @@ while test "$#" -ne 0
 do
 	suite="$1"
 	shift
-	sed -ne "s/^\(void test_$suite__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$\)/extern \1;/p" "$suite" ||
+	sed -ne "s/^\(void test_$suite__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)\)$/extern \1;/p" "$suite" ||
 	exit 1
 done >"$OUTPUT"

Thanks!

Patrick

[1]: <cover.1728903464.git.ps@pks.im>

Patrick Steinhardt (3):
  Makefile: extract script to generate clar declarations
  cmake: fix compilation of clar-based unit tests
  cmake: set up proper dependencies for generated clar headers

 Makefile                            |  4 +--
 contrib/buildsystems/CMakeLists.txt | 52 ++++++++---------------------
 t/unit-tests/generate-clar-decls.sh | 18 ++++++++++
 3 files changed, 32 insertions(+), 42 deletions(-)
 create mode 100755 t/unit-tests/generate-clar-decls.sh

-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH 1/3] Makefile: extract script to generate clar declarations

[Patrick Steinhardt]

Extract the script to generate function declarations for the clar unit
testing framework into a standalone script. This is done such that we
can reuse it in other build systems.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Makefile                            |  4 +---
 t/unit-tests/generate-clar-decls.sh | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)
 create mode 100755 t/unit-tests/generate-clar-decls.sh

diff --git a/Makefile b/Makefile
index feeed6f9321..87b86c5be1a 100644
--- a/Makefile
+++ b/Makefile
@@ -3904,9 +3904,7 @@ GIT-TEST-SUITES: FORCE
             fi
 
 $(UNIT_TEST_DIR)/clar-decls.h: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUITES)) GIT-TEST-SUITES
-	$(QUIET_GEN)for suite in $(CLAR_TEST_SUITES); do \
-		sed -ne "s/^\(void test_$${suite}__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$$\)/extern \1;/p" $(UNIT_TEST_DIR)/$$suite.c; \
-	done >$@
+	$(QUIET_GEN)$(SHELL_PATH) $(UNIT_TEST_DIR)/generate-clar-decls.sh "$@" $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUITES))
 $(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h
 	$(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/clar-generate.awk $< >$(UNIT_TEST_DIR)/clar.suite
 $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h
diff --git a/t/unit-tests/generate-clar-decls.sh b/t/unit-tests/generate-clar-decls.sh
new file mode 100755
index 00000000000..6646a90f711
--- /dev/null
+++ b/t/unit-tests/generate-clar-decls.sh
@@ -0,0 +1,18 @@
+#!/bin/sh
+
+if test $# -lt 2
+then
+	echo "USAGE: $0 <OUTPUT> <SUITE>..." 2>&1
+	exit 1
+fi
+
+OUTPUT="$1"
+shift
+
+while test "$#" -ne 0
+do
+	suite="$1"
+	shift
+	sed -ne "s/^\(void test_$suite__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$\)/extern \1;/p" "$suite" ||
+	exit 1
+done >"$OUTPUT"
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH 2/3] cmake: fix compilation of clar-based unit tests

[Patrick Steinhardt]

The compilation of clar-based unit tests is broken because we do not
add the binary directory into which we generate the "clar-decls.h" and
"clar.suite" files as include directories. Instead, we accidentally set
up the source directory as include directory.

Fix this and propagate the include directories of "unit-tests.lib" to
the "unit-tests" executable so that the latter uses the same include
directories.

Reported-by: Ed Reel <edreel@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 contrib/buildsystems/CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 62af7b33d2f..093852ad9d6 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1042,7 +1042,7 @@ file(WRITE "${CMAKE_BINARY_DIR}/t/unit-tests/clar.suite" "${clar_decls}" "${clar
 list(TRANSFORM clar_test_SUITES PREPEND "${CMAKE_SOURCE_DIR}/t/unit-tests/")
 list(TRANSFORM clar_test_SUITES APPEND ".c")
 add_library(unit-tests-lib ${clar_test_SUITES} "${CMAKE_SOURCE_DIR}/t/unit-tests/clar/clar.c")
-target_include_directories(unit-tests-lib PRIVATE "${CMAKE_SOURCE_DIR}/t/unit-tests")
+target_include_directories(unit-tests-lib PUBLIC "${CMAKE_BINARY_DIR}/t/unit-tests")
 add_executable(unit-tests "${CMAKE_SOURCE_DIR}/t/unit-tests/unit-test.c")
 target_link_libraries(unit-tests unit-tests-lib common-main)
 set_target_properties(unit-tests
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH 3/3] cmake: set up proper dependencies for generated clar headers

[Patrick Steinhardt]

The auto-generated headers used by clar are written at configure time
and thus do not get regenerated automatically. Refactor the build
recipes such that we use custom commands instead, which also has the
benefit that we can reuse the same infrastructure as our Makefile.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 contrib/buildsystems/CMakeLists.txt | 50 +++++++----------------------
 1 file changed, 12 insertions(+), 38 deletions(-)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 093852ad9d6..9f80ab92656 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1002,46 +1002,20 @@ foreach(unit_test ${unit_test_PROGRAMS})
 endforeach()
 
 parse_makefile_for_scripts(clar_test_SUITES "CLAR_TEST_SUITES" "")
-
-set(clar_decls "")
-set(clar_cbs "")
-set(clar_cbs_count 0)
-set(clar_suites "static struct clar_suite _clar_suites[] = {\n")
-list(LENGTH clar_test_SUITES clar_suites_count)
-foreach(suite ${clar_test_SUITES})
-	file(STRINGS "${CMAKE_SOURCE_DIR}/t/unit-tests/${suite}.c" decls
-		REGEX "^void test_${suite}__[a-zA-Z_0-9][a-zA-Z_0-9]*\\(void\\)$")
-
-	list(LENGTH decls decls_count)
-	string(REGEX REPLACE "void (test_${suite}__([a-zA-Z_0-9]*))\\(void\\)" "    { \"\\2\", &\\1 },\n" cbs ${decls})
-	string(JOIN "" cbs ${cbs})
-	list(TRANSFORM decls PREPEND "extern ")
-	string(JOIN ";\n" decls ${decls})
-
-	string(APPEND clar_decls "${decls};\n")
-	string(APPEND clar_cbs
-		"static const struct clar_func _clar_cb_${suite}[] = {\n"
-		${cbs}
-		"};\n")
-	string(APPEND clar_suites
-		"    {\n"
-		"        \"${suite}\",\n"
-		"        { NULL, NULL },\n"
-		"        { NULL, NULL },\n"
-		"        _clar_cb_${suite}, ${decls_count}, 1\n"
-		"    },\n")
-	math(EXPR clar_cbs_count "${clar_cbs_count}+${decls_count}")
-endforeach()
-string(APPEND clar_suites
-	"};\n"
-	"static const size_t _clar_suite_count = ${clar_suites_count};\n"
-	"static const size_t _clar_callback_count = ${clar_cbs_count};\n")
-file(WRITE "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h" "${clar_decls}")
-file(WRITE "${CMAKE_BINARY_DIR}/t/unit-tests/clar.suite" "${clar_decls}" "${clar_cbs}" "${clar_suites}")
-
 list(TRANSFORM clar_test_SUITES PREPEND "${CMAKE_SOURCE_DIR}/t/unit-tests/")
 list(TRANSFORM clar_test_SUITES APPEND ".c")
-add_library(unit-tests-lib ${clar_test_SUITES} "${CMAKE_SOURCE_DIR}/t/unit-tests/clar/clar.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})
+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")
+
+add_library(unit-tests-lib ${clar_test_SUITES}
+	"${CMAKE_SOURCE_DIR}/t/unit-tests/clar/clar.c"
+	"${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h"
+	"${CMAKE_BINARY_DIR}/t/unit-tests/clar.suite"
+)
 target_include_directories(unit-tests-lib PUBLIC "${CMAKE_BINARY_DIR}/t/unit-tests")
 add_executable(unit-tests "${CMAKE_SOURCE_DIR}/t/unit-tests/unit-test.c")
 target_link_libraries(unit-tests unit-tests-lib common-main)
-- 
2.47.0.72.gef8ce8f3d4.dirty

Re: [PATCH 0/3] cmake: fix autogenerated clar headers

[Taylor Blau]

On Mon, Oct 14, 2024 at 04:06:38PM +0200, Patrick Steinhardt wrote:
> Hi,
>
> this small patch series fixes how we set up autogenerated clar headers
> in CMake such that the clar builds again. The underlying issue is that
> we accidentally added the source directory, not the build directory, to
> our list of include directories. I guess this went unnoticed because if
> you also happen to use Makefiles in the same repo, we'd have found the
> Makefile-generated headers by accident.
>
> The other two patches unify the infra to generate "clar-decls.h", which
> has basically been pulled out of my build system modernization patch
> series.
>
> Taylor: note that this conflicts with the patch series at [1]. The
> conflict can be solved by accepting the Makefile of this series here and
> adding the following on top of "t/unit-tests/generate-clar-decls.sh":

Thanks. I had initially kicked the latter out of 'seen' when applying
both to my tree. But when I went back and read the contents of these
patches more carefully I saw your note here, which is very much
appreciated.

Thanks,
Taylor

Re: [PATCH 1/3] Makefile: extract script to generate clar declarations

[Taylor Blau]

On Mon, Oct 14, 2024 at 04:06:41PM +0200, Patrick Steinhardt wrote:
> Extract the script to generate function declarations for the clar unit
> testing framework into a standalone script. This is done such that we
> can reuse it in other build systems.

Makes sense.

> +while test "$#" -ne 0
> +do
> +	suite="$1"
> +	shift

I'm perhaps nit-picking here, but I find:

    for suite in "$@"
    do
        ...
    done

to be much more readable than 'while test "$#" -ne 0' and 'shift'
above, since the for-loop variant does not need to mangle its arguments
with shift.

> +	sed -ne "s/^\(void test_$suite__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$\)/extern \1;/p" "$suite" ||

This part is a faithful translation of the original Makefile. Looking
good.

Thanks,
Taylor

Re: [PATCH 2/3] cmake: fix compilation of clar-based unit tests

[Taylor Blau]

On Mon, Oct 14, 2024 at 04:06:44PM +0200, Patrick Steinhardt wrote:
> The compilation of clar-based unit tests is broken because we do not
> add the binary directory into which we generate the "clar-decls.h" and
> "clar.suite" files as include directories. Instead, we accidentally set
> up the source directory as include directory.

I am confused. What is the difference between CMAKE_SOURCE_DIR and
CMAKE_BINARY_DIR here, and why does the difference between the two
matter?

> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 62af7b33d2f..093852ad9d6 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1042,7 +1042,7 @@ file(WRITE "${CMAKE_BINARY_DIR}/t/unit-tests/clar.suite" "${clar_decls}" "${clar
>  list(TRANSFORM clar_test_SUITES PREPEND "${CMAKE_SOURCE_DIR}/t/unit-tests/")
>  list(TRANSFORM clar_test_SUITES APPEND ".c")
>  add_library(unit-tests-lib ${clar_test_SUITES} "${CMAKE_SOURCE_DIR}/t/unit-tests/clar/clar.c")
> -target_include_directories(unit-tests-lib PRIVATE "${CMAKE_SOURCE_DIR}/t/unit-tests")
> +target_include_directories(unit-tests-lib PUBLIC "${CMAKE_BINARY_DIR}/t/unit-tests")

This also changes the 'scope' parameter of 'target_include_directories'
from PRIVATE to PUBLIC, but the commit message doesn't mention such a
change.

Is it intentional? If so, can the commit message be updated to explain
why this is done? If not, is this a stray change that snuck in?

(If all of this is obvious to you, I apologize for the confusion on my
end. I'm not at all familiar with our CMake bits, so the extra
explanation would help me quite a bit in making sense of this.)

Thanks,
Taylor

Re: [PATCH 3/3] cmake: set up proper dependencies for generated clar headers

[Taylor Blau]

On Mon, Oct 14, 2024 at 04:06:47PM +0200, Patrick Steinhardt wrote:
> The auto-generated headers used by clar are written at configure time
> and thus do not get regenerated automatically. Refactor the build
> recipes such that we use custom commands instead, which also has the
> benefit that we can reuse the same infrastructure as our Makefile.

OK.

> +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})
> +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")
> +
> +add_library(unit-tests-lib ${clar_test_SUITES}
> +	"${CMAKE_SOURCE_DIR}/t/unit-tests/clar/clar.c"
> +	"${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h"
> +	"${CMAKE_BINARY_DIR}/t/unit-tests/clar.suite"
> +)
>  target_include_directories(unit-tests-lib PUBLIC "${CMAKE_BINARY_DIR}/t/unit-tests")
>  add_executable(unit-tests "${CMAKE_SOURCE_DIR}/t/unit-tests/unit-test.c")
>  target_link_libraries(unit-tests unit-tests-lib common-main)

Ahh. The post-image is a definite improvement ;-).

Thanks,
Taylor

Re: [PATCH 1/3] Makefile: extract script to generate clar declarations

[Patrick Steinhardt]

On Mon, Oct 14, 2024 at 05:42:39PM -0400, Taylor Blau wrote:
> On Mon, Oct 14, 2024 at 04:06:41PM +0200, Patrick Steinhardt wrote:
> > +while test "$#" -ne 0
> > +do
> > +	suite="$1"
> > +	shift
> 
> I'm perhaps nit-picking here, but I find:
> 
>     for suite in "$@"
>     do
>         ...
>     done
> 
> to be much more readable than 'while test "$#" -ne 0' and 'shift'
> above, since the for-loop variant does not need to mangle its arguments
> with shift.

It certainly is, yeah. I know that there was a reason to write it in
this convoluted way... was it compatibility with non-Bash shells when
there were spaces in the paths? I don't know, and I cannot find a case
right now where it breaks.

Will adapt accordingly.

Patrick

Re: [PATCH 2/3] cmake: fix compilation of clar-based unit tests

[Patrick Steinhardt]

On Mon, Oct 14, 2024 at 05:46:30PM -0400, Taylor Blau wrote:
> On Mon, Oct 14, 2024 at 04:06:44PM +0200, Patrick Steinhardt wrote:
> > The compilation of clar-based unit tests is broken because we do not
> > add the binary directory into which we generate the "clar-decls.h" and
> > "clar.suite" files as include directories. Instead, we accidentally set
> > up the source directory as include directory.
> 
> I am confused. What is the difference between CMAKE_SOURCE_DIR and
> CMAKE_BINARY_DIR here, and why does the difference between the two
> matter?

This is for out-of-tree builds. The outputs generated by CMake are
written into the binary directory, which is not the source directory
where the Git source files are stored.

> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> > index 62af7b33d2f..093852ad9d6 100644
> > --- a/contrib/buildsystems/CMakeLists.txt
> > +++ b/contrib/buildsystems/CMakeLists.txt
> > @@ -1042,7 +1042,7 @@ file(WRITE "${CMAKE_BINARY_DIR}/t/unit-tests/clar.suite" "${clar_decls}" "${clar
> >  list(TRANSFORM clar_test_SUITES PREPEND "${CMAKE_SOURCE_DIR}/t/unit-tests/")
> >  list(TRANSFORM clar_test_SUITES APPEND ".c")
> >  add_library(unit-tests-lib ${clar_test_SUITES} "${CMAKE_SOURCE_DIR}/t/unit-tests/clar/clar.c")
> > -target_include_directories(unit-tests-lib PRIVATE "${CMAKE_SOURCE_DIR}/t/unit-tests")
> > +target_include_directories(unit-tests-lib PUBLIC "${CMAKE_BINARY_DIR}/t/unit-tests")
> 
> This also changes the 'scope' parameter of 'target_include_directories'
> from PRIVATE to PUBLIC, but the commit message doesn't mention such a
> change.

It does mention it, it's the "propagate the include directories" part.

> Is it intentional? If so, can the commit message be updated to explain
> why this is done? If not, is this a stray change that snuck in?
> 
> (If all of this is obvious to you, I apologize for the confusion on my
> end. I'm not at all familiar with our CMake bits, so the extra
> explanation would help me quite a bit in making sense of this.)

That's fair. I'll clarify the message a bit to provide more context.

Patrick

[PATCH v2 0/3] cmake: fix autogenerated clar headers

[Patrick Steinhardt]

Hi,

this is the second version of this patch series that fixes building clar
with the CMake build system.

Changes compared to v1:

  - Use a for loop instead of `while; shift`.

  - Provide a bit more context around the PRIVATE/PUBLIC switch in the
    commit message.

Thanks!

Patrick

Patrick Steinhardt (3):
  Makefile: extract script to generate clar declarations
  cmake: fix compilation of clar-based unit tests
  cmake: set up proper dependencies for generated clar headers

 Makefile                            |  4 +--
 contrib/buildsystems/CMakeLists.txt | 52 ++++++++---------------------
 t/unit-tests/generate-clar-decls.sh | 16 +++++++++
 3 files changed, 30 insertions(+), 42 deletions(-)
 create mode 100755 t/unit-tests/generate-clar-decls.sh

Range-diff against v1:
1:  346aa2f0830 ! 1:  7a619677c7a Makefile: extract script to generate clar declarations
    @@ t/unit-tests/generate-clar-decls.sh (new)
     +OUTPUT="$1"
     +shift
     +
    -+while test "$#" -ne 0
    ++for suite in "$@"
     +do
    -+	suite="$1"
    -+	shift
     +	sed -ne "s/^\(void test_$suite__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$\)/extern \1;/p" "$suite" ||
     +	exit 1
     +done >"$OUTPUT"
2:  b9afeffda29 ! 2:  447afc4a0f3 cmake: fix compilation of clar-based unit tests
    @@ Commit message
         "clar.suite" files as include directories. Instead, we accidentally set
         up the source directory as include directory.
     
    -    Fix this and propagate the include directories of "unit-tests.lib" to
    -    the "unit-tests" executable so that the latter uses the same include
    -    directories.
    +    Fix this by including the binary directory instead of the source
    +    directory. Furthermore, set up the include directories as PUBLIC instead
    +    of PRIVATE such that they propagate from "unit-tests.lib" to the
    +    "unit-tests" executable, which needs to include the same directory.
     
         Reported-by: Ed Reel <edreel@gmail.com>
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
3:  129d28ae48a = 3:  cf4955b2ddc cmake: set up proper dependencies for generated clar headers
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 1/3] Makefile: extract script to generate clar declarations

[Patrick Steinhardt]

Extract the script to generate function declarations for the clar unit
testing framework into a standalone script. This is done such that we
can reuse it in other build systems.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Makefile                            |  4 +---
 t/unit-tests/generate-clar-decls.sh | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 3 deletions(-)
 create mode 100755 t/unit-tests/generate-clar-decls.sh

diff --git a/Makefile b/Makefile
index feeed6f9321..87b86c5be1a 100644
--- a/Makefile
+++ b/Makefile
@@ -3904,9 +3904,7 @@ GIT-TEST-SUITES: FORCE
             fi
 
 $(UNIT_TEST_DIR)/clar-decls.h: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUITES)) GIT-TEST-SUITES
-	$(QUIET_GEN)for suite in $(CLAR_TEST_SUITES); do \
-		sed -ne "s/^\(void test_$${suite}__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$$\)/extern \1;/p" $(UNIT_TEST_DIR)/$$suite.c; \
-	done >$@
+	$(QUIET_GEN)$(SHELL_PATH) $(UNIT_TEST_DIR)/generate-clar-decls.sh "$@" $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUITES))
 $(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h
 	$(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/clar-generate.awk $< >$(UNIT_TEST_DIR)/clar.suite
 $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h
diff --git a/t/unit-tests/generate-clar-decls.sh b/t/unit-tests/generate-clar-decls.sh
new file mode 100755
index 00000000000..81da732917a
--- /dev/null
+++ b/t/unit-tests/generate-clar-decls.sh
@@ -0,0 +1,16 @@
+#!/bin/sh
+
+if test $# -lt 2
+then
+	echo "USAGE: $0 <OUTPUT> <SUITE>..." 2>&1
+	exit 1
+fi
+
+OUTPUT="$1"
+shift
+
+for suite in "$@"
+do
+	sed -ne "s/^\(void test_$suite__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$\)/extern \1;/p" "$suite" ||
+	exit 1
+done >"$OUTPUT"
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 2/3] cmake: fix compilation of clar-based unit tests

[Patrick Steinhardt]

The compilation of clar-based unit tests is broken because we do not
add the binary directory into which we generate the "clar-decls.h" and
"clar.suite" files as include directories. Instead, we accidentally set
up the source directory as include directory.

Fix this by including the binary directory instead of the source
directory. Furthermore, set up the include directories as PUBLIC instead
of PRIVATE such that they propagate from "unit-tests.lib" to the
"unit-tests" executable, which needs to include the same directory.

Reported-by: Ed Reel <edreel@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 contrib/buildsystems/CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 62af7b33d2f..093852ad9d6 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1042,7 +1042,7 @@ file(WRITE "${CMAKE_BINARY_DIR}/t/unit-tests/clar.suite" "${clar_decls}" "${clar
 list(TRANSFORM clar_test_SUITES PREPEND "${CMAKE_SOURCE_DIR}/t/unit-tests/")
 list(TRANSFORM clar_test_SUITES APPEND ".c")
 add_library(unit-tests-lib ${clar_test_SUITES} "${CMAKE_SOURCE_DIR}/t/unit-tests/clar/clar.c")
-target_include_directories(unit-tests-lib PRIVATE "${CMAKE_SOURCE_DIR}/t/unit-tests")
+target_include_directories(unit-tests-lib PUBLIC "${CMAKE_BINARY_DIR}/t/unit-tests")
 add_executable(unit-tests "${CMAKE_SOURCE_DIR}/t/unit-tests/unit-test.c")
 target_link_libraries(unit-tests unit-tests-lib common-main)
 set_target_properties(unit-tests
-- 
2.47.0.72.gef8ce8f3d4.dirty

[PATCH v2 3/3] cmake: set up proper dependencies for generated clar headers

[Patrick Steinhardt]

The auto-generated headers used by clar are written at configure time
and thus do not get regenerated automatically. Refactor the build
recipes such that we use custom commands instead, which also has the
benefit that we can reuse the same infrastructure as our Makefile.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 contrib/buildsystems/CMakeLists.txt | 50 +++++++----------------------
 1 file changed, 12 insertions(+), 38 deletions(-)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 093852ad9d6..9f80ab92656 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1002,46 +1002,20 @@ foreach(unit_test ${unit_test_PROGRAMS})
 endforeach()
 
 parse_makefile_for_scripts(clar_test_SUITES "CLAR_TEST_SUITES" "")
-
-set(clar_decls "")
-set(clar_cbs "")
-set(clar_cbs_count 0)
-set(clar_suites "static struct clar_suite _clar_suites[] = {\n")
-list(LENGTH clar_test_SUITES clar_suites_count)
-foreach(suite ${clar_test_SUITES})
-	file(STRINGS "${CMAKE_SOURCE_DIR}/t/unit-tests/${suite}.c" decls
-		REGEX "^void test_${suite}__[a-zA-Z_0-9][a-zA-Z_0-9]*\\(void\\)$")
-
-	list(LENGTH decls decls_count)
-	string(REGEX REPLACE "void (test_${suite}__([a-zA-Z_0-9]*))\\(void\\)" "    { \"\\2\", &\\1 },\n" cbs ${decls})
-	string(JOIN "" cbs ${cbs})
-	list(TRANSFORM decls PREPEND "extern ")
-	string(JOIN ";\n" decls ${decls})
-
-	string(APPEND clar_decls "${decls};\n")
-	string(APPEND clar_cbs
-		"static const struct clar_func _clar_cb_${suite}[] = {\n"
-		${cbs}
-		"};\n")
-	string(APPEND clar_suites
-		"    {\n"
-		"        \"${suite}\",\n"
-		"        { NULL, NULL },\n"
-		"        { NULL, NULL },\n"
-		"        _clar_cb_${suite}, ${decls_count}, 1\n"
-		"    },\n")
-	math(EXPR clar_cbs_count "${clar_cbs_count}+${decls_count}")
-endforeach()
-string(APPEND clar_suites
-	"};\n"
-	"static const size_t _clar_suite_count = ${clar_suites_count};\n"
-	"static const size_t _clar_callback_count = ${clar_cbs_count};\n")
-file(WRITE "${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h" "${clar_decls}")
-file(WRITE "${CMAKE_BINARY_DIR}/t/unit-tests/clar.suite" "${clar_decls}" "${clar_cbs}" "${clar_suites}")
-
 list(TRANSFORM clar_test_SUITES PREPEND "${CMAKE_SOURCE_DIR}/t/unit-tests/")
 list(TRANSFORM clar_test_SUITES APPEND ".c")
-add_library(unit-tests-lib ${clar_test_SUITES} "${CMAKE_SOURCE_DIR}/t/unit-tests/clar/clar.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})
+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")
+
+add_library(unit-tests-lib ${clar_test_SUITES}
+	"${CMAKE_SOURCE_DIR}/t/unit-tests/clar/clar.c"
+	"${CMAKE_BINARY_DIR}/t/unit-tests/clar-decls.h"
+	"${CMAKE_BINARY_DIR}/t/unit-tests/clar.suite"
+)
 target_include_directories(unit-tests-lib PUBLIC "${CMAKE_BINARY_DIR}/t/unit-tests")
 add_executable(unit-tests "${CMAKE_SOURCE_DIR}/t/unit-tests/unit-test.c")
 target_link_libraries(unit-tests unit-tests-lib common-main)
-- 
2.47.0.72.gef8ce8f3d4.dirty

Re: [PATCH v2 1/3] Makefile: extract script to generate clar declarations

[Taylor Blau]

On Tue, Oct 15, 2024 at 11:46:11AM +0200, Patrick Steinhardt wrote:
> +for suite in "$@"
> +do
> +	sed -ne "s/^\(void test_$suite__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$\)/extern \1;/p" "$suite" ||
> +	exit 1
> +done >"$OUTPUT"

Much cleaner, thanks.

> 2.47.0.72.gef8ce8f3d4.dirty

Thanks,
Taylor

Re: [PATCH v2 0/3] cmake: fix autogenerated clar headers

[Taylor Blau]

On Tue, Oct 15, 2024 at 11:46:08AM +0200, Patrick Steinhardt wrote:
> Patrick Steinhardt (3):
>   Makefile: extract script to generate clar declarations
>   cmake: fix compilation of clar-based unit tests
>   cmake: set up proper dependencies for generated clar headers

Thanks; will queue.

Thanks,
Taylor

Re: [PATCH v2 1/3] Makefile: extract script to generate clar declarations

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

> Extract the script to generate function declarations for the clar unit
> testing framework into a standalone script. This is done such that we
> can reuse it in other build systems.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  Makefile                            |  4 +---
>  t/unit-tests/generate-clar-decls.sh | 16 ++++++++++++++++
>  2 files changed, 17 insertions(+), 3 deletions(-)
>  create mode 100755 t/unit-tests/generate-clar-decls.sh
>
> diff --git a/Makefile b/Makefile
> index feeed6f9321..87b86c5be1a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3904,9 +3904,7 @@ GIT-TEST-SUITES: FORCE
>              fi
>  
>  $(UNIT_TEST_DIR)/clar-decls.h: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUITES)) GIT-TEST-SUITES
> -	$(QUIET_GEN)for suite in $(CLAR_TEST_SUITES); do \
> -		sed -ne "s/^\(void test_$${suite}__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$$\)/extern \1;/p" $(UNIT_TEST_DIR)/$$suite.c; \
> -	done >$@
> +	$(QUIET_GEN)$(SHELL_PATH) $(UNIT_TEST_DIR)/generate-clar-decls.sh "$@" $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUITES))
>  $(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h
>  	$(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/clar-generate.awk $< >$(UNIT_TEST_DIR)/clar.suite
>  $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h
> diff --git a/t/unit-tests/generate-clar-decls.sh b/t/unit-tests/generate-clar-decls.sh
> new file mode 100755
> index 00000000000..81da732917a
> --- /dev/null
> +++ b/t/unit-tests/generate-clar-decls.sh
> @@ -0,0 +1,16 @@
> +#!/bin/sh
> +
> +if test $# -lt 2
> +then
> +	echo "USAGE: $0 <OUTPUT> <SUITE>..." 2>&1
> +	exit 1
> +fi
> +
> +OUTPUT="$1"
> +shift
> +
> +for suite in "$@"
> +do
> +	sed -ne "s/^\(void test_$suite__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$\)/extern \1;/p" "$suite" ||

In the Makefile the first `suite` was wrapped in curly braces. And I
think we need to keep them in this script as well. I noticed because I
was reviewing this code in my editor I've noticed it highlights
"source__" as the variable name. You can see what happens if you add
`set -x` to the top of the script:

    $ make t/unit-tests/clar-decls.h V=1
    /bin/sh t/unit-tests/generate-clar-decls.sh "t/unit-tests/clar-decls.h" t/unit-tests/ctype.c t/unit-tests/strvec.c
    + test 3 -lt 2
    + OUTPUT=t/unit-tests/clar-decls.h
    + shift
    + for suite in "$@"
    + sed -ne 's/^\(void test_[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$\)/extern \1;/p' t/unit-tests/ctype.c
    + for suite in "$@"
    + sed -ne 's/^\(void test_[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$\)/extern \1;/p' t/unit-tests/strvec.c

So it seems the script currently works "by accident".

You should replace the first $suite with something like:

    $(basename $suite .c)

One other suggestion, and feel free to disagree. What do you think about
replacing the `$(patsubst ...)` in the recipe to `$(filter %.c,$^)`?

--
Toon

Re: [PATCH v2 1/3] Makefile: extract script to generate clar declarations

[Patrick Steinhardt]

On Fri, Oct 18, 2024 at 05:21:17PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/t/unit-tests/generate-clar-decls.sh b/t/unit-tests/generate-clar-decls.sh
> > new file mode 100755
> > index 00000000000..81da732917a
> > --- /dev/null
> > +++ b/t/unit-tests/generate-clar-decls.sh
> > @@ -0,0 +1,16 @@
> > +#!/bin/sh
> > +
> > +if test $# -lt 2
> > +then
> > +	echo "USAGE: $0 <OUTPUT> <SUITE>..." 2>&1
> > +	exit 1
> > +fi
> > +
> > +OUTPUT="$1"
> > +shift
> > +
> > +for suite in "$@"
> > +do
> > +	sed -ne "s/^\(void test_$suite__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$\)/extern \1;/p" "$suite" ||
> 
> In the Makefile the first `suite` was wrapped in curly braces. And I
> think we need to keep them in this script as well. I noticed because I
> was reviewing this code in my editor I've noticed it highlights
> "source__" as the variable name. You can see what happens if you add
> `set -x` to the top of the script:
> 
>     $ make t/unit-tests/clar-decls.h V=1
>     /bin/sh t/unit-tests/generate-clar-decls.sh "t/unit-tests/clar-decls.h" t/unit-tests/ctype.c t/unit-tests/strvec.c
>     + test 3 -lt 2
>     + OUTPUT=t/unit-tests/clar-decls.h
>     + shift
>     + for suite in "$@"
>     + sed -ne 's/^\(void test_[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$\)/extern \1;/p' t/unit-tests/ctype.c
>     + for suite in "$@"
>     + sed -ne 's/^\(void test_[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$\)/extern \1;/p' t/unit-tests/strvec.c
> 
> So it seems the script currently works "by accident".

Oh, indeed.

> You should replace the first $suite with something like:
> 
>     $(basename $suite .c)
> 
> One other suggestion, and feel free to disagree. What do you think about
> replacing the `$(patsubst ...)` in the recipe to `$(filter %.c,$^)`?

Yup, both of these are good suggestions.

Thanks!

Patrick