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

Remove "mbedtls/" prefix in include file path #2319

Merged
merged 6 commits into from
Jan 24, 2019

Conversation

amisare
Copy link
Contributor

@amisare amisare commented Dec 28, 2018

Status

READY

Additional comments

Fix for file not found, which was reporting in header files. In header files at mbedlts/include folder, The include path has prefix "mbedtls/" may cause the header file not be found in the case of mbedtls as a dynamic/static library.

Todos

  • Tests
  • Documentation
  • Changelog updated
  • Backported

@amisare amisare changed the title Fix include file path Remove "mbedtls/" prefix in include file path Dec 28, 2018
@RonEld RonEld added CLA requested component-crypto Crypto primitives and low-level interfaces labels Dec 30, 2018
@RonEld
Copy link
Contributor

RonEld commented Dec 30, 2018

@amisare Thank you for your interest in Mbed TLS!
We are currently in the process of CLA acceptance, therefore labeling this PR as "CLA requested".

Note: this is related to #921 which was merged before these new includes were added, and to #857

Copy link
Contributor

@RonEld RonEld left a comment

Choose a reason for hiding this comment

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

The code Looks good to me.
I added my comment on the ChangeLog phrasing.

ChangeLog Outdated Show resolved Hide resolved
@simonbutcher simonbutcher added the component-platform Portability layer and build scripts label Jan 3, 2019
simonbutcher
simonbutcher previously approved these changes Jan 3, 2019
Copy link
Contributor

@simonbutcher simonbutcher left a comment

Choose a reason for hiding this comment

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

ChangeLog needs attention, but can be fixed in merge.

ChangeLog Outdated

Bugfix
* Fix for file not found, which was reporting in header files.
In header files at mbedlts/include folder, The include path has prefix
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo. Should be 'include/mbedtls', and 'the' shouldn't be capitalised. Not a blocker and can be fixed in merge.

ChangeLog Outdated
* Fix for file not found, which was reporting in header files.
In header files at mbedlts/include folder, The include path has prefix
"mbedtls/" may cause the header file not be found in the case of
mbedtls as a dynamic/static library.
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to credit @amisare. Not a blocker and can be fixed in merge.

@simonbutcher
Copy link
Contributor

@RonEld - I can fix the ChangeLog in merge. If you see nothing else as wrong, can you please approve? Thanks.

@simonbutcher
Copy link
Contributor

Needs backporting, but the same PR can be used on branch 2.16.

Co-Authored-By: amisare <243297288@qq.com>
@amisare
Copy link
Contributor Author

amisare commented Jan 4, 2019

@RonEld @sbutcher-arm I try to fix the ChangeLog. if it still has some error, you can modify it according to your rules.

@RonEld RonEld mentioned this pull request Jan 7, 2019
4 tasks
@simonbutcher simonbutcher added the approved Design and code approved - may be waiting for CI or backports label Jan 9, 2019
@simonbutcher simonbutcher merged commit 5c0b5b5 into Mbed-TLS:development Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports bug component-crypto Crypto primitives and low-level interfaces component-platform Portability layer and build scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants