Skip to content

Conversation

@gilles-peskine-arm
Copy link
Contributor

Make scripts/code_style.py easier to use by occasional contributors. In particular, insist on the version we require. Currently we have a warning but people ignore warnings.

PR checklist

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

  • changelog not required
  • backport TODO
  • tests manually only (maintenance script)

We know that using a different version of uncrustify produces different
results. So make that an error rather than a warning.

Also make the error output more helpful if uncrustify is not found.

Signed-off-by: Gilles Peskine <[email protected]>
This makes it easier to run the script on a machine where the
system-installed uncrustify is a different version.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm added 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 component-test Test framework and CI scripts priority-medium Medium priority - this can be reviewed as time permits size-xs Estimated task size: extra small (a few hours at most) labels Nov 13, 2023
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.

Looks good overall, with a minor tibit.

stderr=subprocess.PIPE)
return str(output, "utf-8").strip()
except FileNotFoundError:
sys.stderr.write('Fatal: command {} not found in PATH.\n'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very nice practise.

if args.fix:
# Fix mode
return fix_style(src_files)
return fix_style(args.uncrustify, src_files)
Copy link
Contributor

Choose a reason for hiding this comment

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

The default for args.uncrustify could be set to be to UNCRUSTIFY_EXE set at at line17.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UNCRUSTIFY_EXE shouldn't exist anymore... But I missed an occurrence. I'll fix that.

@gilles-peskine-arm gilles-peskine-arm added needs-work and removed 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 labels Nov 20, 2023
Before this commit, verify mode still hard-coded "uncrustify" as the command
name.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm added 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 and removed needs-work labels Nov 20, 2023
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

@Ryan-Everett-arm Ryan-Everett-arm self-requested a review November 23, 2023 14:39
@Ryan-Everett-arm Ryan-Everett-arm added the needs-backports Backports are missing or are pending review and approval. label Nov 23, 2023
@yuhaoth yuhaoth removed the needs-reviewer This PR needs someone to pick it up for review label Nov 30, 2023
@gilles-peskine-arm
Copy link
Contributor Author

Most of the affected code is now in the framework, so I raised a new pull request there: Mbed-TLS/mbedtls-framework#233

The all.sh part is now in #10493

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component-test Test framework and CI scripts needs-backports Backports are missing or are pending review and approval. needs-review Every commit must be reviewed by at least two team members, priority-medium Medium priority - this can be reviewed as time permits size-xs Estimated task size: extra small (a few hours at most)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants