Repository navigation
Add CPython to CI - #28113
Add CPython to CI#28113hoodmane wants to merge 6 commits into
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).
| # we always want to use the local emscripten. | ||
| env.pop('EMSDK_CACHE', None) | ||
| if config.EM_CONFIG: | ||
| env['EM_CONFIG'] = config.EM_CONFIG |
There was a problem hiding this comment.
I believe this is already done in config.py:init (on line 274)
|
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. |
Resolves issue #28097.