-
Notifications
You must be signed in to change notification settings - Fork 699
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
base: vsomeip_3.5.3
Are you sure you want to change the base?
Modernize CMake code #734
Conversation
CMakeLists.txt
Outdated
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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 |
46c5cac
to
da94c06
Compare
@kheaactua tried to build this new CMake code and got a failure. I have ubuntu 22.04, gcc version 11.4 the error i got was the following: |
@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. |
@kheaactua i just did a checkout to your branch and tried to build! |
@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:
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 :) |
da94c06
to
b45251e
Compare
b45251e
to
0c2f5f9
Compare
CMakeLists.txt
Outdated
# Dependencies | ||
################################################################################ | ||
# ###################################################################################################################### | ||
|
||
# Threads | ||
find_package(Threads REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove (duplicate)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
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.
Just a tonne of updates that help both with QNX and really just in general. |
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.
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.
e2c4cdc
to
9c53ff7
Compare
9c53ff7
to
d56e11e
Compare
- 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]>
d56e11e
to
38c935a
Compare
74bc25d
to
429fac5
Compare
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
.cmake-format
file, and ran applied it to the mainCMakeList
files in this project.if
andforeach
blocks, but I believe overall it does a better job that the default settings. The main benefit is a consistent formatinternal.hpp
to build directory instead of source to prevent needless rebuildsCMAKE_VERBOSE_MAKEFILE
, this should be injected by the user if so desired--wrap
option) the "socket" symbol, seewrappers.cpp
vsomeip/example/hello_world
andvsomeip_ctrl
ENABLE_SIGNALS
, but its setting was missing in CMakebacktrace.h
fileVSOMEIP_BASE_PATH
. Upstream defaults this to/var
for QNX which is a very bad choice.Misc