Skip to content

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

Merged
merged 1 commit into from
Aug 17, 2020

Conversation

0xc0170
Copy link
Contributor

@0xc0170 0xc0170 commented Aug 17, 2020

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

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


@@ -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
Copy link
Collaborator

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:

Suggested change
${CMAKE_CURRENT_SOURCE_DIR}/include/cryptocell310/internal
${CMAKE_CURRENT_SOURCE_DIR}/include/cryptocell310
${CMAKE_CURRENT_SOURCE_DIR}/include/cryptocell310/internal

Copy link
Contributor Author

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

Copy link
Collaborator

@hugueskamba hugueskamba Aug 17, 2020

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.

Copy link
Contributor Author

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

hugueskamba
hugueskamba previously approved these changes Aug 17, 2020
@mergify mergify bot added needs: work and removed needs: CI labels Aug 17, 2020
@mergify mergify bot dismissed hugueskamba’s stale review August 17, 2020 09:37

Pull request has been modified.

@0xc0170
Copy link
Contributor Author

0xc0170 commented Aug 17, 2020

It looks like the build is passing, I'll rebase the PR to clean the second commit.

@ciarmcom ciarmcom requested a review from a team August 17, 2020 10:00
@ciarmcom
Copy link
Member

@0xc0170, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

@0xc0170
Copy link
Contributor Author

0xc0170 commented Aug 17, 2020

rebased, locally tested, passing

@0xc0170 0xc0170 merged commit ed3c766 into ARMmbed:feature-cmake Aug 17, 2020
@0xc0170 0xc0170 deleted the fix_nrf_crypto branch August 17, 2020 11:21
@mergify mergify bot removed the ready for merge label Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants