Refactor scripts to use config.py instead of config.pl #178

Harry-Ramsey · 2024-10-07T09:35:13Z

This commit refactors Windows testing scripts to use config.py instead of config.pl because config.pl is being removed from the Mbed TLS repository.

This is required for issue in Mbed TLS: 9663
This is required for pull request in Mbed TLS: 9666

Test runs on 8e4283c:

development: Internal CI
mbedtls-2.28: OpenCI

gilles-peskine-arm · 2024-10-07T12:15:47Z

resources/windows/windows_testing.py

        ))
        try:
            enable_output = subprocess.run(
-                [self.perl_command, self.config_pl_location, "full"],
+                [self.config_py_location, "full"],


Don't we need to invoke the Python interpreter explicitly? Windows doesn't understand shebang lines, and I don't know if our CI systems are configured to have *.py files executable via python.exe.

I have resolved this now, I still need to test it however Docker on MacOS is proving to be a bit troublesome.

gilles-peskine-arm · 2024-10-07T12:17:39Z

resources/windows/windows_testing.py

        self.selftest_exe = "selftest.exe"
        self.mingw_command = "mingw32-make"
        self.git_command = "git"
-        self.perl_command = "perl"


Historical note: we need this script to run with all maintained branches, not just the latest. The last supported branch of Mbed TLS that had only config.pl and not config.py was 2.16. The oldest still-supported branch is 2.28, and it has config.py. Therefore it's ok to remove support for branches that don't have config.py.

Harry-Ramsey · 2024-11-13T09:47:33Z

I haven't been able to fully test this due to various docker issues with MacOS. Would anyone be able to run this on their docker setup to ensure it passes successfully?

mpg

Looks good to me on code inspection.

What's missing IMO, rather than local testing with docker on someone's machine, is testing on the actual CI, because that's what we ultimately care about.

mpg · 2024-11-14T10:35:42Z

@Harry-Ramsey I'm not sure if we have documentation about this, but I think what you want to do is:

go to https://jenkins-mbedtls.oss.arm.com/job/mbedtls-release-ci-testing/
(log in if you're not logged in already)
click "build with parameters"
check at least "windows testing"
push your branch to this repo (eg git push origin refactor-confif-pl-config-py-main:dev/harry/refactor-confif-pl-config-py-main)
select it under "TEST_BRANCH"
start the job, add a link to it in the PR description, and have a look at the results once it completes
do the same for the Internal CI.

(Disclaimer: I don't test changes on mbedtls-test that often - if I got it wrong, @gilles-peskine-arm or @bensze01 should be able to correct me.)

This commit refactors Windows testing scripts to use config.py instead of config.pl because config.pl is being removed from the Mbed TLS repository. Signed-off-by: Harry Ramsey <[email protected]>

Harry-Ramsey · 2024-11-14T11:24:56Z

@mpg I believe I do not have permission to push to mbedtls-test as git is giving me a permission error.

mpg · 2024-11-14T11:52:58Z

Can you try again? I believe permissions have been fixed now.

Harry-Ramsey · 2024-11-14T12:02:40Z

Thanks, building now. The result can be found at 729.

gilles-peskine-arm · 2024-11-18T12:45:32Z

I'm not sure if we have documentation about this,

It's waiting for review ☹️ #126

if I got it wrong, @gilles-peskine-arm or @bensze01 should be able to correct me

That looks right to me.

gilles-peskine-arm

There's a change (no extension → .exe) that seems unneeded and risky. Other than that, looks good to me on code inspection.

The test run looks fine, but please also run one on 2.28 (which has slightly different requirements) and one on OpenCI (which has a similar but not identical Windows installation). They can be the same run.

gilles-peskine-arm · 2024-11-18T12:47:11Z

resources/windows/windows_testing.py

        self.selftest_exe = "selftest.exe"
        self.mingw_command = "mingw32-make"
        self.git_command = "git"
-        self.perl_command = "perl"
+        self.python_command = "python.exe"


Why python.exe and not python? What if python is a .bat wrapper?

Sorry, I made that change under the pretense from your other comment that python.exe is how python is always invoked on Windows. (I haven't done much development on Windows so I do not know the lay of the land).

This commit replaces python.exe with python to run python scripts. Signed-off-by: Harry Ramsey <[email protected]>

Harry-Ramsey · 2024-11-18T13:10:43Z

Latest Ci job: 730

gilles-peskine-arm

LGTM on code inspection — now waiting for the test jobs to complete

gilles-peskine-arm · 2024-11-18T15:53:47Z

https://jenkins-mbedtls.oss.arm.com/job/mbedtls-release-ci-testing/730/ looks fine. What about 2.28 and OpenCI?

Harry-Ramsey · 2024-11-20T15:20:49Z

https://jenkins-mbedtls.oss.arm.com/job/mbedtls-release-ci-testing/730/ looks fine. What about 2.28 and OpenCI?

I believe for OpenCI this should work but how would I go about testing it? As for 2.28, this will fail because the script isn't being backported to that version. What is the appropriate solution for seperating them out?

gilles-peskine-arm · 2024-11-20T16:21:19Z

@Harry-Ramsey Please start a similar job on OpenCI (https://mbedtls.trustedfirmware.org/job/mbedtls-release-ci-testing/). I'd prefer for you to do it so that you can make sure you have permission.

This should work on 2.28, it does have config.py. But I want a successful CI run, just to be sure that it does work.

Harry-Ramsey · 2024-11-20T16:25:53Z

I do not have permission to run anything on that CI :(

Harry-Ramsey · 2024-11-20T18:59:56Z

Build 298

Harry-Ramsey · 2024-11-21T13:24:33Z

Build 299. It appears that this works for 2.28 so perhaps the changes in MbedTLS can be backported to 2.28?

gilles-peskine-arm

LGTM on code reading and from the CI results

gilles-peskine-arm · 2024-11-21T14:36:39Z

Yes, we've now confirmed this works on 2.28.

perhaps the changes in MbedTLS can be backported to 2.28?

Sorry, what changes? This was expected to work on 2.28, and it's now confirmed to work. What would we backport?

Harry-Ramsey · 2024-11-21T15:17:22Z

Sorry, what changes? This was expected to work on 2.28, and it's now confirmed to work. What would we backport?

The changes are removing obsolete CI scripts from MbedTLS. The original goal was to do it for development but since the python script also exists in 2.28, I see no reason why it cannot be removed from 2.28 other than perhaps external reason?
Mbed-TLS/mbedtls#9666

gilles-peskine-arm · 2024-11-21T15:22:12Z

Oh ok. No, we shouldn't remove config.pl in LTS branches because it might be used in users' build scripts. Especially in 2.28.

mpg

LGTM, thanks!

Harry-Ramsey mentioned this pull request Oct 7, 2024

Remove obsolete CI Scripts Mbed-TLS/mbedtls#9666

Merged

5 tasks

gilles-peskine-arm reviewed Oct 7, 2024

View reviewed changes

Harry-Ramsey force-pushed the refactor-config-pl-config-py-main branch from 5a1c2d3 to 8315fe3 Compare October 8, 2024 15:03

mpg previously approved these changes Nov 14, 2024

View reviewed changes

Refactor scripts to use config.py instead of config.pl

30b2f77

This commit refactors Windows testing scripts to use config.py instead of config.pl because config.pl is being removed from the Mbed TLS repository. Signed-off-by: Harry Ramsey <[email protected]>

Harry-Ramsey force-pushed the refactor-config-pl-config-py-main branch from 8315fe3 to 30b2f77 Compare November 14, 2024 11:23

Harry-Ramsey self-assigned this Nov 14, 2024

gilles-peskine-arm reviewed Nov 18, 2024

View reviewed changes

gilles-peskine-arm added needs: ci priority-high labels Nov 18, 2024

Replace python.exe with python

8e4283c

This commit replaces python.exe with python to run python scripts. Signed-off-by: Harry Ramsey <[email protected]>

Harry-Ramsey dismissed mpg’s stale review via 8e4283c November 18, 2024 13:08

gilles-peskine-arm reviewed Nov 18, 2024

View reviewed changes

gilles-peskine-arm approved these changes Nov 21, 2024

View reviewed changes

gilles-peskine-arm removed the needs: ci label Nov 21, 2024

gilles-peskine-arm added the needs: review label Nov 21, 2024

gilles-peskine-arm requested a review from mpg November 21, 2024 14:36

gilles-peskine-arm added the size-xs Estimated task size: extra small (a few hours at most) label Nov 21, 2024

mpg approved these changes Nov 25, 2024

View reviewed changes

mpg added approved Approved in review. May need additional CI. and removed needs: review labels Nov 25, 2024

mpg merged commit 99a5a4f into Mbed-TLS:main Nov 25, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor scripts to use config.py instead of config.pl #178

Refactor scripts to use config.py instead of config.pl #178

Harry-Ramsey commented Oct 7, 2024 •

edited by gilles-peskine-arm

Loading

gilles-peskine-arm Oct 7, 2024

Harry-Ramsey Oct 8, 2024

gilles-peskine-arm Oct 7, 2024

Harry-Ramsey commented Nov 13, 2024

mpg left a comment •

edited

Loading

mpg commented Nov 14, 2024

Harry-Ramsey commented Nov 14, 2024

mpg commented Nov 14, 2024

Harry-Ramsey commented Nov 14, 2024

gilles-peskine-arm commented Nov 18, 2024 •

edited

Loading

gilles-peskine-arm left a comment

gilles-peskine-arm Nov 18, 2024

Harry-Ramsey Nov 18, 2024

Harry-Ramsey commented Nov 18, 2024

gilles-peskine-arm left a comment •

edited

Loading

gilles-peskine-arm commented Nov 18, 2024

Harry-Ramsey commented Nov 20, 2024

gilles-peskine-arm commented Nov 20, 2024

Harry-Ramsey commented Nov 20, 2024

Harry-Ramsey commented Nov 20, 2024

Harry-Ramsey commented Nov 21, 2024

gilles-peskine-arm left a comment

gilles-peskine-arm commented Nov 21, 2024

Harry-Ramsey commented Nov 21, 2024 •

edited

Loading

gilles-peskine-arm commented Nov 21, 2024

mpg left a comment

Refactor scripts to use config.py instead of config.pl #178

Refactor scripts to use config.py instead of config.pl #178

Conversation

Harry-Ramsey commented Oct 7, 2024 • edited by gilles-peskine-arm Loading

gilles-peskine-arm Oct 7, 2024

Choose a reason for hiding this comment

Harry-Ramsey Oct 8, 2024

Choose a reason for hiding this comment

gilles-peskine-arm Oct 7, 2024

Choose a reason for hiding this comment

Harry-Ramsey commented Nov 13, 2024

mpg left a comment • edited Loading

Choose a reason for hiding this comment

mpg commented Nov 14, 2024

Harry-Ramsey commented Nov 14, 2024

mpg commented Nov 14, 2024

Harry-Ramsey commented Nov 14, 2024

gilles-peskine-arm commented Nov 18, 2024 • edited Loading

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Nov 18, 2024

Choose a reason for hiding this comment

Harry-Ramsey Nov 18, 2024

Choose a reason for hiding this comment

Harry-Ramsey commented Nov 18, 2024

gilles-peskine-arm left a comment • edited Loading

Choose a reason for hiding this comment

gilles-peskine-arm commented Nov 18, 2024

Harry-Ramsey commented Nov 20, 2024

gilles-peskine-arm commented Nov 20, 2024

Harry-Ramsey commented Nov 20, 2024

Harry-Ramsey commented Nov 20, 2024

Harry-Ramsey commented Nov 21, 2024

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Nov 21, 2024

Harry-Ramsey commented Nov 21, 2024 • edited Loading

gilles-peskine-arm commented Nov 21, 2024

mpg left a comment

Choose a reason for hiding this comment

Harry-Ramsey commented Oct 7, 2024 •

edited by gilles-peskine-arm

Loading

mpg left a comment •

edited

Loading

gilles-peskine-arm commented Nov 18, 2024 •

edited

Loading

gilles-peskine-arm left a comment •

edited

Loading

Harry-Ramsey commented Nov 21, 2024 •

edited

Loading