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 include in psa compliance tests to mbedtls config file #10041

Merged
merged 3 commits into from Mar 12, 2019

Conversation

@netanelgonen
Copy link
Contributor

commented Mar 11, 2019

Description

Fix issue #10035
change the way pal_crypto_config.h includes mbedtls configuration.
in this way, it will take the user file if needed

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@TaniaMirzin @orenc17 @avolinski

Release Notes

@0xc0170 0xc0170 requested review from TaniaMirzin, orenc17 and avolinski Mar 11, 2019

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-psa Mar 11, 2019

@0xc0170
Copy link
Member

left a comment

Not certain why all these additional spaces added though

@netanelgonen netanelgonen force-pushed the netanelgonen:Fix_include_mbedtls branch Mar 11, 2019

@netanelgonen netanelgonen force-pushed the netanelgonen:Fix_include_mbedtls branch to 24244b5 Mar 11, 2019

@netanelgonen netanelgonen changed the title Fix include mbedtls Fix include in psa compliance tests to mbedtls config file Mar 11, 2019

@alzix

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

@0xc0170,

Not certain why all these additional spaces added though

there is a tab hiding at the begging of the line

remove hidden tad
Co-Authored-By: netanelgonen <netanel.gonen@arm.com>
@@ -408,4 +404,4 @@

#include "pal_crypto_config_check.h"

#endif /* _PAL_CRYPTO_CONFIG_H_ */

This comment has been minimized.

Copy link
@alzix

alzix Mar 11, 2019

Contributor

what is the reason for this change?

This comment has been minimized.

Copy link
@NirSonnenschein

NirSonnenschein Mar 11, 2019

Contributor

Bug in the code, user config should augment the mbedtls one not replace it

#else
#include MBEDTLS_CONFIG_FILE
#endif
#include "mbedtls/config.h"

This comment has been minimized.

Copy link
@alzix

alzix Mar 11, 2019

Contributor

There are two changes here:

  1. change default header file include path
  2. remove an option to override this file from app sources

I'm fine with the first one, the second one is not clear to me.
What is the reason for removing the ability to use custom config file?

This comment has been minimized.

Copy link
@netanelgonen

netanelgonen Mar 11, 2019

Author Contributor

part 2 is actually made inside part 1
mbedtls overwriting the file, we should not do this.

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

starting CI while we wait for reviews

@ChiefBureaucraticOfficer
Copy link

left a comment

Meets criteria, PSA required. Approved.

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 11, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 1
Build artifacts

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

CI has passed and this is PM approved. @avolinski @TaniaMirzin please review

@cmonr cmonr merged commit 1471b4c into ARMmbed:master Mar 12, 2019

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9277 cycles (-1030 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the needs: review label Mar 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.