Refactor scripts to use config.py instead of config.pl #178
Conversation
resources/windows/windows_testing.py
Outdated
| )) | ||
| try: | ||
| enable_output = subprocess.run( | ||
| [self.perl_command, self.config_pl_location, "full"], | ||
| [self.config_py_location, "full"], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have resolved this now, I still need to test it however Docker on MacOS is proving to be a bit troublesome.
| self.selftest_exe = "selftest.exe" | ||
| self.mingw_command = "mingw32-make" | ||
| self.git_command = "git" | ||
| self.perl_command = "perl" |
There was a problem hiding this comment.
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
force-pushed
the
refactor-config-pl-config-py-main
branch
from
5a1c2d3
to
8315fe3
Compare
|
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? |
|
@Harry-Ramsey I'm not sure if we have documentation about this, but I think what you want to do is:
(Disclaimer: I don't test changes on |
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
|
@mpg I believe I do not have permission to push to mbedtls-test as git is giving me a permission error. |
|
Can you try again? I believe permissions have been fixed now. |
|
Thanks, building now. The result can be found at 729. |
It's waiting for review
That looks right to me. |
There was a problem hiding this comment.
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.
resources/windows/windows_testing.py
Outdated
| self.selftest_exe = "selftest.exe" | ||
| self.mingw_command = "mingw32-make" | ||
| self.git_command = "git" | ||
| self.perl_command = "perl" | ||
| self.python_command = "python.exe" |
There was a problem hiding this comment.
Why python.exe and not python? What if python is a .bat wrapper?
There was a problem hiding this comment.
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]>
|
Latest Ci job: 730 |
|
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? |
|
@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 |
|
I do not have permission to run anything on that CI :( |
|
Build 298 |
|
Build 299. It appears that this works for 2.28 so perhaps the changes in MbedTLS can be backported to 2.28? |
|
Yes, we've now confirmed this works on 2.28.
Sorry, what changes? This was expected to work on 2.28, and it's now confirmed to work. What would we backport? |
gilles-peskine-arm
added
the
size-xs
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? |
|
Oh ok. No, we shouldn't remove |
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 CImbedtls-2.28: OpenCI