Skip to content

Conversation

@davidhorstmann-arm
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm commented Nov 26, 2021

Allow the demo scripts dlopen_demo.sh and key_ladder_demo.sh to be run in out-of-tree builds, by requiring that they are run in the project's build directory. For in-tree builds, this is just the project root directory.

Status

READY

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

set -e -u

program="${0%/*}"/key_ladder_demo
program=./programs/psa/key_ladder_demo
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately this breaks

cd programs/psa
./key_ladder_demo.sh

which is a perfectly reasonable to try to run the script. So this way is nice for our CI, but not nice for our users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true, I tried to mitigate that with an error message (could be reworded to be more clear). It's not ideal, but is it too much of a problem to require the scripts to be run from a certain place, provided that it's made clear to the user?

Alternatively, I could:

  • Add some kind of --build-dir option to pass an out-of-tree path.
  • Do some kind of detection magic and cd to the top of the tree at the start.
  • Just ignore out-of-tree builds for the purposes of the demo scripts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Digging back, it looks like I implemented some kind of detection magic in #2698. How about building on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've preserved the original behaviour but added some code that searches if the executable is not found. This allows the script to be run from the build directory in an out-of-tree build.

@tom-daubney-arm
Copy link
Contributor

Hi, I was wondering what the status of this PR is? Is it something that is likely to be picked up? And is it still relevant or wanted? Thanks for any info

@tom-daubney-arm tom-daubney-arm added the historical-reviewing Currently reviewing (for legacy PR/issues) label Apr 14, 2023
@gilles-peskine-arm
Copy link
Contributor

This is still relevant and wanted. I have no idea when we'll have time for it. But we should keep it open since it hasn't bitrotted.

Allow demo scripts to be run from the build directory for out-of-tree
builds.

If the executable is not found in the source tree then search in the
current directory in case the script is being run from a build
directory.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
@davidhorstmann-arm
Copy link
Contributor Author

Apologies for the multi-year latency. I've addressed feedback and rebased on development, this should be ready for re-review.

@davidhorstmann-arm davidhorstmann-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels May 9, 2023
@tom-daubney-arm tom-daubney-arm added historical-reviewed Reviewed & agreed to keep legacy PR/issue and removed historical-reviewing Currently reviewing (for legacy PR/issues) labels May 9, 2023
@gilles-peskine-arm
Copy link
Contributor

Now that I look at the content again and not just the title, would this be superseded by #2698 ? That PR at least adds a common source file where the directory detection code lives.

@davidhorstmann-arm
Copy link
Contributor Author

would this be superseded by #2698 ?

As far as I can tell, no. That PR only works for in-tree builds. This one allows the scripts to be run from the build directory in an out-of-tree build.

@daverodgman daverodgman self-requested a review May 19, 2023 13:29
@tom-daubney-arm tom-daubney-arm self-requested a review May 19, 2023 13:51
Copy link
Contributor

@tom-daubney-arm tom-daubney-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.

@tom-daubney-arm tom-daubney-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 May 19, 2023
@daverodgman daverodgman merged commit 7f97675 into Mbed-TLS:development May 19, 2023
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 historical-reviewed Reviewed & agreed to keep legacy PR/issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants