Move everest headers #153
Move everest headers #153
Conversation
bjwtaylor
force-pushed
the
move-everest-headers
branch
2 times, most recently
from
d6bf105 to
b388233
Compare
bjwtaylor
added
needs-review
scripts/test_psa_constant_names.py
Outdated
| @@ -172,7 +172,8 @@ def main(): | |||
| parser.add_argument('--include', '-I', | |||
| action='append', default=['tf-psa-crypto/include', | |||
| 'tf-psa-crypto/drivers/builtin/include', | |||
| 'tf-psa-crypto/drivers/everest/include', | |||
| 'tf-psa-crypto/drivers/everest/include/', | |||
There was a problem hiding this comment.
Has this change accidentally been split on to two lines? I assume it's meant to be tf-psa-crypto/drivers/everest/include/tf-psa-crypto/private rather than two separate elements in the list.
There was a problem hiding this comment.
It just looks like a typo, I've removed it.
There was a problem hiding this comment.
I need to remember that these reviews never show any lines under the one you click... anyway now in scripts/test_psa_constant_names.py there's + 'tf-psa-crypto/private/', which is a directory that doesn't exist...
I still think that this change should be replacing 'tf-psa-crypto/drivers/everest/include', with tf-psa-crypto/drivers/everest/include/tf-psa-crypto/private but as it's a long line perhaps
'tf-psa-crypto/drivers/everest/include/'
'tf-psa-crypto/private',
works I believe, which is how it was in the original version of this PR, just with the comma removed after /include/' so that it doesn't create a new list element and instead concatenates 'tf-psa-crypto/private' at the end of everest/include/ which I think was the intended result of this change originally?
There was a problem hiding this comment.
I've updated it, lets see if the ci passes.
There was a problem hiding this comment.
I think you just missed the / after include (or before the second tf-psa-crypto) in the updated patch - although I don't think the CI will pass here anyway as it's using the development mbedtls & tf-psa-crypto so naturally can't find anything in tf-psa-crypto/drivers/everest/include/tf-psa-crypto/private as the directory doesn't exist.
I this issue (the directory doesn't exist in development) is causing both the CI issues (i.e. the check_names issue and the compilation issue where it tries to include drivers/everest/include/tf-psa-crypto/private/ from the change in psa_stoage.py) - so presumably we would want this patch changed to accommodate both scenarios (everest headers are in drivers/everest/include or drivers/everest/include/tf-psa-crypto/private) and then can remove the old functionality in a later patch...
There was a problem hiding this comment.
I think this is a duplicate @gilles-peskine-arm comment. Basically we will need both the 3.6 paths and the new path present or the CI will fail with the older version. I'll resolve the one below and hopefully that will resolve both.
Signed-off-by: Ben Taylor <[email protected]>
Signed-off-by: Ben Taylor <[email protected]>
Signed-off-by: Ben Taylor <[email protected]>
Signed-off-by: Ben Taylor <[email protected]>
bjwtaylor
force-pushed
the
move-everest-headers
branch
from
42cf70d to
1ef13c8
Compare
Signed-off-by: Ben Taylor <[email protected]>
Signed-off-by: Ben Taylor <[email protected]>
Signed-off-by: Ben Taylor <[email protected]>
Signed-off-by: Ben Taylor <[email protected]>
gilles-peskine-arm
added
priority-high
github-project-automation
bot
moved this to In Development
in Roadmap pull requests (new board)
| @@ -703,8 +703,8 @@ def comprehensive_parse(self): | |||
| "include/tf-psa-crypto/*.h", | |||
| "include/mbedtls/*.h", | |||
| "drivers/builtin/include/mbedtls/*.h", | |||
| "drivers/everest/include/everest/everest.h", | |||
There was a problem hiding this comment.
In the framework, we need to keep the old paths everywhere, since they're still relevant for 3.6. It doesn't matter if some of these patterns match nothing in a given branch (except for human readers who might not think of the LTS branches, for which a comment could be helpful).
scripts/test_psa_constant_names.py
Outdated
| @@ -172,7 +172,8 @@ def main(): | |||
| parser.add_argument('--include', '-I', | |||
| action='append', default=['tf-psa-crypto/include', | |||
| 'tf-psa-crypto/drivers/builtin/include', | |||
| 'tf-psa-crypto/drivers/everest/include', | |||
| 'tf-psa-crypto/drivers/everest/include' | |||
There was a problem hiding this comment.
| 'tf-psa-crypto/drivers/everest/include' | |
| 'tf-psa-crypto/drivers/everest/include,' |
gilles-peskine-arm
added
needs-work
and removed
needs-review
Signed-off-by: Ben Taylor <[email protected]>
Signed-off-by: Ben Taylor <[email protected]>
Description
Move everest headers
PR checklist