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.

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

Modernize CMake code #734

Draft
wants to merge 24 commits into
base: vsomeip_3.5.3
Choose a base branch
from
Draft

Conversation

kheaactua
Copy link
Contributor

@kheaactua kheaactua commented Jun 28, 2024

Status

For the moment this is still a WIP. I would love community feedback on this PR so that we can produce a modern CMake setup that functions well for everyone.

I suspect the cleanest way to organize the commits would be to squash them.

Description

General CMakeLists improvements - cmake v2→v3

  • Included a .cmake-format file, and ran applied it to the main CMakeList files in this project.
    • The format file does a poor job on if and foreach blocks, but I believe overall it does a better job that the default settings. The main benefit is a consistent format
  • Use targets instead of lib/include variables in CMake
  • [Conditionally] Linking DLT as a target
  • Write internal.hpp to build directory instead of source to prevent needless rebuilds
    • The include directives for this file are also updated in the code to use include paths rather than relative paths.
  • Remove hard coded CMAKE_VERBOSE_MAKEFILE, this should be injected by the user if so desired
  • "wrap"ing (ld's --wrap option) the "socket" symbol, see wrappers.cpp
  • Added install directive to vsomeip/example/hello_world and vsomeip_ctrl
  • Platform conditional socket lib linkage
  • hello_world code uses ENABLE_SIGNALS, but its setting was missing in CMake
  • Improve setup for boost stacktrace, really only helps on Linux and requires the backtrace.h file
  • Added QNX platform section, though the credential code changes for QNX are not included in this commit.
  • Use a CACHE variable for VSOMEIP_BASE_PATH. Upstream defaults this to /var for QNX which is a very bad choice.

Misc

  • Added more ignore rules to for common IDEs

CMakeLists.txt Outdated
Comment on lines 112 to 126
include(CMakeDependentOption)
cmake_dependent_option(ENABLE_WERROR "Enable -Werror flag" ON "CMAKE_SYSTEM_NAME MATCHES LINUX" OFF)
if (ENABLE_WERROR)
message(STATUS "Enabling -Werror flag")
add_compile_options(-Werror)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did you separated this flag from the others related to LINUX?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The -Werror blocks my linux builds :) Honestly I don't think any component should ever include this. -Werror should be up to the toolchain - as the toolchain usually specifies which compile options to use, which warnings to watch for, and whether warnings should be allowed.

@kheaactua
Copy link
Contributor Author

I am still working on this PR, I'll likely rebase to Fabio's 3.5.x branch. Stuff just takes time. :)

@duartenfonseca
Copy link
Collaborator

I am still working on this PR, I'll likely rebase to Fabio's 3.5.x branch. Stuff just takes time. :)

In that case maybe i will check this PR later! great job anyway, it has interesting improvements

@kheaactua kheaactua force-pushed the modern_cmake branch 6 times, most recently from 46c5cac to da94c06 Compare August 16, 2024 21:29
@duartenfonseca
Copy link
Collaborator

duartenfonseca commented Sep 9, 2024

@kheaactua tried to build this new CMake code and got a failure.
I did :
cmake -B build
cmake --build build

I have ubuntu 22.04, gcc version 11.4

the error i got was the following:
/home/duartef/repos/covesa/vsomeip_duarte/vsomeip/implementation/endpoints/src/../../routing/include/../../endpoints/include/netlink_connector.hpp:197:47: error: ‘VSOMEIP_MAX_NETLINK_RETRIES’ was not declared in this scope
197 | static const std::uint32_t max_retries_ = VSOMEIP_MAX_NETLINK_RETRIES;

@kheaactua
Copy link
Contributor Author

@duartenfonseca I'll take a look! Was this on Fabio's 3.5.x? He did push a change to the network tests a few days back.

I'll also fix that conflict that appeared.

@duartenfonseca
Copy link
Collaborator

@kheaactua i just did a checkout to your branch and tried to build!

@duartenfonseca
Copy link
Collaborator

@kheaactua is the increase of minimum version on cmake is really necessary? from our side, we would like to avoid that

@kheaactua
Copy link
Contributor Author

@kheaactua is the increase of minimum version on cmake is really necessary? from our side, we would like to avoid that

I raised it to 3.15 to accommodate Conan 2.0. From the author:

Conan 2.0 requires at least CMake 3.15.

Though not yet posted, I've been prepping covesa/capi-conan, covesa/capi-docker, and some others that all use Conan to set up CommonAPI environments.

Curious though, what would be desirable about staying at 3.13? With 3.23 vsomeip could use file sets for a cleaner install of the includes

Locally I jack it up to 3.30 :)

CMakeLists.txt Outdated
# Dependencies
################################################################################
# ######################################################################################################################

# Threads
find_package(Threads REQUIRED)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove (duplicate)

Copy link
Collaborator

@duartenfonseca duartenfonseca Nov 26, 2024

Choose a reason for hiding this comment

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

if you can remove the duplicate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@duartenfonseca
Copy link
Collaborator

@kheaactua is the increase of minimum version on cmake is really necessary? from our side, we would like to avoid that

I raised it to 3.15 to accommodate Conan 2.0. From the author:

Conan 2.0 requires at least CMake 3.15.

Though not yet posted, I've been prepping covesa/capi-conan, covesa/capi-docker, and some others that all use Conan to set up CommonAPI environments.

Curious though, what would be desirable about staying at 3.13? With 3.23 vsomeip could use file sets for a cleaner install of the includes

Locally I jack it up to 3.30 :)

I understand your reasons, but if we could find a way to keep the CMake changes without increasing from 3.13 would be best. From our internal process, increasing this there would need to be a strong justification for this change.

@kheaactua
Copy link
Contributor Author

kheaactua commented Oct 16, 2024

@kheaactua is the increase of minimum version on cmake is really necessary? from our side, we would like to avoid that

I raised it to 3.15 to accommodate Conan 2.0. From the author:

Conan 2.0 requires at least CMake 3.15.

Though not yet posted, I've been prepping covesa/capi-conan, covesa/capi-docker, and some others that all use Conan to set up CommonAPI environments.
Curious though, what would be desirable about staying at 3.13? With 3.23 vsomeip could use file sets for a cleaner install of the includes
Locally I jack it up to 3.30 :)

I understand your reasons, but if we could find a way to keep the CMake changes without increasing from 3.13 would be best. From our internal process, increasing this there would need to be a strong justification for this change.

I could make Conan apply a patch, that would solve the issue with Conan. I'll give it a try with 3.13 as soon as I can. I may have to remove some of the generator expressions to support 3.13 as well.

Also, for me the Docker setup for the network tests fails with CMake 3.22 and below.

That said, I would strongly encourage the update.

  • 3.26 supports LTO detection on QNX
  • 3.26 improves the debug on try_compile - really helpful when trying to build on new systems
  • 3.23 brings target_source( FILE_SET ) for better installation
  • 3.18 and 3.30 improve how Boost is found (even as far back as 3.16 find_package(boost) is harder than it should be)
  • 3.15 improves generator expressions
  • 3.14 and 3.27 improve the build debug
  • ~3.30 (I think) improves handling of generated files (helpful for CommonAPI)

Just a tonne of updates that help both with QNX and really just in general.

Ana Rodrigues and others added 10 commits November 18, 2024 17:59
Revert Fix compile issue with pthreads in android commit
Revert Force abort hanging detached threads commit

The commits related to the shutdown cause crashes.
Summary
Adds parsing to the routing_info command wireshark dissector
Details
The wireshark vsomeip dissector will now also parse the payload
of the routing_info command entries.
It will display the information of the subcommands:
add_client, del_client, add_service and del_service.
Adds also parsing for the request service and fixes the assign client name.
Summary
Fix race condition when boost async operations are executed
Summary
Fix core dump back traces.
Added some improves in initial event test client.

Details
Issue with initial_event_stop_service: The application tries to send a message,
and then starts the deregister process.
The issue occurs when the routing manager received the deregister request
before forwarding the message sent by the application. The issue was solved
with a small sleep after the application sent the message.
Summary
Adds the VSOMEIP_CONFIG command payload parsing.
Details
The wireshark vsomeip dissector will now also parse the payload
of the VSOMEIP_CONFIG command.
It will display the information of the key and value.
Additionally, a hint is added in the documentation referring to the
magical number prologue and epilogue contained in vsomeip packet payload.
Summary
REGISTER_APPLICATION_ACK command will now validate past requests.
Local tcp connections are now limited to 2 retries instead of being unlimited.
Details
After receiving the REGISTER_APPLICATION_ACK command the routing
host will now check if the client has previously request services.
This will be specially needed in cases where, after/during a suspend,
a local tcp client loses its connection. When re registering successfully,
the host will check the requests.
The host will send new routing infos with the services to notify the client
of its availabilities/unavailabilities.
the local_tcp endpoints will now follow a similiar logic of the local_uds
endpoints on re connect attempts.
Before this was unlimited, and the client endpoints could be stuck in a
forever loop trying to reconnect to applications that would never return.
Now they will have 2 re attemps before closing.
The only exception is the connection between client to routing host,
this one still needs to keep on retrying.
Summary
Suspend resume tests now starts "runner" thread after app->init
Details
The goal of the changes in this PR is to move the start of the "runner"
thread in which the app->start runs from the initialization list of the
suspend_resume_test_service and only start the thread after the app init.
The problem that lead to this change was that for some reason, the "runner"
thread although it should start when the test starts, very often it only started
after the "start" function of the test that runs the app->init. This is a problem
since when the "runner" thread starts, it locks a mutex and waits a condition
variable to be notified to then run the app->start, and the "start" function is
the one that notifies the condition variable after running the app->init. What
was happening was that the "runner" thread did not start at the beginning of
the test, the "start" function would notify the condition variable and only then
the "runner" thread would start and wait for the condition variable to be
notified, which never happened since the condition variable was notified
before the "runner" thread started, leading to a timeout.
To fix this issue, I removed the initialization of the "runner" thread away
from the initialization list and only initialized it after the app->init was run,,
without using condition variables to guarantee that the app->start happens
after the app->init. Before this fix the issue would occur very frequently
(>50% of runs) on a VM with roughly the same resources as zuul, and with
this fix it never failed again locally.
andrefesilva and others added 12 commits November 18, 2024 18:01
Summary
Add service and method to the clients_ array so it is possible to process 2 requests with the same client and session
Details
Add service and method to the clients_ array so it is possible to process 2 requests with the same client and session
Summary
Protects member variable receiver_, adds nullptr checks and locker to routing stop function
Details
New mutex is created for receiver_ member variable (as we have done with sender_). The receiver_ is now thread safe.
Adds extra nullptr checks and new lock for the routing stop function for proper synchronization among the threads.
The routing_stop_mutex_ is created to prevent other actions from occurring while ::stop() is being executed.
For example, this prevents actions like network changes or assigning the client during the stop operation.
You will observe this mutex being used in ::on_net_state_change and ::on_message.
Specifically, it is utilized in the ASSIGN_CLIENT_ACK_ID condition to ensure proper sync during these operations.
Summary
Add include of unordered map
Details
To fix a problem in a previous PR where unordered map
was used without including unordered map, unordered map
was now included in the appropriate file
Summary
Fixes possible DOS.
Details
Prevent vector allocation with invalid length during option deserialization.
Return early if the user-supplied length value is zero to avoid exceptions.
Summary
Ignore send_cbk if the socket is not connected
Details
Possible race condition during connect_cbk and send_cbk,
causes the endpoint to stop attempting to reconnect.
In the case where the socket is not yet connected, or connecting,
the send_cbk should do nothing.
Summary
Move suspend/resume of endpoints
Details

Suspend endpoints as the last action when suspending
Resume endpoints as the first action when resuming
Check the target state of the endpoints after suspending/resuming them
Summary
Change handling of availabilities for wildcasts (ANY_SERVICE, ANY_INSTANCE)
Details
Do _NOT report the state of the wildcard (there cannot be any consistent state),
but report the available instances instead.
Summary
Allow to configure the initial routing state to allow suspended startup
Details
Add a configuration parameter service-discovery.initial_state to allow the configuration of the behavior
on startup. Additionally, ensure that new endpoints that are created while the routing is suspended are
not started. This will happen on the next resume.
Changes:
Revert fix compile issue with pthreads in android commit
Revert Force abort hanging detached threads commit
Remove dead code path
Update wireshark dissector
fix race condition on boost async operations
Enabled initial event tests
Add vsomeip config parsing
Whitelist initial events tests
Adds on_register_application_ack
Suspend resume tests fix
add service and method ids to the clients_ array
Fix capturing references to stack variables
Protects receiver_ and lock to routing stop function
Add include unordered map
Add remove local in deregistration(rmi)
protect deserialization from malicious input
Ignore send_cbk if the socket is not connected
Move suspend/resume of endpoints
Change handling of availabilities for wildcasts
Allow to configure the initial routing state
Included a .cmake-format file, and ran applied it to the main CMakeList files in this project.
@kheaactua kheaactua force-pushed the modern_cmake branch 5 times, most recently from e2c4cdc to 9c53ff7 Compare November 26, 2024 13:19
@duartenfonseca duartenfonseca changed the base branch from master to vsomeip_3.5.3 November 26, 2024 17:07
kheaactua and others added 2 commits November 26, 2024 12:53
- Use literal target names instead of variables
- Write internal.hpp to build directory instead of source to prevent needless rebuilds
  - Fixed how this file is includes in source files
- Remove hard coded CMAKE_VERBOSE_MAKEFILE
- Ran cmake-format
- "wrap"ing (ld's --wrap option) the "socket" symbol, see wrappers.cpp
- Added install directive to vsomeip/example/hello_world and
  vsomeip_ctrl
- Platform conditional socket lib linkage
- hello_world code uses ENABLE_SIGNALS, but its setting was missing in CMake
- Improve setup for boost stacktrace, really only helps on Linux and
  requires the backtrace.h file
- Added QNX platform section, though the credential code changes for QNX
  are not included in this commit.
- Use a CACHE variable for VSOMEIP_BASE_PATH.  Upstream defaults this to
  /var for QNX which is a very bad choice.
- Followed the 3.5.x example and remove the dependency on routingmanager
  in the tests, as it's not available on Windows
Co-authored-by: "Duarte Fonseca" <[email protected]>
Co-authored-by: "Matthew Russell" <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants