Applying all patches inside patches directory #79
Conversation
…m explicitly in main script. - Moved unused patches to graveyard.
do_everything.sh
Outdated
|
|
||
| ## Demo Application specific patches | ||
|
|
||
| run_cmd apply_patches $my_loc/patches/source_patches $prefix |
There was a problem hiding this comment.
Why the additional subdirectory source_patches? I would just leave them in patches and only move the unused ones to patches/graveyard/ or delete them directly (they can still be found via the Git history).
There was a problem hiding this comment.
There's a single patch that has to be applied in a different directory: android.toolchain.cmake.patch; in principle that one has to be in a separate folder.
Perhaps we can move that one to files directory and move all the patches up one level as you propose?
There was a problem hiding this comment.
👍
An alternative approach I took in https://github.com/Intermodalics/ros_android_private/pull/17 is to commit a wrapper android.toolchain.cmake (or a template) and apply changes as CMake code after having included the original file, instead of patching the file in the Android NDK installation folder.
Such a custom and, if necessary, generated toolchain file could also replace most of the cmake options passed in build_catkin_workspace.sh:103ff and utils.sh:21 and be exposed to the user to be used in overlay workspaces (installed to target).
| apply_patch $my_loc/patches/pluginlib.patch | ||
| # apply_patch image_transport # to fix faulty export plugins | ||
| apply_patch $my_loc/patches/image_transport.patch | ||
| fi |
There was a problem hiding this comment.
This special case is not handled anymore. In combination with do_everything.sh:120ff if use_pluginlib is disabled in config.sh:12, applying the patches would break pluginlib.
The alternative would be to ignore package pluginlib and all its dependees when building with use_pluginlib=0, because the unpatched version would not work anyway on Android if I am not mistaken.
There was a problem hiding this comment.
AFAIU the unpatched version won't work; my rationale is that if you don't want pluginlib, applying a few extra packages doesn't add too much overhead, and you won't be able to use pluginlib anyway.
I agree that for correctness we should ignore pluginlib and deps, but on second thought I'd just drop the flag and build pluginlib support always, as it really doesn't add too much build time to the overall, and the flag just adds some more complexity to the scripts. WDYT?
There was a problem hiding this comment.
Always building with pluginlib support enabled would be fine for me.
…le that includes the one inside NDK.
|
@meyerj I removed I'm running a build from scratch right now with these changes and so far it's going well; PTAL. EDIT: build finished without problems. |
jubeira
changed the title
| # Debug and release flags. | ||
| list(REMOVE_ITEM ANDROID_COMPILER_FLAGS -g) | ||
| list(APPEND ANDROID_COMPILER_FLAGS_DEBUG -g) | ||
| list(APPEND ANDROID_COMPILER_FLAGS_RELWITHDEBINFO -g ${ANDROID_COMPILER_FLAGS_RELEASE}) |
There was a problem hiding this comment.
It is not sufficient to change ANDROID_COMPILER_FLAGS_*. The values have already been copied into CMAKE_C_FLAGS_*, CMAKE_CXX_FLAGS_*,CMAKE_ASM_FLAGS_* further down in the included toolchain file and are not applied directly. So the replacements have to be applied to all of those variables:
## Build without debug symbols in Release mode and add RelWithDebInfo mode
foreach(_var ANDROID_COMPILER_FLAGS CMAKE_C_FLAGS CMAKE_CXX_FLAGS CMAKE_ASM_FLAGS)
string(REPLACE "-g " "" ${_var} "${${_var}}")
set(${_var}_DEBUG "-g ${${_var}_DEBUG}")
set(${_var}_RELWITHDEBINFO "-g ${${_var}_RELEASE}")
endforeach()
unset(_var)There was a problem hiding this comment.
Thanks for the observation; I'll open a PR with the modification.
See #76; this PR allows applying patches over the codebase without explicitly listing them in the main script, making it easier to maintain.