Skip to content
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

Fix FAASM_SGX flag #596

Merged
merged 2 commits into from
Feb 22, 2022
Merged

Fix FAASM_SGX flag #596

merged 2 commits into from
Feb 22, 2022

Conversation

csegarragonz
Copy link
Collaborator

@csegarragonz csegarragonz commented Feb 21, 2022

In a previous PR (#590) I had to overwrite the FAASM_SGX CMake variable to set it to ON explicitly to make sure the SGX code was being built and tested. My fix was too brute, and broke the option to disable the SGX build, which is why the flag was introduced in the first place.

The crux of the problem is that, even though the default value is now ON, it is cached (in CMakeCache.txt) as OFF from when the container image was built. As a consequence, if we don't set FAASM_SGX to ON explicitely, it will be set to OFF. As a proof of concept, I print all cached variables in an action run, where we can see that FAASM_SGX is set to OFF.

One possible workaround is to re-build the container images with a different default value, so that it gets properly cached. Another workaround (the one I have chosen) is to unset the variable from the CMake cache so that it is either the default (ON) or OFF if overriden from the command line.

Relevant links:

@csegarragonz csegarragonz force-pushed the fix-sgx-flag branch 2 times, most recently from 6a23e88 to d8e8b94 Compare February 21, 2022 16:33
@csegarragonz csegarragonz self-assigned this Feb 21, 2022
@csegarragonz
Copy link
Collaborator Author

@KubaSz Could you check what I say makes any sense at all?

@eigenraven
Copy link
Collaborator

Because you're already using python scripts for everything, imo the best solution is to always pass -DFAASM_SGX=value to cmake, on/off and just not relying on the default value.

The cache keeps whatever was last used and really shouldn't be overwritten by the cmake scripts, as it's intended for user configuration and cmake will rerun itself with no extra arguments to regenerate build files keeping old cache settings.

Personally I've been using CMake presets (https://cmake.org/cmake/help/v3.22/manual/cmake-presets.7.html, https://github.com/auto-ndp/faasm/blob/master/CMakePresets.json) as they're very convenient for local builds and integrate well with vscode/clion unlike the python-based configuration, you may want to look into those to have a couple of pre-set configuration sets like docker, tests, local build, etc.

@csegarragonz
Copy link
Collaborator Author

Ok thanks, I was not sure about tampering with the cache manually. To fix the current bug I think it makes more sense to just amend the CLI script.

Copy link
Collaborator

@Shillaker Shillaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, good catch!

@csegarragonz csegarragonz merged commit 00b2bb2 into main Feb 22, 2022
@csegarragonz csegarragonz deleted the fix-sgx-flag branch February 22, 2022 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants