Single source-of-trust for build variables #102
Merged
Conversation
This file contains hidden or 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
csegarragonz
commented
Oct 24, 2022
| get_serialised_cmake_env_vars(), | ||
| "cmake", | ||
| "-GNinja", | ||
| "-DFAASM_BUILD_TYPE={}".format(build_type), |
Contributor
Author
There was a problem hiding this comment.
This is already set by the toolchain file
0fd7c7e to
fe42914
Compare
…ned in python script
412a483 to
269216c
Compare
csegarragonz
commented
Oct 28, 2022
| } | ||
|
|
||
|
|
||
| def get_dict_as_cmake_vars(env_dict): |
Contributor
Author
There was a problem hiding this comment.
In Faasm, we can pass the FAASM_RUNTIME_ENV_DICT variables as CMake CACHE variables, which is preferrable than env. variables. Unfortunately, CACHE variables are not always read/loaded properly in toolchain files so in faasm/cpp we revert to using env. variables.
Shillaker
approved these changes
Oct 31, 2022
Collaborator
Shillaker
left a comment
There was a problem hiding this comment.
Nice this looks good.
Re. the stand-alone CMake toolchain file, I had thought that it would be nice to template it from the Python code, rather than have it rely on env vars, but this is definitely not in scope atm.
8 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.
In this PR I move all the variables needed to cross-compile C and C++ to WASM to a single Python file
faasmtools/build.py. All other files now read the values defined in this file. This change removes duplication of highly sensitive variables like the linker or compiler flags.I have decided to define all variables in a python file because we wrap calls to the compiler in python scripts anyway, which makes it easy to add the required variables.
The mechanism I have chosen to share variables between the python script and other files are environment variables. For example, a file like
WasiToolchain.cmakenow relies on a certain number of environment variables to be defined. Not having a self-contained*.cmakefile to include could make it less straight-forward to cross-compile a library outside offaasm/cpp, as you'd have to remember to prepend the env. vars before callingcmake, but works seamlessly in the scenario where we hold all the code to cross-compile libraries infaasm/cpp(as we have been doing withFFmpeg,ImageMagick, andTensorFlow, and I am planning to port LAMMPs and LULESH in the future as well).Another argument in favour of having all supported cross-compiled libraries in
faasm/cppis that this way it is easier to catch regressions, where a change in the toolchain breaks something that was building before. This, together with a minimal test in faasm/faasm, should ensure that we don't drop support to things we already support (even if the submodules rot away).In the process, I fix some formatting misalignments.
Closes #6