Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extend basic checks and C coding style check to framework files #9088

Open
wants to merge 4 commits into
base: development
Choose a base branch
from
Open

Extend basic checks and C coding style check to framework files #9088

wants to merge 4 commits into from
+37 −7

Conversation

ronald-cron-arm
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm commented May 2, 2024 •
edited

Description

Extend basic checks and C coding style checks to framework files
Progresses Mbed-TLS/mbedtls-framework#7

PR checklist

  • changelog not required, only check scripts are changed
  • 3.6 backport required, todo
  • 2.28 backport not required, no framework submodule in 2.28
  • tests not required

@ronald-cron-arm
Update framework submodule
72b9800 
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
@ronald-cron-arm
Extend basic checks of files to framework files
1e05deb 
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
@ronald-cron-arm ronald-cron-arm added the component-test Test framework and CI scripts label May 2, 2024
@ronald-cron-arm ronald-cron-arm added this to To Do in Roadmap Board for Mbed TLS via automation May 2, 2024
@ronald-cron-arm ronald-cron-arm moved this from To Do to In Development in Roadmap Board for Mbed TLS May 2, 2024
@ronald-cron-arm ronald-cron-arm added needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon labels May 2, 2024
@ronald-cron-arm ronald-cron-arm mentioned this pull request May 2, 2024
Open
4 tasks
@ronald-cron-arm
Extend C code style check to framework files
62a908d 
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
@minosgalanakis minosgalanakis mentioned this pull request May 3, 2024
Closed
@tom-cosgrove-arm
Copy link
Contributor

CI:

[2024-05-03T12:48:31.422Z] Running pylint ...
[2024-05-03T12:49:18.220Z] ************* Module check_files
[2024-05-03T12:49:18.220Z] tests/scripts/check_files.py:491:0: C0330: Wrong continued indentation (add 1 space).
[2024-05-03T12:49:18.220Z]                                                'ls-files', '-z'])
[2024-05-03T12:49:18.220Z]                                                ^| (bad-continuation)
[2024-05-03T12:49:18.220Z]
[2024-05-03T12:49:18.220Z] ------------------------------------
[2024-05-03T12:49:18.220Z] Your code has been rated at 10.00/10
[2024-05-03T12:49:18.220Z]
[2024-05-03T12:49:18.220Z]
[2024-05-03T12:49:18.220Z] Running mypy ...
[2024-05-03T12:49:18.220Z] pylint reported errors
[2024-05-03T12:49:23.485Z] Success: no issues found in 36 source files
[2024-05-03T12:49:24.047Z] ^^^^check_python_files: Lint: Python scripts: tests/scripts/check-python-files.sh -> 1^^^^

@ronald-cron-arm
Do not use --recurse-submodules
7661aa0 
On the CI, the git version when running on
Ubuntu 16.04 is 2.7 and it does not support
the "--recurse-submodules" option of
"git ls-files" thus do not use it.

Another argument to not use it is that
when TF-PSA-Crypto will be a submodule of
mbedtls we will not want check_files.py to
check the TF-PSA-Crypto files as well.

Signed-off-by: Ronald Cron <ronald.cron@arm.com>
@ronald-cron-arm ronald-cron-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-ci Needs to pass CI tests labels May 3, 2024
@ronald-cron-arm ronald-cron-arm moved this from In Development to In Review in Roadmap Board for Mbed TLS May 3, 2024
@tom-daubney-arm tom-daubney-arm removed the needs-reviewer This PR needs someone to pick it up for review label May 7, 2024
tom-daubney-arm
tom-daubney-arm approved these changes May 14, 2024
View reviewed changes
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.

This LGTM.

Testing wise I added a dummy.c to the framework submodule locally and committed it with some messy code in it. The altered scripts worked as expected on this new file, as well as the other pre-existing files in the submodule so I am happy that this is working correctly.

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

@tom-daubney-arm tom-daubney-arm tom-daubney-arm approved these changes

@davidhorstmann-arm davidhorstmann-arm Awaiting requested review from davidhorstmann-arm

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
component-test Test framework and CI scripts needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon
Projects
Roadmap Board for Mbed TLS
In Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ronald-cron-arm @tom-cosgrove-arm @tom-daubney-arm