Use GNUInstallDirs CMAKE_INSTALL_INCLUDEDDIR path for headers installation

[Begasus]

Description

Using a hardcoded path like "include" for headers path doesn't follow GNUInstallDirs method.

PR checklist

• changelog provided
• development PR provided here
• TF-PSA-Crypto PR provided | Use GNUIstalldir CMAKE_INSTALL_INCLUDEDIR for headers installation TF-PSA-Crypto#547
• framework PR not required
• 3.6 PR provided Use GNUInstallDirs CMAKE_INSTALL_INCLUDEDIR path for headers installation #10470
• tests not required because: The installation process is exercised by programs/test/cmake_package_install.

[ronald-cron-arm]

Thanks for this. We also have "bin" hard-coded for the installation of program and test executables. We will fix that as well.

[ronald-cron-arm]

And please add a "Signed-off-by:" line to your commit message.

[Begasus]

Thanks for this. We also have "bin" hard-coded for the installation of program and test executables. We will fix that as well.

Hard coded "bin" isn't really a problem as that is covered in our $PATH, so no changes needed there.

And please add a "Signed-off-by:" line to your commit message.

Should I push this in a separate commit or just amend it to this commit?
EDIT: added in amend, don't think it makes much sense to add a separate commit for it.

[ronald-cron-arm]

Should I push this in a separate commit or just amend it to this commit?

Just amend the commit message of your commit.

[Begasus]

For the record, checked a build for 3.6.5 which went fine, test case succeeded with zero errors, I'm just not sure where ts-psa-crypto is used?

[ronald-cron-arm]

For the record, checked a build for 3.6.5 which went fine, test case succeeded with zero errors, I'm just not sure where ts-psa-crypto is used?

There is no tf-psa-crypto submodule in 3.6.5; it was introduced in 4.0.

[ronald-cron-arm]

Our DCO check is still not happy:
Commit sha: 268dbd2, Author: Luc Schrijvers, Committer: Luc Schrijvers; Expected "Luc Schrijvers begasus@gmail.com", but got "Schrijvers Luc begasus@gmail.com".

[Begasus]

Our DCO check is still not happy: Commit sha: 268dbd2, Author: Luc Schrijvers, Committer: Luc Schrijvers; Expected "Luc Schrijvers begasus@gmail.com", but got "Schrijvers Luc begasus@gmail.com".

Haven't used it that much (guess you got that), will update. :)

[Begasus]

I guess this should go into the upstream source repositories?

diff --git a/3rdparty/everest/CMakeLists.txt b/3rdparty/everest/CMakeLists.txt
index e0e5ade..f105c89 100644
--- a/3rdparty/everest/CMakeLists.txt
+++ b/3rdparty/everest/CMakeLists.txt
@@ -29,7 +29,7 @@ endif()
 if(INSTALL_MBEDTLS_HEADERS)
 
   install(DIRECTORY include/everest
-    DESTINATION include
+    DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
     FILE_PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
     DIRECTORY_PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE
     FILES_MATCHING PATTERN "*.h")
diff --git a/3rdparty/p256-m/CMakeLists.txt b/3rdparty/p256-m/CMakeLists.txt
index 2ef0d48..884002e 100644
--- a/3rdparty/p256-m/CMakeLists.txt
+++ b/3rdparty/p256-m/CMakeLists.txt
@@ -27,7 +27,7 @@ endif()
 if(INSTALL_MBEDTLS_HEADERS)
 
   install(DIRECTORY :${CMAKE_CURRENT_SOURCE_DIR}
-    DESTINATION include
+    DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
     FILE_PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
     DIRECTORY_PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE
     FILES_MATCHING PATTERN "*.h")

[ronald-cron-arm]

Your PR is against the development branch. As you are using 3.6.5, you would need to raise a PR against the mbedtls-3.6 branch. In that PR you indeed need changes in the 3rdparty directory as well (which does not exist anymore in development).

[ronald-cron-arm]

Our DCO check is still not happy: Commit sha: 268dbd2, Author: Luc Schrijvers, Committer: Luc Schrijvers; Expected "Luc Schrijvers begasus@gmail.com", but got "Schrijvers Luc begasus@gmail.com".

Haven't used it that much (guess you got that), will update. :)

Thank you, our DCO check is happy now.

[Begasus]

Your PR is against the development branch. As you are using 3.6.5, you would need to raise a PR against the mbedtls-3.6 branch. In that PR you indeed need changes in the 3rdparty directory as well (which does not exist anymore in development).

For 3.6.5 we can keep the patch downstream, unless branch 3.6 will still see updates (eg 3.6.7 or up) I guess this could go into a separate PR? I also presume development branch targets 4.* releases?

[ronald-cron-arm]

Your PR is against the development branch. As you are using 3.6.5, you would need to raise a PR against the mbedtls-3.6 branch. In that PR you indeed need changes in the 3rdparty directory as well (which does not exist anymore in development).

For 3.6.5 we can keep the patch downstream, unless branch 3.6 will still see updates (eg 3.6.7 or up) I guess this could go into a separate PR? I also presume development branch targets 4.* releases?

Yes, this would be a different PR, targeting the branch mbedtls-3.6.
Yes, the development branch is now the branch from which we will build the 4.* releases.
I am pretty sure we will do a 3.6.6 release thus your changes would make it there.

[Begasus]

I am pretty sure we will do a 3.6.6 release thus your changes would make it there.

PR created, hope I got it right, used to making changes on master branches. :)

[ronald-cron-arm]

I am pretty sure we will do a 3.6.6 release thus your changes would make it there.

PR created, hope I got it right, used to making changes on master branches. :)

Yes this looks good, thanks.

[Begasus]

Checked a build for 4.0.0, seems some more include path(s) are involved, had to do some python/rust magic (adding requirements on our side) for it also (compared to 3.6.5).

[Begasus]

The other part of the patch is for TF-SPA-CRYPTO, should fork and do a PR upstream there.

[ronald-cron-arm]

This looks good to me. We just need a change log, a file *.txt in ChangeLog.d containing something like:

Bugfix
    * CMake now installs headers to `CMAKE_INSTALL_INCLUDEDIR` instead of the hard-coded `include` directory.

[Begasus]

This looks good to me. We just need a change log, a file *.txt in ChangeLog.d containing something like:

Bugfix
    * CMake now installs headers to `CMAKE_INSTALL_INCLUDEDIR` instead of the hard-coded `include` directory.

I'll try to finish up on that today, thanks so far!

[ronald-cron-arm]

LGTM, thanks.

[minosgalanakis]

LGTM