tests/subsys/settings/functional: make common code a zephyr_library() #19531

marc-hb · 2019-10-01T21:54:54Z

Stops leaking very long source paths in build directories; makes them
deterministic.

Besides satisfying a CMake requirement, the new empty_file.c provide a
clue that the actual test code is not in the directory of the test case.

See zephyrproject-rtos/hal_nordic#6 and
https://gitlab.kitware.com/cmake/cmake/issues/19475 for more details.

Test with a simple:

sanitycheck -T $ZEPHYR_BASE/tests/subsys/settings/functional/

Before:

CMakeFiles
├── app.dir
│   ├── HOME
│   │   └── JOHN
│   │       └── zephyrproject
│   │           └── zephyr
│   │               └── tests
│   │                   └── subsys
│   │                       └── settings
│   │                           └── functional
│   │                               └── src
│   │                                   └── settings_basic_test.c.obj

After:

func_test_bindir/
├── CMakeFiles
│   └── settings_func_test.dir
│       └── settings_basic_test.c.obj
│
├── libsettings_func_test.a

Signed-off-by: Marc Herbert marc.herbert@intel.com

zephyrbot · 2019-10-01T22:01:49Z

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

marc-hb · 2019-10-01T23:35:01Z

This fixes only tests/subsys/settings/functional and nothing else in tests/subsys/settings.

I have a bigger, WIP series in my workspace that fixes everything in tests/subsys/settings/ with the same approach. However the (CMake) structure of tests/subsys/settings/ is a maze that takes me at least 1/2h to wrap my head around every time. To avoid losing reviewers too in this maze (and duplication) let's focus on this much smaller, self-contained subset first. Will replicate the same approach elsewhere if/when this is merged.

For a complete record, this issue was also mentioned briefly in PR #18891

mbolivar · 2019-10-01T23:40:21Z

tests/subsys/settings/functional/src/CMakeLists.txt

+
+zephyr_library_named(  settings_func_test)
+zephyr_library_sources(settings_basic_test.c)
+add_dependencies(      settings_func_test offsets_h)


Why is this dependency required?

I see this in the top level zephyr/CMakeLists.txt:

foreach(zephyr_lib ${ZEPHYR_LIBS_PROPERTY}) # TODO: Could this become an INTERFACE property of zephyr_interface? add_dependencies(${zephyr_lib} ${OFFSETS_H_TARGET}) endforeach()

So it looks like zephyr_library_named() should be taking care of this:

macro(zephyr_library_named name) [...] set(ZEPHYR_CURRENT_LIBRARY ${name}) add_library(${name} STATIC "") zephyr_append_cmake_library(${name}) [...] endmacro() [...] function(zephyr_append_cmake_library library) set_property(GLOBAL APPEND PROPERTY ZEPHYR_LIBS ${library}) endfunction()

I'm not sure why this explicit dependency is needed. It was suggested by @SebastianBoe . See below how make -C build [settings_func_test] always fails without it.

Note: make -j sometimes succeeds in building even without this dependency thanks to a race condition. make -k; make obviously succeeds too.

make: Entering directory 'zephyr/tests/subsys/settings/functional/nvs/build' /usr/bin/cmake -Szephyr/tests/subsys/settings/functional/nvs -Bzephyr/tests/subsys/settings/functional/nvs/build --check-build-system CMakeFiles/Makefile.cmake 0 /usr/bin/cmake -E cmake_progress_start zephyr/tests/subsys/settings/functional/nvs/build/CMakeFiles zephyr/tests/subsys/settings/functional/nvs/build/CMakeFiles/progress.marks make -f CMakeFiles/Makefile2 all make[1]: Entering directory 'zephyr/tests/subsys/settings/functional/nvs/build' make -f func_test_bindir/CMakeFiles/settings_func_test.dir/build.make func_test_bindir/CMakeFiles/settings_func_test.dir/depend make[2]: Entering directory 'zephyr/tests/subsys/settings/functional/nvs/build' cd zephyr/tests/subsys/settings/functional/nvs/build && /usr/bin/cmake -E cmake_depends "Unix Makefiles" zephyr/tests/subsys/settings/functional/nvs zephyr/tests/subsys/settings/functional/src zephyr/tests/subsys/settings/functional/nvs/build zephyr/tests/subsys/settings/functional/nvs/build/func_test_bindir zephyr/tests/subsys/settings/functional/nvs/build/func_test_bindir/CMakeFiles/settings_func_test.dir/DependInfo.cmake --color= make[2]: Leaving directory 'zephyr/tests/subsys/settings/functional/nvs/build' make -f func_test_bindir/CMakeFiles/settings_func_test.dir/build.make func_test_bindir/CMakeFiles/settings_func_test.dir/build make[2]: Entering directory 'zephyr/tests/subsys/settings/functional/nvs/build' [ 0%] Building C object func_test_bindir/CMakeFiles/settings_func_test.dir/settings_basic_test.c.obj In file included from zephyr/lib/libc/minimal/include/errno.h:20, from zephyr/include/kernel.h:18, from zephyr/include/zephyr.h:18, from zephyr/tests/subsys/settings/functional/src/settings_basic_test.c:12: zephyr/include/sys/errno_private.h:34:10: fatal error: syscalls/errno_private.h: No such file or directory #include <syscalls/errno_private.h> ^~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. make[2]: *** [func_test_bindir/CMakeFiles/settings_func_test.dir/build.make:63: func_test_bindir/CMakeFiles/settings_func_test.dir/settings_basic_test.c.obj] Error 1 make[2]: Leaving directory 'zephyr/tests/subsys/settings/functional/nvs/build' make[1]: *** [CMakeFiles/Makefile2:2972: func_test_bindir/CMakeFiles/settings_func_test.dir/all] Error 2 make[1]: Leaving directory 'zephyr/tests/subsys/settings/functional/nvs/build' make: *** [Makefile:84: all] Error 2 make: Leaving directory 'zephyr/tests/subsys/settings/functional/nvs/build'

That seems to be papering over a bug in the cmake extensions somehow, then. @SebastianBoe any comment?

I guess as a band-aid that makes sense then.

ninja -j1 doesn't fail, it seems "lucky" enough to build in some order that avoids the missing dependency issue.

However ninja settings_func_test fails with the same error than make settings_func_test above.

@mbolivar It seems to be that at the time the foreach loop is executed the library settings_func_test has not been added to the list yet. I think the TODO statement in the foreach loop would fix that issue.

@joerchan has it right. As do you @mbolivar , I'll look into moving the 'add_dependencies' call from the loop in zephyr/CMakeLists.txt to extensions.cmake.

But for now this is best-practice.

a black-magic it is

Any sufficiently obscure code is indistinguishable from magic ...

mbolivar

Thanks for explaining @SebastianBoe @joerchan

nvlsianpu

I would add license and copyright to empty files in order to satisfy CI, also include comment which explains why the empty file is needed.

Stops leaking very long source paths in build directories; makes them deterministic. Besides satisfying a CMake requirement, the new empty_file.c provide a clue that the actual test code is not in the directory of the test case. See zephyrproject-rtos/hal_nordic#6 and https://gitlab.kitware.com/cmake/cmake/issues/19475 for more details. - Test with a simple: sanitycheck -T $ZEPHYR_BASE/tests/subsys/settings/functional/ - Before: CMakeFiles ├── app.dir │ ├── HOME │ │ └── JOHN │ │ └── zephyrproject │ │ └── zephyr │ │ └── tests │ │ └── subsys │ │ └── settings │ │ └── functional │ │ └── src │ │ └── settings_basic_test.c.obj - After: func_test_bindir/ ├── CMakeFiles │ └── settings_func_test.dir │ └── settings_basic_test.c.obj │ ├── libsettings_func_test.a Signed-off-by: Marc Herbert <marc.herbert@intel.com>

Stops leaking very long source paths in build directories; makes them deterministic. This is identical to what was already merged in zephyrproject-rtos/hal_nordic#6 or zephyrproject-rtos/zephyr#19531 see previous code reviews there. See also CMake issue https://gitlab.kitware.com/cmake/cmake/issues/19475 for more details. This is basically a search/replace of "zephyr_sources" with "zephyr_library_sources" For instance, when running sanitycheck -p stm3210c_eval, this changes the build directories from: sample.helloworld/ └── zephyr ├── CMakeFiles │ │ ├── HOME │ │ │ └── JOHN │ │ │ └── ZEPHYRPROJECT │ │ │ └── modules │ │ │ └── hal │ │ │ └── stm32 │ │ │ └── stm32cube │ │ │ └── stm32f1xx │ │ │ ├── drivers │ │ │ │ └── src │ │ │ │ ├── stm32f1xx_hal.c.obj │ │ │ │ ├── stm32f1xx_hal_rcc.c.obj │ │ │ │ └── stm32f1xx_ll_utils.c.obj │ │ │ └── soc │ │ │ └── system_stm32f1xx.c.obj ... to: sample.helloworld/ ├── modules │ ├── stm32 │ │ ├── CMakeFiles │ │ └── stm32cube │ │ ├── CMakeFiles │ │ │ └── ..__modules__hal__stm32__stm32cube.dir │ │ │ └── stm32f1xx │ │ │ ├── drivers │ │ │ │ └── src │ │ │ │ ├── stm32f1xx_hal.c.obj │ │ │ │ ├── stm32f1xx_hal_rcc.c.obj │ │ │ │ └── stm32f1xx_ll_utils.c.obj │ │ │ └── soc │ │ │ └── system_stm32f1xx.c.obj │ │ ├── │ │ ├── lib..__modules__hal__stm32__stm32cube.a Signed-off-by: Marc Herbert <marc.herbert@intel.com>

marc-hb · 2019-10-08T03:09:24Z

I have a bigger, WIP series in my workspace that fixes everything in tests/subsys/settings/ with the same approach. [...] To avoid losing reviewers too in this maze (and duplication) let's focus on this much smaller, self-contained subset first. Will replicate the same approach elsewhere if/when this is merged.

Completed in PR #19671

Stops leaking very long source paths in build directories; makes them deterministic. This is identical to what was already merged in zephyrproject-rtos/hal_nordic#6 or zephyrproject-rtos/zephyr#19531 see previous code reviews there. See also CMake issue https://gitlab.kitware.com/cmake/cmake/issues/19475 for more details. This is basically a search/replace of "zephyr_sources" with "zephyr_library_sources" For instance, when running sanitycheck -p stm3210c_eval, this changes the build directories from: sample.helloworld/ └── zephyr ├── CMakeFiles │ │ ├── HOME │ │ │ └── JOHN │ │ │ └── ZEPHYRPROJECT │ │ │ └── modules │ │ │ └── hal │ │ │ └── stm32 │ │ │ └── stm32cube │ │ │ └── stm32f1xx │ │ │ ├── drivers │ │ │ │ └── src │ │ │ │ ├── stm32f1xx_hal.c.obj │ │ │ │ ├── stm32f1xx_hal_rcc.c.obj │ │ │ │ └── stm32f1xx_ll_utils.c.obj │ │ │ └── soc │ │ │ └── system_stm32f1xx.c.obj ... to: sample.helloworld/ ├── modules │ ├── stm32 │ │ ├── CMakeFiles │ │ └── stm32cube │ │ ├── CMakeFiles │ │ │ └── ..__modules__hal__stm32__stm32cube.dir │ │ │ └── stm32f1xx │ │ │ ├── drivers │ │ │ │ └── src │ │ │ │ ├── stm32f1xx_hal.c.obj │ │ │ │ ├── stm32f1xx_hal_rcc.c.obj │ │ │ │ └── stm32f1xx_ll_utils.c.obj │ │ │ └── soc │ │ │ └── system_stm32f1xx.c.obj │ │ ├── │ │ ├── lib..__modules__hal__stm32__stm32cube.a Signed-off-by: Marc Herbert <marc.herbert@intel.com>

zephyrbot added the area: Tests Issues related to a particular existing or missing test label Oct 1, 2019

marc-hb force-pushed the deterministic-test-settings-functional branch from 5445725 to ca97635 Compare October 1, 2019 22:11

marc-hb requested a review from SebastianBoe October 1, 2019 22:12

marc-hb marked this pull request as ready for review October 1, 2019 23:24

marc-hb requested a review from nvlsianpu as a code owner October 1, 2019 23:24

marc-hb requested review from mbolivar, Vudentz, ozhuraki, anangl, carlescufi, ioannisg, mped-oticon and joerchan October 1, 2019 23:32

mbolivar reviewed Oct 1, 2019

View reviewed changes

marc-hb added area: Build System Enhancement Changes/Updates/Additions to existing features labels Oct 2, 2019

ozhuraki approved these changes Oct 2, 2019

View reviewed changes

SebastianBoe approved these changes Oct 2, 2019

View reviewed changes

mbolivar approved these changes Oct 2, 2019

View reviewed changes

nvlsianpu reviewed Oct 3, 2019

View reviewed changes

marc-hb mentioned this pull request Oct 3, 2019

zephyr_library: missing 'kernel-mode' vs 'app-mode' documentation #19582

Closed

marc-hb force-pushed the deterministic-test-settings-functional branch from ca97635 to df3b926 Compare October 3, 2019 17:45

nvlsianpu approved these changes Oct 3, 2019

View reviewed changes

joerchan approved these changes Oct 5, 2019

View reviewed changes

andrewboie merged commit b4282bf into zephyrproject-rtos:master Oct 7, 2019

marc-hb mentioned this pull request Oct 7, 2019

make stm32cube a zephyr_library() zephyrproject-rtos/hal_stm32#23

Merged

marc-hb deleted the deterministic-test-settings-functional branch October 8, 2019 03:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests/subsys/settings/functional: make common code a zephyr_library() #19531

tests/subsys/settings/functional: make common code a zephyr_library() #19531

marc-hb commented Oct 1, 2019

zephyrbot commented Oct 1, 2019 •

edited

Loading

marc-hb commented Oct 1, 2019

mbolivar Oct 1, 2019

marc-hb Oct 2, 2019 •

edited

Loading

mbolivar Oct 2, 2019

marc-hb Oct 2, 2019

joerchan Oct 2, 2019

SebastianBoe Oct 2, 2019

SebastianBoe Oct 2, 2019

nvlsianpu Oct 3, 2019

SebastianBoe Oct 3, 2019

mbolivar left a comment

nvlsianpu left a comment

marc-hb commented Oct 8, 2019

tests/subsys/settings/functional: make common code a zephyr_library() #19531

tests/subsys/settings/functional: make common code a zephyr_library() #19531

Conversation

marc-hb commented Oct 1, 2019

zephyrbot commented Oct 1, 2019 • edited Loading

marc-hb commented Oct 1, 2019

Choose a reason for hiding this comment

marc-hb Oct 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbolivar left a comment

Choose a reason for hiding this comment

nvlsianpu left a comment

Choose a reason for hiding this comment

marc-hb commented Oct 8, 2019

zephyrbot commented Oct 1, 2019 •

edited

Loading

marc-hb Oct 2, 2019 •

edited

Loading