Skip to content
This repository has been archived by the owner on Sep 7, 2020. It is now read-only.

PPM-300: Add Bus agnostic API as optional dependency to the builds #1545

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

PPM-300: Add Bus agnostic API as optional dependency to the builds #1545

wants to merge 7 commits into from

Conversation

abelog
Copy link
Collaborator

@abelog abelog commented Jul 21, 2020 •
edited
Loading

Description

This PR intention is to introduce new HLAPI lib (High Level API) which should which links all BAAPI (Bus Agnostic API) libs.
hlapi is yet optional for build, as we don't use it right now.
Tested for Dummy build on CI. Flag was added by 113da93 commit. Job and pipeline.

Changes

  • Add Findamx?.cmake files to locate each `amx(c/d/j...) lib.
  • Add new hlapi lib and new flag ENABLE_HLAPI to build it.

Note

As we don't need to build hlapi right now - ENABLE_HLAPI flag isn't required yet, so no changes to regular build procedure used by CI and developers is required. Build in CI was done just for verification.

Jira link: https://jira.prplfoundation.org/browse/PPM-300

arnout
arnout reviewed Jul 24, 2020
View reviewed changes
Copy link
Collaborator

@arnout arnout left a comment

Choose a reason for hiding this comment

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

Since draft, not approving yet.

I gave comments to the first Findxxx.cmake but obviously they apply to all.

cmake/Findamxc.cmake Outdated Show resolved Hide resolved
cmake/Findamxc.cmake Outdated Show resolved Hide resolved
cmake/Findamxc.cmake Outdated
Comment on lines 9 to 11
find_path(AMXC_INCLUDE_DIRS
NAMES amxc_llist.h amxc_string_join.h amxc_common.h amxc_rbuffer.h amxc_variant_type.h amxc_variant.h amxc_aqueue.h amxc_timestamp.h amxc_astack.h amxc_hash.h amxc_array.h amxc.h amxc_lstack.h amxc_string.h amxc_htable.h amxc_lqueue.h amxc_string_split.h
PATH_SUFFIXES amxc
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need to list all of them, typically amxc.h should be enough.

Also, typically you'd include it with #include <amxc/amxc.h> so you should search directly for that file.

Suggested change
find_path(AMXC_INCLUDE_DIRS
NAMES amxc_llist.h amxc_string_join.h amxc_common.h amxc_rbuffer.h amxc_variant_type.h amxc_variant.h amxc_aqueue.h amxc_timestamp.h amxc_astack.h amxc_hash.h amxc_array.h amxc.h amxc_lstack.h amxc_string.h amxc_htable.h amxc_lqueue.h amxc_string_split.h
PATH_SUFFIXES amxc
)
find_path(AMXC_INCLUDE_DIRS NAMES amxc/amxc.h)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Applied. Only thing, that PATH_SUFFIXES already point to the subdirectory which should be checked, so i am not sure that is worth to implicitly add amxc/amxc.h.

CMakeLists.txt Outdated
@@ -14,6 +14,7 @@ option (BUILD_AGENT "Build EasyMesh agent" ON)
option (BUILD_CONTROLLER "Build EasyMesh controller" ON)

option(BUILD_SHARED_LIBS "Build shared libraries (.so) instead of static ones (.a)" ON)
option (ENABLE_AMBIORIX "Include Bus agnostic API in the build" OFF)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I mentioned orally:

Suggested change
option (ENABLE_AMBIORIX "Include Bus agnostic API in the build" OFF)
option (ENABLE_HLAPI "Build the northbound (high-level) API" OFF)

Also please turn it on in at least one build in CI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Applied. Will resolve as commit to alter CI to check build with flag is added.

@abelog abelog force-pushed the feature/ppm-300-optional-dependency-on-BAAPI-to-prplMesh-CMakeLists branch from 0ca22ec to 81d14b9 Compare July 24, 2020 20:40
RanRegev
RanRegev reviewed Jul 26, 2020
View reviewed changes
Copy link
Collaborator

@RanRegev RanRegev left a comment

Choose a reason for hiding this comment

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

Maybe it is too early in the process, but I guess we don't want just hlapi.h file that represents the entire high-level interface of prplmesh.
The HLAPI should be designed and it would probably end up with

namespace prplmesh::hlapi { /*...*/ }

and few classes and/or free functions within this namespace.

framework/platform/CMakeLists.txt Outdated Show resolved Hide resolved
framework/platform/HLAPI/CMakeLists.txt Outdated Show resolved Hide resolved
framework/platform/HLAPI/CMakeLists.txt Outdated
@@ -0,0 +1,43 @@
project(hlapi VERSION ${prplmesh_VERSION})
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe preceding with prplmesh_?

Suggested change
project(hlapi VERSION ${prplmesh_VERSION})
project(prplmesh_hlapi VERSION ${prplmesh_VERSION})

framework/platform/HLAPI/CMakeLists.txt Outdated Show resolved Hide resolved
@abelog abelog force-pushed the feature/ppm-266-baapi-to-dummy-docker-builder branch from c6f7f8d to 9f1c0d2 Compare August 3, 2020 14:12
@arnout arnout force-pushed the feature/ppm-266-baapi-to-dummy-docker-builder branch from 9f1c0d2 to 35f9570 Compare August 3, 2020 14:57
Base automatically changed from feature/ppm-266-baapi-to-dummy-docker-builder to master August 3, 2020 15:12
@abelog abelog force-pushed the feature/ppm-300-optional-dependency-on-BAAPI-to-prplMesh-CMakeLists branch from 81d14b9 to 43562cb Compare August 3, 2020 15:49
@abelog abelog changed the title Feature/ppm 300 optional dependency on baapi to prpl mesh c make lists PPM-300: optional dependency on baapi to prpl mesh c make lists Aug 4, 2020
@abelog abelog force-pushed the feature/ppm-300-optional-dependency-on-BAAPI-to-prplMesh-CMakeLists branch 6 times, most recently from 198f070 to 113da93 Compare August 6, 2020 12:24
@abelog abelog changed the title PPM-300: optional dependency on baapi to prpl mesh c make lists PPM-300: Add Bus agnostic API as optional dependency to the builds Aug 6, 2020
@abelog abelog force-pushed the feature/ppm-300-optional-dependency-on-BAAPI-to-prplMesh-CMakeLists branch from 113da93 to 9a1ff0b Compare August 6, 2020 12:54
@abelog abelog marked this pull request as ready for review August 6, 2020 13:29
@abelog
Copy link
Collaborator Author

abelog commented Aug 13, 2020

@Mergifyio rebase

abelog added 3 commits August 13, 2020 13:34
@abelog
cmake: add Findamxc.cmake file
05a42f7 
Add Findamxc.cmake file to locate AMXC library of BAAPI and relevant header
files.

Jira link: https://jira.prplfoundation.org/browse/PPM-300

Signed-off-by: Anton Bilohai <[email protected]>
@abelog
cmake: add Findamxb.cmake file
93c29ea 
Add Findamxb.cmake file to locate AMXB library of BAAPI and relevant
header files.

Jira link: https://jira.prplfoundation.org/browse/PPM-300

Signed-off-by: Anton Bilohai <[email protected]>
@abelog
cmake: add Findamxd.cmake file
d08a665 
Add Findamxd.cmake file to locate AMXD library of BAAPI and relevant
header files.

Jira link: https://jira.prplfoundation.org/browse/PPM-300

Signed-off-by: Anton Bilohai <[email protected]>
abelog added 4 commits August 13, 2020 13:34
@abelog
cmake: add Findamxj.cmake file
c78296b 
Add Findamxj.cmake file to locate AMXJ library of BAAPI and relevant
header files.

Jira link: https://jira.prplfoundation.org/browse/PPM-300

Signed-off-by: Anton Bilohai <[email protected]>
@abelog
cmake: add Findamxo.cmake file
da06824 
Add Findamxo.cmake file to locate AMXO library of BAAPI and relevant
header files.

Jira link: https://jira.prplfoundation.org/browse/PPM-300

Signed-off-by: Anton Bilohai <[email protected]>
@abelog
cmake: add Findamxp.cmake file
1c7be8c 
Add Findamxp.cmake file to locate AMXP library of BAAPI and relevant
header files.

Jira link: https://jira.prplfoundation.org/browse/PPM-300

Signed-off-by: Anton Bilohai <[email protected]>
@abelog
cmake: create separate HLAPI library
84eceea 
Create new Hight Level API library, which should handle interactions
with different system buses.

Jira link: https://jira.prplfoundation.org/browse/PPM-300

Signed-off-by: Anton Bilohai <[email protected]>
@mergify
Copy link

mergify bot commented Aug 13, 2020

Command rebase: success

Branch has been successfully rebased

@arnout arnout force-pushed the feature/ppm-300-optional-dependency-on-BAAPI-to-prplMesh-CMakeLists branch from 9a1ff0b to 84eceea Compare August 13, 2020 13:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@RanRegev RanRegev RanRegev left review comments

@arnout arnout arnout left review comments

@adam1985d adam1985d Awaiting requested review from adam1985d adam1985d is a code owner

@kantera800 kantera800 Awaiting requested review from kantera800 kantera800 is a code owner

@mariomaz mariomaz Awaiting requested review from mariomaz mariomaz is a code owner

@morantr morantr Awaiting requested review from morantr morantr is a code owner

@rmelotte rmelotte Awaiting requested review from rmelotte rmelotte is a code owner

@tomereli tomereli Awaiting requested review from tomereli tomereli is a code owner

@vitalybu vitalybu Awaiting requested review from vitalybu vitalybu is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

@abelog abelog

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abelog @arnout @RanRegev