Skip to content

Support DESTDIR for install and add build-system test#10631

Open
yiwu0b11 wants to merge 13 commits intoMbed-TLS:developmentfrom
yiwu0b11:destdir_install_env_support
Open

Support DESTDIR for install and add build-system test#10631
yiwu0b11 wants to merge 13 commits intoMbed-TLS:developmentfrom
yiwu0b11:destdir_install_env_support

Conversation

@yiwu0b11
Copy link
Copy Markdown

@yiwu0b11 yiwu0b11 commented Mar 5, 2026

Description

Add support for DESTDIR env for build install.
Tests are added

Issue: #10627

PR checklist

Signed-off-by: Yi Wu <yi.wu2@arm.com>
@yiwu0b11 yiwu0b11 added needs-work needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) labels Mar 5, 2026
@VVD
Copy link
Copy Markdown

VVD commented Mar 7, 2026

Also libmbedcrypto.so.4.0.0 isn't stripped.

freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this pull request Mar 7, 2026
Not respect DESTDIR during create symlinks libmbedcrypto.so*.
Upstream issue: Mbed-TLS/mbedtls#10627

- Add fix for installing libmbedcrypto.so from upstream pool request:
  Mbed-TLS/mbedtls#10631
- Add strip for libmbedcrypto.so.
- Sort pkg-plist.

PR:		293653
Approved by:	Paavo-Einari Kaipila <pkaipila@gmail.com> (maintainer)
Signed-off-by: Yi Wu <yi.wu2@arm.com>
Signed-off-by: Yi Wu <yi.wu2@arm.com>
Signed-off-by: Yi Wu <yi.wu2@arm.com>
@yiwu0b11 yiwu0b11 marked this pull request as ready for review March 16, 2026 14:16
@yiwu0b11 yiwu0b11 added needs-review Every commit must be reviewed by at least two team members, and removed needs-work needs-ci Needs to pass CI tests labels Mar 16, 2026
@bjwtaylor bjwtaylor self-requested a review March 17, 2026 11:15
Comment thread tests/scripts/components-build-system.sh Outdated
Signed-off-by: Yi Wu <yi.wu2@arm.com>
bjwtaylor
bjwtaylor previously approved these changes Apr 8, 2026
Comment thread ChangeLog.d/cmake-destdir-instal.txt Outdated
Copy link
Copy Markdown

@bjwtaylor bjwtaylor left a comment

Choose a reason for hiding this comment

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

As per previous comments, I accidentally hit the improve instead of comment checkbox.

@bjwtaylor bjwtaylor self-requested a review April 8, 2026 08:53
Signed-off-by: Yi Wu <yi.wu2@arm.com>
@gilles-peskine-arm gilles-peskine-arm self-requested a review April 10, 2026 12:02
@gilles-peskine-arm gilles-peskine-arm added the priority-medium Medium priority - this can be reviewed as time permits label Apr 10, 2026
Copy link
Copy Markdown
Contributor

@gilles-peskine-arm gilles-peskine-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 for the fix. The non-regression test is fine but the code is a bit too complicated.

Comment thread ChangeLog.d/cmake-destdir-instal.txt Outdated
Comment thread ChangeLog.d/cmake-destdir-instal.txt Outdated
Comment thread tests/scripts/components-build-system.sh Outdated
Comment thread tests/scripts/components-build-system.sh Outdated
Comment thread tests/scripts/components-build-system.sh Outdated
Comment thread tests/scripts/components-build-system.sh Outdated
@gilles-peskine-arm gilles-peskine-arm added needs-work needs-backports Backports are missing or are pending review and approval. and removed needs-review Every commit must be reviewed by at least two team members, labels Apr 10, 2026
@gilles-peskine-arm
Copy link
Copy Markdown
Contributor

Note that we should side-port the test, and presumably the fix, to TF-PSA-Crypto.

Comment thread ChangeLog.d/cmake-destdir-instal.txt Outdated
Signed-off-by: Yi Wu <yi.wu2@arm.com>
Copy link
Copy Markdown
Contributor

@gilles-peskine-arm gilles-peskine-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 for what this is attempting to do. The CI is complaining and I haven't found the mistake that it found.

Comment thread tests/scripts/components-build-system.sh Outdated
Comment thread tests/scripts/components-build-system.sh
Signed-off-by: Yi Wu <yi.wu2@arm.com>
Signed-off-by: Yi Wu <yi.wu2@arm.com>
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Apr 14, 2026
Copy link
Copy Markdown
Contributor

@gilles-peskine-arm gilles-peskine-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 except the if structure


install_lib_path="$OUT_OF_SOURCE_DIR/stage/usr/${install_lib_subdir}"

if [[ "$OSTYPE" != darwin* ]]; then
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now the else branch is unreachable. Also, please avoid stacking negations, as they make the code harder to understand. In general, avoid if not x then A else B: write if x then B else A. (Keep if not x then A when there's no else clause.) So here:

if [[ "$OSTYPE" == "darwin" ]]; then …

Signed-off-by: Yi Wu <yi.wu2@arm.com>
Signed-off-by: Yi Wu <yi.wu2@arm.com>
Copy link
Copy Markdown
Contributor

@gilles-peskine-arm gilles-peskine-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 but we could simplify the validation of the link a bit

Comment thread tests/scripts/components-build-system.sh Outdated
Signed-off-by: Yi Wu <yi.wu2@arm.com>
Copy link
Copy Markdown
Contributor

@gilles-peskine-arm gilles-peskine-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

@github-project-automation github-project-automation Bot moved this from In Development to Has Approval in Non-roadmap pull requests Apr 16, 2026
@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Apr 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Design and code approved - may be waiting for CI or backports needs-backports Backports are missing or are pending review and approval. priority-medium Medium priority - this can be reviewed as time permits size-s Estimated task size: small (~2d)

Projects

Status: Has Approval

Development

Successfully merging this pull request may close these issues.

4 participants