-
Notifications
You must be signed in to change notification settings - Fork 3k
CMake cryptocell: fix include path #13441
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
Conversation
@@ -8,8 +8,7 @@ target_include_directories(mbed-os | |||
PUBLIC | |||
${CMAKE_CURRENT_SOURCE_DIR} | |||
${CMAKE_CURRENT_SOURCE_DIR}/include | |||
${CMAKE_CURRENT_SOURCE_DIR}/include/cryptocell310 | |||
${CMAKE_CURRENT_SOURCE_DIR}/include/internal | |||
${CMAKE_CURRENT_SOURCE_DIR}/include/cryptocell310/internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have seen header files included with paths starting a number of directories before the file is. To make sure all is covered, I include all directories.
So I would have:
${CMAKE_CURRENT_SOURCE_DIR}/include/cryptocell310/internal | |
${CMAKE_CURRENT_SOURCE_DIR}/include/cryptocell310 | |
${CMAKE_CURRENT_SOURCE_DIR}/include/cryptocell310/internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, fixing now, also internal should be private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a number of dirs that should be private. However, I thought we should do it once we start breaking down the library into small components.
It's fine if you do it now but I thought it could be done then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we will need to clean it. I put it back as it failed, investigating
03540d7
to
0b1f821
Compare
Pull request has been modified.
It looks like the build is passing, I'll rebase the PR to clean the second commit. |
@0xc0170, thank you for your changes. |
f2347b8
to
135bcc3
Compare
rebased, locally tested, passing |
Summary of changes
Broken after the restructure, Travis is red in another PR affected by the latest rebase
Impact of changes
Migration actions required
Documentation
Pull request type
Test results
Reviewers