Re: [Tarantool-patches] [PATCH luajit 3/7] cmake: introduce AppendTestEnvVar macro

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the review!
See my answers below.

On 23.09.24, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for the patch! See comments below.
> 
> On 23.09.2024 10:18, Sergey Kaplun wrote:
> > It is useful to update the environment variable for some tests. For
> > CMake versions >= 3.22, we can use ENVIRONMENT_MODIFICATION [1] instead.
> > But unless we bump the CMake version, this macro is a workaround.
> >
> > [1]:https://cmake.org/cmake/help/latest/prop_test/ENVIRONMENT_MODIFICATION.html
> >
> > Part of tarantool/tarantool#9898
> > ---
> >   test/tarantool-tests/CMakeLists.txt | 23 +++++++++++++++++++++++
> >   1 file changed, 23 insertions(+)
> >
> > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> > index 11a84496..4530c9fd 100644
> > --- a/test/tarantool-tests/CMakeLists.txt
> > +++ b/test/tarantool-tests/CMakeLists.txt
> > @@ -18,6 +18,29 @@ macro(BuildTestCLib lib sources)
> >     set(LD_LIBRARY_PATH "${CMAKE_CURRENT_BINARY_DIR}:${LD_LIBRARY_PATH}" PARENT_SCOPE)
> >   endmacro()
> >   
> > +# FIXME: This is used only due to ancient CMake requirements.
> > +# If we update to CMake >= 3.22, we can use
> > +# ENVIRONMENT_MODIFICATION [1] instead.
> > +# [1]:https://cmake.org/cmake/help/latest/prop_test/ENVIRONMENT_MODIFICATION.html
> I would add a small description for a macro.

Added the description. Branch is force-pushed.
===================================================================
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index 4530c9fd..a5f3efcd 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -22,6 +22,9 @@ endmacro()
 # If we update to CMake >= 3.22, we can use
 # ENVIRONMENT_MODIFICATION [1] instead.
 # [1]: https://cmake.org/cmake/help/latest/prop_test/ENVIRONMENT_MODIFICATION.html
+# This macro appends to the given test's environment variable the
+# given value. If the variable doesn't exist, it is created equal
+# to the given argument.
 macro(AppendTestEnvVar testname var value)
   get_test_property(${testname} ENVIRONMENT old_env)
   foreach(loopvar "${old_env}")
===================================================================

> > +macro(AppendTestEnvVar testname var value)
> 
> I would rename macro to something like "ModifyTestEnv"
> 
> because it appends env var when variable with the same name was not found
> 
> and modifies it when variable exist.

I prefer to leave AppendTestEnvVar (since "ModifyTestEnv" lacks the
description of the Append part) and clarify its behaviour with a
comment.

> 
> 

<snipped>

> >   add_subdirectory(gh-4427-ffi-sandwich)

-- 
Best regards,
Sergey Kaplun