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

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 15, 2024, 09:09 UTC
Message-ID: <Zw4xNwnrLicx2nDa@pks.im>
In-Reply-To: <Zw2RNsptlkHCyTgi@nand.local>

On Mon, Oct 14, 2024 at 05:46:30PM -0400, Taylor Blau wrote:

Show 9 quoted lines

> 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.

Show 14 quoted lines

> > 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.

Show 6 quoted lines

> 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