Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move ble stubs to FEATURE_BLE lib #14912

Merged
merged 5 commits into from
Jul 21, 2021
Merged

Move ble stubs to FEATURE_BLE lib #14912

merged 5 commits into from
Jul 21, 2021

Conversation

rajkan01
Copy link
Contributor

Summary of changes

fixes #14857
Preceding PR #14896

  • Previously ble headers are part of mbed-headers-connectivity, this PR moves ble headers into new mbed-headers-ble to make ble stubs to be more self-contained and improves the composition of the library.
  • Moved all the headers out of mbed-headers-connectivity, and removed all mbed-headers-connectivity lib reference from lorawan and cellular unit tests
  • Previously ble fakes as part of UNITTESTS/fakes, this PR moves ble fakes to FEATURE_BLE double directory to make ble stubs
    to be self-contained.

Impact of changes

None.

Migration actions required

None

Documentation

To be updated

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 ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Jul 12, 2021
@ciarmcom
Copy link
Member

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

@ciarmcom ciarmcom requested a review from a team July 12, 2021 11:00
@mergify
Copy link

mergify bot commented Jul 12, 2021

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

LDong-Arm
LDong-Arm reviewed Jul 12, 2021
View reviewed changes
Copy link
Contributor

@LDong-Arm LDong-Arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me apart from a small comment

connectivity/FEATURE_BLE/tests/UNITTESTS/doubles/fakes/CMakeLists.txt Outdated
# Copyright (c) 2020 ARM Limited. All rights reserved.
# SPDX-License-Identifier: Apache-2.0

add_subdirectory(ble)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need the ble subdirectory because this is already in the BLE component.
Maybe we can also flatten files in fakes/ble/source/ into fakes/, to be consistent with other components' stubs?

Copy link
Contributor Author

@rajkan01 rajkan01 Jul 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't move fakes/ble/source/ into fakes as this directory structure try to replicate the actual FEATURE_BLE component source structure

For example:

https://github.com/ARMmbed/mbed-os/blob/master/connectivity/FEATURE_BLE/source/Gap.cpp#L19

@paul-szczepanek-arm
Copy link
Member

I'm fine with the move however, did anyone try compiling the ble fake library? Does it compile for you?

@LDong-Arm
Copy link
Contributor

LDong-Arm commented Jul 12, 2021 •
edited
Loading

I'm fine with the move however, did anyone try compiling the ble fake library? Does it compile for you?

@paul-szczepanek-arm Good question, I assumed it was used during my review. But now having checked the CMake target mbed-fakes-ble, apparently it's not linked by any tests. Same for mbed-fakes-event-queue.

@rajkan01 It's probably worth trying deleting the fakes and check that tests compile?

@paul-szczepanek-arm
Copy link
Member

I don't think they can compile. ble fakes depends on event queue but that is no longer in the dependencies and also platform headers are now separate so they're missing too. Tests in https://github.com/ARMmbed/mbed-os-experimental-ble-services/ use them and they now fail to compile because of it. I'm working on fixes but obviously I have to wait until you finish moving the files.

@LDong-Arm
Copy link
Contributor

@paul-szczepanek-arm If the fakes are not used by mbed-os, why not move them into mbed-os-experimental-ble-services?

@paul-szczepanek-arm
Copy link
Member

then no one else could use them

@paul-szczepanek-arm
Copy link
Member

Disregard my previous comments. The libs compile fine. What is not obvious is that you have to add the headers yourself as otherwise they are not compiled and cmake doesn't tell you that a library it depends on is missing - just blindly tries to compile and fails. I got the tests running just by manually adding the headers libs to the my cmake.

@0xc0170 0xc0170 requested a review from a team July 13, 2021 14:21
@rajkan01 rajkan01 force-pushed the move_unittest_ble_headers_lib branch from 163d2f8 to 0a8f971 Compare July 13, 2021 14:57
0xc0170
0xc0170 previously approved these changes Jul 14, 2021
View reviewed changes
@0xc0170
Copy link
Contributor

0xc0170 commented Jul 14, 2021

CI started

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 14, 2021

Once CI completes, we might close/open PR to retrigger Travis

@mbed-ci
Copy link

mbed-ci commented Jul 14, 2021

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_build-cloud-example-ARM ✔️
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-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 ✔️

@ciarmcom ciarmcom added the stale Stale Pull Request label Jul 15, 2021
@ciarmcom
Copy link
Member

This pull request has automatically been marked as stale because it has had no recent activity. @ARMmbed/mbed-os-maintainers, please start CI to get the PR merged.

@0xc0170 0xc0170 closed this Jul 16, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Jul 16, 2021

I'll reopen in couple of minutes to restart Travis

@mergify mergify bot removed component: test needs: CI release-type: patch Indentifies a PR as containing just a patch stale Stale Pull Request labels Jul 16, 2021
@LDong-Arm
Copy link
Contributor

Guess we can reopen this PR?

@LDong-Arm LDong-Arm reopened this Jul 20, 2021
@mergify mergify bot added the needs: work label Jul 20, 2021
@rajkan01
CMake: Create mbed-headers-ble library
a61392a 
Previously ble headers are part of mbed-headers-connectivity, this PR
moves ble headers into new mbed-headers-ble to make ble stubs to be
more self-contained and improves the composition of the library.
@rajkan01
Remove mbed-headers-connectivity
86737bd 
Moved all the headers out of mbed-headers-connectivity, and these changes remove
that headers lib reference from lorawan and cellular unit tests
@rajkan01
CMake: fakes ble: Remove trailing whitespace
1b5f597
@rajkan01
CMake: Move ble fakes into FEATURE_BLE double dir
9df7a50 
Previously ble fakes as part of UNITTESTS/fakes, this PR moves
ble fakes to FEATURE_BLE double directory to make ble stubs
to be self-contained.
@rajkan01
CMake: Limit the mbed-fakes-ble to required dependency mock
fb5b0ed
@rajkan01 rajkan01 force-pushed the move_unittest_ble_headers_lib branch from 0a8f971 to fb5b0ed Compare July 21, 2021 08:57
@mergify mergify bot dismissed stale reviews from paul-szczepanek-arm and 0xc0170 July 21, 2021 08:57

Pull request has been modified.

@0xc0170 0xc0170 added needs: CI release-type: patch Indentifies a PR as containing just a patch and removed needs: work labels Jul 21, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Jul 21, 2021

Ci started, labels fixed.

@mbed-ci
Copy link

mbed-ci commented Jul 21, 2021

Jenkins CI Test : ✔️ SUCCESS

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

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
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_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-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-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@0xc0170 0xc0170 merged commit 655d889 into master Jul 21, 2021
@0xc0170 0xc0170 deleted the move_unittest_ble_headers_lib branch July 21, 2021 12:52
@mergify mergify bot removed the ready for merge label Jul 21, 2021
@mbedmain mbedmain added release-version: 6.14.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LDong-Arm LDong-Arm LDong-Arm left review comments

@0xc0170 0xc0170 0xc0170 approved these changes

@paul-szczepanek-arm paul-szczepanek-arm paul-szczepanek-arm left review comments

Assignees
No one assigned
Labels
release-version: 6.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move remaining stubs files from UNITTESTS/stubs/connectivity to the Connectivity/**
7 participants
@rajkan01 @ciarmcom @paul-szczepanek-arm @LDong-Arm @0xc0170 @mbed-ci @mbedmain