Repository navigation
Add CPython to CI - #28113
Add CPython to CI#28113
Conversation
Resolves issue emscripten-core#28097.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new CircleCI job and a supporting Python script (test/cpython_test.py) to build and run the CPython test suite against the local Emscripten checkout. The review feedback focuses on improving the robustness of the script: adding safety checks for config.NODE_JS in node_path(), preventing a trailing path separator when modifying PATH in build_env(), simplifying the test() function by removing redundant exception handling, and catching subprocess.CalledProcessError at the top level in main() to exit cleanly without tracebacks.
6cc8b2f to
395403d
Compare
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Nice!
Sorry about new noise from gemini code review... sometimes is get be useful, but maybe not here.
| sys.path.insert(0, __rootpath__) | ||
|
|
||
| from tools import config | ||
| from tools.utils import path_from_root |
There was a problem hiding this comment.
Just FYI, I'm trying to move away from using parts of the compiler itself in test code like this.
This looks pretty minimal but the more you can avoid using these the better (since in the long run I'd like to remove this code sharing).
|
Thanks @sbc100 for the quick review! |
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
lgtm!
I wonder if we could split the CI into two steps:
- Build cpython
- Run the tests
That way it will be easy to see in the circleci UI how long each phase takes.
| # CPython's Platforms/emscripten build scripts need Python >= 3.12, hence | ||
| # the different executor. | ||
| executor: linux-python | ||
| resource_class: large |
There was a problem hiding this comment.
Does the cpython build automatically use parallel build? How about the test suite?
There was a problem hiding this comment.
I think compilation is parallelized, so far the test suite is completely serial.
Yeah makes sense will do. |
| - build-linux | ||
| - test-cpython: | ||
| requires: | ||
| - build-linux |
There was a problem hiding this comment.
I wonder if you need to have this dependency? For quick feedback its best not to depend on this. Reling on build-linux is mostly usefull for bots that would build many different system library configurations.
There was a problem hiding this comment.
Okay I'll try dropping this.
|
In the current run Sync + Build took 9m 18s out of what IIRC is ~20 minutes total, so I guess the runtime is about equal between build and test. |
|
lgtm once all the tests pass and assuming this is not the slowest job we have. @hoodmane do you have commit permissions here already? |
|
I do, will merge once I confirm those things. |
|
Yeah it's not anywhere close to being the slowest job now that it doesn't depend on build-linux. But if it did depend on build-linux I guess it would be the slowest. |
Resolves issue #28097.