gh-Mbed-TLS/mbedtls-framework
Watch
1
0
Fork
You've already forked mbedtls-framework
 0
Code Issues 39 Pull requests 42 Releases Activity

all.sh: fail if a file is missing on a "not grep ..." line #268

Open
gilles-peskine-arm wants to merge 2 commits from gilles-peskine-arm/not-not-grep-framework into main
pull from: gilles-peskine-arm/not-not-grep-framework
merge into: gh-Mbed-TLS:main
Conversation 10 Commits 2 Files changed 1 +36
gilles-peskine-arm commented 2026-01-09 12:24:03 -06:00 (Migrated from github.com)

Fixes https://github.com/Mbed-TLS/mbedtls-framework/issues/266

This is transparent since not grep automatically redirects to the new function. I'm not sure whether this should be transitional or if we should keep this automation.

Needs preceding PR: https://github.com/Mbed-TLS/mbedtls/pull/10559, https://github.com/Mbed-TLS/mbedtls/pull/10562, TODO to fix the problems that this reveals.

PR checklist

  • TF-PSA-Crypto PR provided # | not required because: transparent change
  • development PR provided # | not required because: transparent change
  • 3.6 PR provided # | not required because: transparent change
Fixes https://github.com/Mbed-TLS/mbedtls-framework/issues/266 This is transparent since `not grep` automatically redirects to the new function. I'm not sure whether this should be transitional or if we should keep this automation. Needs preceding PR: https://github.com/Mbed-TLS/mbedtls/pull/10559, https://github.com/Mbed-TLS/mbedtls/pull/10562, TODO to fix the problems that this reveals. ## PR checklist - [x] **TF-PSA-Crypto PR** provided # | not required because: transparent change - [x] **development PR** provided # | not required because: transparent change - [x] **3.6 PR** provided # | not required because: transparent change
mpg (Migrated from github.com) reviewed 2026-01-13 04:01:08 -06:00
mpg (Migrated from github.com) left a comment
No description provided.
LGTM, thanks!
mpg commented 2026-01-13 04:03:05 -06:00 (Migrated from github.com)

This is transparent since not grep automatically redirects to the new function. I'm not sure whether this should be transitional or if we should keep this automation.

I think this is nice and should stay, as it avoids a possibility of error.

> This is transparent since `not grep` automatically redirects to the new function. I'm not sure whether this should be transitional or if we should keep this automation. I think this is nice and should stay, as it avoids a possibility of error.
mpg commented 2026-01-13 04:23:49 -06:00 (Migrated from github.com)

The latest test run has failures in the following components:

tf-psa-crypto:
tf_psa_crypto_build_custom_config_file

tls-development:
test_full_block_cipher_psa_dispatch_static_keystore
test_full_no_ccm_star_no_tag

tls-mbedtls-3.6:
test_aesni
test_aesni_m32

I'm going to raise PRs for each of them.

The latest test run has failures in the following components: ``` tf-psa-crypto: tf_psa_crypto_build_custom_config_file tls-development: test_full_block_cipher_psa_dispatch_static_keystore test_full_no_ccm_star_no_tag tls-mbedtls-3.6: test_aesni test_aesni_m32 ``` I'm going to raise PRs for each of them.
mpg (Migrated from github.com) reviewed 2026-01-22 02:30:01 -06:00
mpg (Migrated from github.com) left a comment

LGTM but the CI does not seem to agree.

LGTM but the CI does not seem to agree.
gilles-peskine-arm commented 2026-01-22 08:52:11 -06:00 (Migrated from github.com)

Let's see the CI now that more fixes have been merged.

Let's see the CI now that more fixes have been merged.
mpg commented 2026-01-23 02:51:55 -06:00 (Migrated from github.com)

Ok, down to two failing components:

  • test_block_cipher_no_decrypt_aesni - got broken by the merge of https://github.com/Mbed-TLS/mbedtls/pull/10507, obvious fix: update to the cmake paths.
  • test_block_cipher_no_decrypt_aesce_armcc - looks like I fixed the wrong problem (updated to the cmake paths without even thinking), need more though to understand what the actual problem was before fixing it this time.
Ok, down to two failing components: - `test_block_cipher_no_decrypt_aesni` - got broken by the merge of https://github.com/Mbed-TLS/mbedtls/pull/10507, obvious fix: update to the cmake paths. - `test_block_cipher_no_decrypt_aesce_armcc` - looks like [I fixed the wrong problem](https://github.com/Mbed-TLS/mbedtls/pull/10507) (updated to the cmake paths without even thinking), need more though to understand what the actual problem was before fixing it this time.
valeriosetti (Migrated from github.com) requested changes 2026-07-30 07:09:05 -05:00
scripts/all-core.sh
valeriosetti (Migrated from github.com) commented 2026-07-30 07:06:39 -05:00

this seems like a comment to me. If so, can we remove it?

this seems like a comment to me. If so, can we remove it?
scripts/all-core.sh
@ -805,0 +812,4 @@
declare ret=0
grep -H "$@" || ret=$?
if [[ $ret -eq 0 ]]; then
# A match was found, and displayed on stdout.
valeriosetti (Migrated from github.com) commented 2026-07-30 07:08:02 -05:00

Question for my limited knowledge of bash: this trick instead of false is because we want to return the error code given by grep, right?

Question for my limited knowledge of bash: this trick instead of `false` is because we want to return the error code given by `grep`, right?
gilles-peskine-arm (Migrated from github.com) reviewed 2026-07-30 11:27:25 -05:00
scripts/all-core.sh
gilles-peskine-arm (Migrated from github.com) commented 2026-07-30 11:27:25 -05:00

I've rebased on top of main because an old branch wouldn't pass the CI. While I was at it, I amended the commit that added this spurious line.

I've rebased on top of `main` because an old branch wouldn't pass the CI. While I was at it, I amended the commit that added this spurious line.
gilles-peskine-arm (Migrated from github.com) reviewed 2026-07-30 11:29:08 -05:00
scripts/all-core.sh
@ -805,0 +812,4 @@
declare ret=0
grep -H "$@" || ret=$?
if [[ $ret -eq 0 ]]; then
# A match was found, and displayed on stdout.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-30 11:29:08 -05:00

That's right. There's no standard shell command that exits itself with a specified status, only the exit command which exits the shell. So to get the effect of a command exiting with a given status, I use the exit builtin in a subshell.

That's right. There's no standard shell command that exits itself with a specified status, only the `exit` command which exits the shell. So to get the effect of a command exiting with a given status, I use the `exit` builtin in a subshell.
valeriosetti (Migrated from github.com) approved these changes 2026-07-31 04:47:07 -05:00
This pull request is broken due to missing fork information.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'gilles-peskine-arm/not-not-grep-framework:gilles-peskine-arm/not-not-grep-framework'
git switch 'gilles-peskine-arm/not-not-grep-framework'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'main'
git merge --no-ff 'gilles-peskine-arm/not-not-grep-framework'
git switch 'gilles-peskine-arm/not-not-grep-framework'
git rebase 'main'
git switch 'main'
git merge --ff-only 'gilles-peskine-arm/not-not-grep-framework'
git switch 'gilles-peskine-arm/not-not-grep-framework'
git rebase 'main'
git switch 'main'
git merge --no-ff 'gilles-peskine-arm/not-not-grep-framework'
git switch 'main'
git merge --squash 'gilles-peskine-arm/not-not-grep-framework'
git switch 'main'
git merge --ff-only 'gilles-peskine-arm/not-not-grep-framework'
git switch 'main'
git merge 'gilles-peskine-arm/not-not-grep-framework'
git push origin 'main'
Sign in to join this conversation.
Reviewers
No reviewers
valeriosetti
Labels
Clear labels   
approved
Design and code approved - may be waiting for CI or backports

  
bug
Something isn't working

  
dependencies
Pull requests that update a dependency file

  
DO-NOT-MERGE

   
enhancement
New feature or request

  
good-first-issue
Good for newcomers

  
help-wanted
This issue is not being actively worked on, but PRs welcome.

  
historical-reviewed
Reviewed & agreed to keep legacy PR/issue

  
historical-reviewing
Currently reviewing (for legacy PR/issues)

  
needs-adoption
stalled PR that someone should pick up and complete

  
needs-ci
Needs to pass CI tests

  
needs-design-approval

   
needs-info
An issue or PR which needs further info from the reporter / author

  
needs-preceding-pr
Requires another PR to be merged first

  
needs-review
Every commit must be reviewed by at least two team members.

  
needs-reviewer
This PR needs someone to pick it up for review

  
needs-work

   
priority-high
High priority - will be reviewed soon

  
priority-low
Low priority - this may not receive review soon

  
priority-medium
Medium priority - this can be reviewed as time permits

  
priority-scheduled
This PR is big - it will require time to be scheduled for review

  
priority-very-high
Highest priority - prioritise this over other review work

  
single-reviewer
This PR qualifies for having only one reviewer

  
size-l
Estimated task size: large (2w+)

  
size-m
Estimated task size: medium (~1w)

  
size-s
Estimated task size: small (~2d)

  
size-xs
Estimated task size: extra small (a few hours at most)

No labels
approved
bug
dependencies
DO-NOT-MERGE
enhancement
good-first-issue
help-wanted
historical-reviewed
historical-reviewing
needs-adoption
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
single-reviewer
size-l
size-m
size-s
size-xs
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-Mbed-TLS/mbedtls-framework!268
No description provided.