Simplify linking, update dependencies #2069
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The problem persists, this is what the linking error looks like: |
JonasVautherin
previously approved these changes
Jun 1, 2023
julianoes
force-pushed
the
pr-simplify-linking
branch
2 times, most recently
from
5bc5312
to
3a867f4
Compare
|
Not completely sure what the latest changes imply. Do they solve the Windows problem? |
|
Nope, nothing solves the Windows problem 😞 |
julianoes
force-pushed
the
pr-simplify-linking
branch
3 times, most recently
from
d0c74e0
to
ef5e3cf
Compare
julianoes
changed the title
JonasVautherin
requested changes
Jun 11, 2023
CMakeLists.txt
Outdated
| @@ -1,4 +1,4 @@ | |||
| cmake_minimum_required(VERSION 3.13) | |||
| cmake_minimum_required(VERSION 3.13...3.22) | |||
There was a problem hiding this comment.
hmm what's that? 🤔
There was a problem hiding this comment.
It's just a max version that it has been tested with and policies set. I added it because there was a warning about it but it actually came from the zlib CMakeLists.txt where the minimum version was 2.4.
third_party/openssl/CMakeLists.txt
Outdated
| @@ -36,7 +36,7 @@ if(ANDROID) | |||
| PREFIX openssl | |||
| PATCH_COMMAND patch -p 0 < ${PROJECT_SOURCE_DIR}/dockcross-android.patch | |||
| CONFIGURE_COMMAND ${CMAKE_COMMAND} -E env ANDROID_NDK=$ENV{CROSS_ROOT} <SOURCE_DIR>/Configure --sysroot=$ENV{CROSS_ROOT}/sysroot ${OPENSSL_PLATFORM} --prefix=${CMAKE_INSTALL_PREFIX} ${OPENSSL_BUILD_TYPE} no-shared | |||
| BUILD_COMMAND make | |||
| BUILD_COMMAND make install_sw | |||
There was a problem hiding this comment.
Sounds like an INSTALL_COMMAND?
There was a problem hiding this comment.
Seems to work though.
There was a problem hiding this comment.
Sure, we could write ./configure && make && make install in the CONFIGURE_COMMAND, and CMake wouldn't know about it. But we would 😁.
It's nice if the log output says "starting install step" before running the installation step, I think 😇
third_party/zlib/CMakeLists.txt
Outdated
| PREFIX zlib | ||
| PATCH_COMMAND git checkout . && git apply ${PROJECT_SOURCE_DIR}/build_shared_libs.patch | ||
| PATCH_COMMAND git checkout . && git apply ${PROJECT_SOURCE_DIR}/build_shared_libs_and_cmake_version.patch |
There was a problem hiding this comment.
Can we split that in two PATCH_COMMAND lines? They seem unrelated 🤔
There was a problem hiding this comment.
Sure. I just thought putting it into one patch is easier.
There was a problem hiding this comment.
Ok, as you want. I see patches like commits. It's easier for me to read a commit that does one thing than to try to understand which line corresponds to what when reading the commit.
But if you like one patch better, technically it works 👍.
| - DESTINATION ${gRPC_INSTALL_CMAKEDIR} | ||
| - NAMESPACE gRPC:: | ||
| - ) | ||
| + if (gRPCPluginTargets AND gRPC_BUILD_CODEGEN) |
There was a problem hiding this comment.
Is that a bug in gRPC upstream?
There was a problem hiding this comment.
I think so and it has supposedly been fixed but either the fix didn't go into v1.55.1 or it doesn't fully fix it for us for some reason:
grpc/grpc#32885
| PREFIX zlib | ||
| PATCH_COMMAND git checkout . && git apply ${PROJECT_SOURCE_DIR}/build_shared_libs.patch | ||
| PATCH_COMMAND git checkout . && git apply ${PROJECT_SOURCE_DIR}/build_shared_libs.patch && git apply ${PROJECT_SOURCE_DIR}/bump_cmake_min_version.patch |
There was a problem hiding this comment.
Note (but it does not matter much). You can add as many COMMAND as you want between the "key" commands. So this should work:
PATCH_COMMAND git checkout . && git apply ${PROJECT_SOURCE_DIR}/build_shared_libs.patch
COMMAND git apply ${PROJECT_SOURCE_DIR}/bump_cmake_min_version.patchSee the Miscellaneous Options in the docs:
Any of the other ..._COMMAND options can have additional commands appended to them by following them with as many COMMAND ... options as needed
There was a problem hiding this comment.
Yer, on the other hand I like having all of the 3 commands together wrapped as "the" patch command.
JonasVautherin
approved these changes
Jun 12, 2023
|
I think I want to clean up the commits a bit before merging. |
On Windows, the mavsdk_server unit tests don't link properly when building with shared libs. For whatever reason the transitive dependencies didn't get tracked from the protobu/grpc components all the way into the mavsdk_server unit tests. I suspect that either some symbols get stripped too early, or it has to do with the export visibility in MSVC. I couldn't quite get to the bottom of it but I realized that we could just simplify it by avoiding the little libraries and just adding all sources to the mavsdk_server library in the first place. Let's see if it works... Signed-off-by: Julian Oes <[email protected]>
This updates protobuf, gRPC, absl, and c-ares, and then regenerates the code using that. Signed-off-by: Julian Oes <[email protected]>
Signed-off-by: Julian Oes <[email protected]>
Signed-off-by: Julian Oes <[email protected]>
Signed-off-by: Julian Oes <[email protected]>
We now should have XCode 14.2 by default which should no longer require bitcode. Signed-off-by: Julian Oes <[email protected]>
Signed-off-by: Julian Oes <[email protected]>
julianoes
force-pushed
the
pr-simplify-linking
branch
from
da213dc
to
23917e0
Compare
This was referenced Jun 14, 2023
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On Windows, the mavsdk_server unit tests don't link properly when building with shared libs. For whatever reason the transitive dependencies didn't get tracked from the protobu/grpc components all the way into the mavsdk_server unit tests.
I suspect that either some symbols get stripped too early, or it has to do with the export visibility in MSVC.
I couldn't quite get to the bottom of it but I realized that we could just simplify it by avoiding the little libraries and just adding all sources to the mavsdk_server library in the first place.
Let's see if it works...
Edit: same problem...