Skip to content

Conversation

@Begasus
Copy link
Contributor

@Begasus Begasus commented Oct 23, 2025

Description

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

PR checklist

…ation

Signed-off-by: Luc Schrijvers <begasus@gmail.com>
@ronald-cron-arm ronald-cron-arm changed the title Use GNUInstallDirs CMAKE_INSTALL_INCLUDEDDIR path for headers install… Use GNUInstallDirs CMAKE_INSTALL_INCLUDEDDIR path for headers installation Oct 23, 2025
@ronald-cron-arm ronald-cron-arm self-requested a review October 23, 2025 10:13
Signed-off-by: Luc Schrijvers <begasus@gmail.com>
@Begasus
Copy link
Contributor Author

Begasus commented Oct 24, 2025

@ronald-cron-arm could you check if this is OK? If so I could copy this to the other PR's.

Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

Some formatting issue with the change log. Otherwise while working on CMAKE_INSTALL_INCLUDEDIR, isn't it the case that in library/CMakeLists.txt line 350 and following we should have:

    target_include_directories(${target}                                        
        PUBLIC $<BUILD_INTERFACE:${MBEDTLS_DIR}/include/>                       
               $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>                 
        PRIVATE ${MBEDTLS_DIR}/library/                                         
                # Needed to include psa_crypto_driver_wrappers.h                
                ${CMAKE_CURRENT_BINARY_DIR})

instead of

    target_include_directories(${target}                                        
        PUBLIC $<BUILD_INTERFACE:${MBEDTLS_DIR}/include/>                       
               $<INSTALL_INTERFACE:include/>                 
        PRIVATE ${MBEDTLS_DIR}/library/                                         
                # Needed to include psa_crypto_driver_wrappers.h                
                ${CMAKE_CURRENT_BINARY_DIR})

?

Co-authored-by: Ronald Cron <ronald.cron@arm.com>
Signed-off-by: Schrijvers Luc <begasus@gmail.com>
@Begasus
Copy link
Contributor Author

Begasus commented Oct 24, 2025

Some formatting issue with the change log. Otherwise while working on CMAKE_INSTALL_INCLUDEDIR, isn't it the case that in library/CMakeLists.txt line 350 and following we should have:

    target_include_directories(${target}                                        
        PUBLIC $<BUILD_INTERFACE:${MBEDTLS_DIR}/include/>                       
               $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>                 
        PRIVATE ${MBEDTLS_DIR}/library/                                         
                # Needed to include psa_crypto_driver_wrappers.h                
                ${CMAKE_CURRENT_BINARY_DIR})

instead of

    target_include_directories(${target}                                        
        PUBLIC $<BUILD_INTERFACE:${MBEDTLS_DIR}/include/>                       
               $<INSTALL_INTERFACE:include/>                 
        PRIVATE ${MBEDTLS_DIR}/library/                                         
                # Needed to include psa_crypto_driver_wrappers.h                
                ${CMAKE_CURRENT_BINARY_DIR})

?

I guess this should be it?

@Begasus Begasus force-pushed the mbedtls-3.6 branch 2 times, most recently from 63dff2b to 8bdde25 Compare October 24, 2025 10:40
Signed-off-by: Luc Schrijvers <begasus@gmail.com>
@ronald-cron-arm ronald-cron-arm added bug component-platform Portability layer and build scripts size-xs Estimated task size: extra small (a few hours at most) labels Oct 24, 2025
@ronald-cron-arm ronald-cron-arm added the needs-ci Needs to pass CI tests label Oct 24, 2025
@ronald-cron-arm ronald-cron-arm changed the title Use GNUInstallDirs CMAKE_INSTALL_INCLUDEDDIR path for headers installation Use GNUInstallDirs CMAKE_INSTALL_INCLUDEDIR path for headers installation Oct 24, 2025
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@ronald-cron-arm ronald-cron-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests labels Oct 24, 2025
@ronald-cron-arm ronald-cron-arm moved this from In Development to In Review in Non-roadmap pull requests Oct 24, 2025
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

LGTM

@github-project-automation github-project-automation bot moved this from In Review to Has Approval in Non-roadmap pull requests Oct 27, 2025
@minosgalanakis minosgalanakis added this pull request to the merge queue Oct 29, 2025
Merged via the queue into Mbed-TLS:mbedtls-3.6 with commit 2cd2fae Oct 29, 2025
3 checks passed
@github-project-automation github-project-automation bot moved this from Has Approval to Done in Non-roadmap pull requests Oct 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug component-platform Portability layer and build scripts needs-review Every commit must be reviewed by at least two team members, size-xs Estimated task size: extra small (a few hours at most)

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants