Forward CMake variables to prebuilding dependencies #1161
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1161 +/- ##
==========================================
- Coverage 83.64% 83.63% -0.01%
==========================================
Files 57 57
Lines 5954 5953 -1
==========================================
- Hits 4980 4979 -1
Misses 974 974 ☔ View full report in Codecov by Sentry. |
sfod
changed the title
sfod
changed the title
| # on other platforms, the chances are you have to handle additional variables (like CMAKE_OSX_SYSROOT). Refer to | ||
| # https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html to update the list of handled variables, and | ||
| # then you can enable a new platform here. | ||
| if (NOT UNIX OR APPLE) |
There was a problem hiding this comment.
trivial: add parens to make this more clear
| if (NOT UNIX OR APPLE) | |
| if ((NOT UNIX) OR APPLE) |
| # Get list of optional or platform-specific variables that may affect build process. | ||
| function(aws_get_variables_for_prebuild_dependency AWS_CMAKE_PREBUILD_ARGS) | ||
| set(variables "") | ||
| set(variablesToIgnore CMAKE_INSTALL_PREFIX) |
There was a problem hiding this comment.
trivial: I don't see variablesToIgnore being used anywhere. Should it be deleted, or did we forget to do a check later?
|
|
||
| # The CMake variables below were chosen for Linux, BSD, and Android platforms. If you want to use the prebuild logic | ||
| # on other platforms, the chances are you have to handle additional variables (like CMAKE_OSX_SYSROOT). Refer to | ||
| # https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html to update the list of handled variables, and |
There was a problem hiding this comment.
👍 great docs on the thinking behind this, and what someone would need to look into if they're going to modify it in the future
| foreach(var ${vars}) | ||
| # Variables in this block make sense only in cross-compiling mode. The variable list is created from the CMake | ||
| # documentation on toolchains: https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html | ||
| # NOTE: Some variables are missed here (e.g. CMAKE_SYSROOT) because they can be set via toolchain file only. |
| # Other optional variables applicable both in cross-compiling and non-cross-compiling modes. | ||
| if (var STREQUAL "CMAKE_C_COMPILER" | ||
| OR var STREQUAL "CMAKE_CXX_COMPILER" | ||
| OR var STREQUAL "CMAKE_C_FLAGS" |
There was a problem hiding this comment.
Does AWS-LC use C++ anywhere? If so, we also need CMAKE_CXX_FLAGS. If not, maybe remove CMAKE_CXX_COMPILER?
There was a problem hiding this comment.
Oh man. Should we also worry about linker flags? https://cmake.org/cmake/help/latest/envvar/LDFLAGS.html
There was a problem hiding this comment.
Lots of food for thought in this doc:
https://cmake.org/cmake/help/latest/manual/cmake-env-variables.7.html
Issue #, if available:
When prebuilding dependencies, we need to pass certain CMake variables from parent cmake process to child cmake process (e.g. toolchain file or path to C compiler). CMake doesn't provide this functionality out of the box.
Description of changes:
This PR implements a new CMake function that checks CMake cache file and collects variables that may affect build process.
aws_prebuild_dependencyuses this new function, so no changes are required in the downstream projects.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.