-
Notifications
You must be signed in to change notification settings - Fork 714
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: master
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
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. |
e2c4cdc
to
9c53ff7
Compare
d56e11e
to
38c935a
Compare
2638cb5
to
74bc25d
Compare
38c935a
to
758d37a
Compare
1e8136d
to
38685d1
Compare
The build error is weird.. Though I do get a similar one locally:
Not sure what the cause is, will investigate. |
4e04a75
to
dfbf5ec
Compare
08bb3a3
to
b0e3851
Compare
6b4f921
to
4ebb5b6
Compare
examples/CMakeLists.txt
Outdated
# terms of the Mozilla Public License, v. 2.0. If a copy of the MPL was not distributed with this file, You can obtain | ||
# one at http://mozilla.org/MPL/2.0/. | ||
|
||
cmake_minimum_required(VERSION 3.15) |
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.
cmake_minimum_required(VERSION 3.15) | |
cmake_minimum_required(VERSION 3.13) |
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.
Fixed in a5f34c0
tools/vsomeip_ctrl/CMakeLists.txt
Outdated
install( | ||
TARGETS vsomeip_ctrl RUNTIME | ||
COMPONENT vsomeip_ctrl | ||
EXCLUDE_FROM_ALL |
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.
install( | |
TARGETS vsomeip_ctrl RUNTIME | |
COMPONENT vsomeip_ctrl | |
EXCLUDE_FROM_ALL | |
install( | |
TARGETS vsomeip_ctrl RUNTIME | |
DESTINATION "${INSTALL_BIN_DIR}" | |
COMPONENT vsomeip_ctrl | |
EXCLUDE_FROM_ALL |
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.
Fixed in e967d58
Included a .cmake-format file, and ran applied it to the main CMakeList files in this project.
- 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 - "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 - 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]>
Suggested by Duarte, a better fix is likely to be posted upstream.
Adjusting minimum CMake version
Restoring install destination for vsomeip_ctrl
e967d58
to
1ece8d3
Compare
Fixed pkg-config CMake flag
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