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.

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

Move some scripts to framework submodule #8991

Conversation

davidhorstmann-arm
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm commented Mar 26, 2024

Move code_style.py and test code generation scripts to the framework submodule.

Depends on Mbed-TLS/mbedtls-framework#5

Note: The framework submodule is updated to the tip of Mbed-TLS/mbedtls-framework#5 to get the CI results. I can rebase this once that PR is merged.

PR checklist

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

@davidhorstmann-arm davidhorstmann-arm added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Mar 26, 2024
@davidhorstmann-arm davidhorstmann-arm force-pushed the move-scripts-to-framework branch from 72f1e73 to 53dee4d Compare March 26, 2024 17:31
@davidhorstmann-arm davidhorstmann-arm changed the title Move trivial scripts to framework submodule Move some scripts to framework submodule Mar 26, 2024
@davidhorstmann-arm davidhorstmann-arm added needs-ci Needs to pass CI tests and removed needs-work needs-ci Needs to pass CI tests labels Mar 26, 2024
@davidhorstmann-arm davidhorstmann-arm force-pushed the move-scripts-to-framework branch 2 times, most recently from 6c74b65 to 43057d0 Compare March 26, 2024 17:49
@davidhorstmann-arm davidhorstmann-arm added needs-ci Needs to pass CI tests and removed needs-ci Needs to pass CI tests labels Mar 26, 2024
@davidhorstmann-arm davidhorstmann-arm force-pushed the move-scripts-to-framework branch from 93d6b13 to 07033ad Compare March 27, 2024 14:56
@davidhorstmann-arm
Copy link
Contributor Author

[Force-pushed to update to version of submodule with full history]

@davidhorstmann-arm davidhorstmann-arm added needs-ci Needs to pass CI tests and removed needs-ci Needs to pass CI tests labels Mar 27, 2024
@Ryan-Everett-arm Ryan-Everett-arm self-requested a review March 27, 2024 17:59
Copy link
Contributor

@Ryan-Everett-arm Ryan-Everett-arm left a comment

Choose a reason for hiding this comment

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

I have found two comments in the codebase which still reference the old paths (sorry for not leaving my review comments on those lines - I don't think Github lets you comment on unchanged files). I am happy with everything else, though!

@davidhorstmann-arm
Copy link
Contributor Author

Thanks for catching these, should be addressed now!

@Ryan-Everett-arm Ryan-Everett-arm self-requested a review March 28, 2024 13:58
Copy link
Contributor

@Ryan-Everett-arm Ryan-Everett-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, thanks!

@davidhorstmann-arm davidhorstmann-arm added the needs-backports Backports are missing or are pending review and approval. label Apr 4, 2024
@Ryan-Everett-arm Ryan-Everett-arm self-requested a review April 4, 2024 17:08
Copy link
Contributor

@Ryan-Everett-arm Ryan-Everett-arm left a comment

Choose a reason for hiding this comment

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

There are some CI failures in the trivial 3.6 backport (see my comment here). I think that this branch needs to be rebased to pull in the addition of tests/scripts/generate_psa_wrappers.py so that we can change the references to mbedtls_dev in this file, otherwise the merge CI will likely fail.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
Signed-off-by: David Horstmann <david.horstmann@arm.com>
Signed-off-by: David Horstmann <david.horstmann@arm.com>
Signed-off-by: David Horstmann <david.horstmann@arm.com>
Add framework/scripts/*.py to the mypy line in check-python-files.sh

For some reason, this allows mypy to find the framework_dev module that
resides there, and it does not complain about other scripts.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
@davidhorstmann-arm davidhorstmann-arm force-pushed the move-scripts-to-framework branch from 0b18008 to eb6d69f Compare April 5, 2024 15:04
@davidhorstmann-arm davidhorstmann-arm removed the needs-backports Backports are missing or are pending review and approval. label Apr 5, 2024
Signed-off-by: David Horstmann <david.horstmann@arm.com>
Specifically:
* scripts/code_style.py -> framework/scripts/code_style.py
* tests/scripts/audit-validity-dates.py
  -> framework/scripts/audit-validity-dates.py

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

@Ryan-Everett-arm Ryan-Everett-arm left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. It seems that there are now two scripts in both mbedtls_dev and framework_dev, everything else looks good though!

These have been moved to the framework repo.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
@davidhorstmann-arm davidhorstmann-arm removed the needs-reviewer This PR needs someone to pick it up for review label Apr 8, 2024
Copy link
Contributor

@Ryan-Everett-arm Ryan-Everett-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, thanks!

@@ -55,14 +55,14 @@ elif [ "$1" = "--can-mypy" ]; then
fi

echo 'Running pylint ...'
$PYTHON -m pylint scripts/mbedtls_dev/*.py scripts/*.py tests/scripts/*.py || {
$PYTHON -m pylint framework/scripts/framework_dev/*.py scripts/*.py tests/scripts/*.py || {
Copy link
Contributor

Choose a reason for hiding this comment

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

Like in line 65, why not adding framework/scripts/*.py here as well?

@@ -24,7 +24,7 @@
pass

import scripts_path # pylint: disable=unused-import
from mbedtls_dev import build_tree
Copy link
Contributor

Choose a reason for hiding this comment

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

This file has to be modified to check the files that are in the framework submodule as well. It seems we just have to change bytes_output = subprocess.check_output(['git', 'ls-files', '-z']) to bytes_output = subprocess.check_output(['git', 'ls-files', '--recurse-submodules', '-z']).

@davidhorstmann-arm
Copy link
Contributor Author

Closing as superseded by more separate PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-preceding-pr Requires another PR to be merged first needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants