CMake: Remove mbed-stubs-rtos-headers library #14870

rajkan01 · 2021-07-05T10:31:19Z

Summary of changes

Previous changes moved all rtos stubs headers into mbed-stubs-rtos-headers lib, but the decision to keep all stubs headers under the respective component stubs library so moved all stubs rtos headers under mbed-stubs-rtos and updated it depend component CMake

Impact of changes

None.

Migration actions required

None.

Documentation

None.

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

ciarmcom · 2021-07-05T11:00:13Z

@rajkan01, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

connectivity/cellular/tests/UNITTESTS/framework/AT/at_cellularcontext/CMakeLists.txt

LDong-Arm · 2021-07-05T13:38:56Z

rtos/tests/UNITTESTS/doubles/CMakeLists.txt

+
+add_definitions(-DUNITTEST)


Suggested change

add_definitions(-DUNITTEST)

This is already defined in UNITTESTS/CMakeLists.txt. The removal of this can be a separate commit.

Thanks for your review comment. This add_definition is required, and we can remove the one in "UNITTEST/CMakeLists.txt" as add_definitions (definition of UNITTEST macro ) visible in the current directory CMake and below add_subdirectories from there and don't have the option to expose to parent scope to visible across CMake

We may want to create a "unit-tests" INTERFACE target in UNITTEST/CMakeLists.txt, and link to it in each test executable, instead of using the directory scoped add_definitions to set this macro. Or maybe we can just bodge it into CMAKE_C_FLAGS and CMAKE_CXX_FLAGS globally, like we do with the coverage compile options?

Or maybe we can just bodge it into CMAKE_C_FLAGS and CMAKE_CXX_FLAGS globally, like we do with the coverage compile options?

This sounds like the easiest approach.

A more fundamental question is, a few libraries check UNITTEST:

$ grep -rn '\bUNITTEST\b' drivers/include/drivers/MbedCRC.h:32:#ifdef UNITTEST UNITTESTS/target_h/platform/mbed_assert.h:44:#ifndef UNITTEST storage/kvstore/tdbstore/tests/UNITTESTS/TDBStore/CMakeLists.txt:10: UNITTEST storage/kvstore/filesystemstore/tests/UNITTESTS/FileSystemStore/CMakeLists.txt:10: UNITTEST rtos/include/rtos/internal/mbed_rtos_storage.h:20:#if MBED_CONF_RTOS_PRESENT || defined(UNITTEST) rtos/include/rtos/internal/mbed_rtos1_types.h:20:#if MBED_CONF_RTOS_PRESENT || defined(UNITTEST) rtos/include/rtos/Thread.h:36:#if MBED_CONF_RTOS_PRESENT || defined(DOXYGEN_ONLY) || defined(UNITTEST) rtos/include/rtos/mbed_rtos_types.h:20:#if MBED_CONF_RTOS_PRESENT || defined(DOXYGEN_ONLY) || defined(UNITTEST)

but is this actually ideal? I thought libraries shouldn't need to specially handle tests. Instead, fakes/mocks/stubs/... should wrap tested modules while maintaining normal-use interfaces?
Anyway this is just out of curiosity, not the scope of this PR.

Not ideal at all IMO. Adding test specific logic to your production/library code is the wrong approach. Ideally we'd remove the need for those checks altogether.

There's one simple way to get rid of -DUNITTEST, at least from rtos which this PR is for. Notice the snippet in my previous comment - MBED_CONF_RTOS_PRESENT and defined(UNITTEST) are connected by ||. So what about defining MBED_CONF_RTOS_PRESENT=1 directly?

It's enough to remove add_definitions(-DUNITTEST) and add

target_compile_definitions(mbed-stubs-rtos PRIVATE MBED_CONF_RTOS_PRESENT=1 )

here, and also add MBED_CONF_RTOS_PRESENT=1 to

mbed-os/connectivity/cellular/tests/UNITTESTS/framework/device/athandler/CMakeLists.txt

Lines 8 to 16 in 8d3fc33

target_compile_definitions(${TEST_NAME}

PRIVATE

MBED_CONF_CELLULAR_DEBUG_AT=true

OS_STACK_SIZE=2048

DEVICE_SERIAL=1

DEVICE_INTERRUPTIN=1

MBED_CONF_PLATFORM_DEFAULT_SERIAL_BAUD_RATE=115200

MBED_CONF_CELLULAR_AT_HANDLER_BUFFER_SIZE=32

)

because ATHandler.cpp specifically checks this macro.

Then we can safely remove all defined(UNITTEST) checks from rtos/.

Seems like we should review all of these and fix them in another PR. It would be good to keep this PR scoped to removal of the stub-headers library.

There are not many of files to fix and the fix is simple as shown above, we can raise another PR to fix all of them in one go. I already made locally changes when experimenting a bit for my previous comment, so I can do it.

We may want to create a "unit-tests" INTERFACE target in UNITTEST/CMakeLists.txt, and link to it in each test executable, instead of using the directory scoped add_definitions to set this macro. Or maybe we can just bodge it into CMAKE_C_FLAGS and CMAKE_CXX_FLAGS globally, like we do with the coverage compile options?

I just moved the add_defenition into Mbed OS root Cmake under testing check as we may end up removing mbed-os/UNITTEST CMake after we migrated all stubs to their respective component and append -DUNITTEST into CMAKE_C_FLAGS and CMAKE_CXX_FLAGS will not work as it accepts -Dmacro=value when set() calls

Either is fine for this PR. I've created an issue #14883 to fully remove the UNITTEST macro.

Pull request has been modified.

LDong-Arm

LGTM

Pull request has been modified.

rajkan01 · 2021-07-07T08:55:00Z

@0xc0170 Could you review and start the CI for this PR

Patater · 2021-07-07T14:19:26Z

CI started

mergify · 2021-07-07T14:22:18Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

mbed-ci · 2021-07-07T15:04:23Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

Pull request has been modified.

- Previous changes moved all rtos stubs headers into mbed-stubs-rtos-headers lib, but the decision to keep all stubs headers under the respective component stubs library so moved all stubs rtos headers under mbed-stubs-rtos and updated it depend component CMake - Remove unnecessary add_definition call for UNITTEST as any of the stubs library added from UNITTEST/CMakeLists.txt is not required this macro

rajkan01 · 2021-07-09T07:24:07Z

@0xc0170 I have re-based this PR, Could you restart the CI

0xc0170 · 2021-07-09T07:29:46Z

CI restarted

mbed-ci · 2021-07-09T08:19:48Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

0xc0170 · 2021-07-09T08:33:46Z

I'll merge this. It was approved prior the conflict.

rajkan01 requested a review from LDong-Arm July 5, 2021 10:31

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Jul 5, 2021

ciarmcom requested a review from a team July 5, 2021 11:00

ciarmcom added needs: review component: test component: core component: connectivity labels Jul 5, 2021

LDong-Arm previously requested changes Jul 5, 2021

View reviewed changes

mergify bot added needs: work and removed needs: review labels Jul 5, 2021

rajkan01 force-pushed the move_rtos_stub_headers branch from 9d41b9b to d2a7ab9 Compare July 5, 2021 18:07

rajkan01 requested a review from LDong-Arm July 5, 2021 18:14

LDong-Arm previously approved these changes Jul 6, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Jul 6, 2021

LDong-Arm mentioned this pull request Jul 6, 2021

Unit tests: Libraries should not contain test-specific logic #14883

Closed

rajkan01 force-pushed the move_rtos_stub_headers branch from d2a7ab9 to d9fcff7 Compare July 6, 2021 15:41

LDong-Arm mentioned this pull request Jul 6, 2021

Remove the UNITTEST macro from production code #14884

Merged

LDong-Arm previously approved these changes Jul 7, 2021

View reviewed changes

mergify bot added needs: work and removed needs: CI labels Jul 7, 2021

rajkan01 force-pushed the move_rtos_stub_headers branch from d9fcff7 to 3fc6c4d Compare July 8, 2021 12:48

rajkan01 force-pushed the move_rtos_stub_headers branch from 3fc6c4d to bb3cd37 Compare July 8, 2021 14:15

0xc0170 added needs: CI and removed needs: work labels Jul 9, 2021

0xc0170 approved these changes Jul 9, 2021

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Jul 9, 2021

0xc0170 merged commit 8a507e6 into master Jul 9, 2021

mergify bot removed the ready for merge label Jul 9, 2021

0xc0170 deleted the move_rtos_stub_headers branch July 9, 2021 08:35

mbedmain added release-version: 6.13.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jul 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake: Remove mbed-stubs-rtos-headers library #14870

CMake: Remove mbed-stubs-rtos-headers library #14870

rajkan01 commented Jul 5, 2021

ciarmcom commented Jul 5, 2021

LDong-Arm Jul 5, 2021

rajkan01 Jul 5, 2021

rwalton-arm Jul 6, 2021

LDong-Arm Jul 6, 2021

rwalton-arm Jul 6, 2021

LDong-Arm Jul 6, 2021

rwalton-arm Jul 6, 2021 •

edited

Loading

LDong-Arm Jul 6, 2021

rajkan01 Jul 6, 2021

LDong-Arm Jul 6, 2021

LDong-Arm left a comment

rajkan01 commented Jul 7, 2021

Patater commented Jul 7, 2021

mergify bot commented Jul 7, 2021

mbed-ci commented Jul 7, 2021

rajkan01 commented Jul 9, 2021

0xc0170 commented Jul 9, 2021

mbed-ci commented Jul 9, 2021

0xc0170 commented Jul 9, 2021

	target_compile_definitions(${TEST_NAME}
	PRIVATE
	MBED_CONF_CELLULAR_DEBUG_AT=true
	OS_STACK_SIZE=2048
	DEVICE_SERIAL=1
	DEVICE_INTERRUPTIN=1
	MBED_CONF_PLATFORM_DEFAULT_SERIAL_BAUD_RATE=115200
	MBED_CONF_CELLULAR_AT_HANDLER_BUFFER_SIZE=32
	)

CMake: Remove mbed-stubs-rtos-headers library #14870

CMake: Remove mbed-stubs-rtos-headers library #14870

Conversation

rajkan01 commented Jul 5, 2021

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

ciarmcom commented Jul 5, 2021

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

rwalton-arm Jul 6, 2021 • 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

LDong-Arm left a comment

Choose a reason for hiding this comment

rajkan01 commented Jul 7, 2021

Patater commented Jul 7, 2021

mergify bot commented Jul 7, 2021

mbed-ci commented Jul 7, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

rajkan01 commented Jul 9, 2021

0xc0170 commented Jul 9, 2021

mbed-ci commented Jul 9, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

0xc0170 commented Jul 9, 2021

rwalton-arm Jul 6, 2021 •

edited

Loading