Repository navigation
Split CI cargo run and test into build then execute #8705
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
Changes from all commits
fe1745a
569b47a
71fc8b2
84c949e
d23e72a
5f4c50e
8a4ae8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -71,6 +71,10 @@ def parse_args(): | |||||||||
| help="which features to enable when building RustPython (default: [])", | ||||||||||
| default=[], | ||||||||||
| ) | ||||||||||
| parser.add_argument( | ||||||||||
| "--rustpython", | ||||||||||
| help="use this RustPython executable instead of building one with cargo", | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| args = parser.parse_args() | ||||||||||
| return args | ||||||||||
|
|
@@ -446,27 +450,40 @@ def remove_one_indent(s): | |||||||||
| f.write(output + "\n") | ||||||||||
|
|
||||||||||
|
|
||||||||||
| cargo_build_command = ["cargo", "build", "--release"] | ||||||||||
| if args.no_default_features: | ||||||||||
| cargo_build_command.append("--no-default-features") | ||||||||||
| def resolve_rustpython(path): | ||||||||||
| if os.path.isfile(path): | ||||||||||
| return path | ||||||||||
|
Comment on lines
+454
to
+455
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win Reject non-executable RustPython paths. A regular file without execute permission passes this check on POSIX. Proposed fix- if os.path.isfile(path):
+ if os.path.isfile(path) and (os.name == "nt" or os.access(path, os.X_OK)):
return path📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
| if os.name == "nt" and not path.lower().endswith(".exe"): | ||||||||||
| exe = path + ".exe" | ||||||||||
| if os.path.isfile(exe): | ||||||||||
| return exe | ||||||||||
| sys.exit(f"RustPython executable not found: {path}") | ||||||||||
|
|
||||||||||
|
|
||||||||||
| if args.rustpython: | ||||||||||
| rustpython_run_command = [resolve_rustpython(args.rustpython), GENERATED_FILE] | ||||||||||
| else: | ||||||||||
| cargo_build_command = ["cargo", "build", "--release"] | ||||||||||
| if args.no_default_features: | ||||||||||
| cargo_build_command.append("--no-default-features") | ||||||||||
|
|
||||||||||
| joined_features = ",".join(args.features) | ||||||||||
| if args.features: | ||||||||||
| cargo_build_command.extend(["--features", joined_features]) | ||||||||||
| joined_features = ",".join(args.features) | ||||||||||
| if args.features: | ||||||||||
| cargo_build_command.extend(["--features", joined_features]) | ||||||||||
|
|
||||||||||
| subprocess.run(cargo_build_command, check=True) | ||||||||||
| subprocess.run(cargo_build_command, check=True) | ||||||||||
|
|
||||||||||
| cargo_run_command = ["cargo", "run", "--release"] | ||||||||||
| if args.no_default_features: | ||||||||||
| cargo_run_command.append("--no-default-features") | ||||||||||
| rustpython_run_command = ["cargo", "run", "--release"] | ||||||||||
| if args.no_default_features: | ||||||||||
| rustpython_run_command.append("--no-default-features") | ||||||||||
|
|
||||||||||
| if args.features: | ||||||||||
| cargo_run_command.extend(["--features", joined_features]) | ||||||||||
| if args.features: | ||||||||||
| rustpython_run_command.extend(["--features", joined_features]) | ||||||||||
|
|
||||||||||
| cargo_run_command.extend(["-q", "--", GENERATED_FILE]) | ||||||||||
| rustpython_run_command.extend(["-q", "--", GENERATED_FILE]) | ||||||||||
|
|
||||||||||
| result = subprocess.run( | ||||||||||
| cargo_run_command, | ||||||||||
| rustpython_run_command, | ||||||||||
| env={**os.environ.copy(), "RUSTPYTHONPATH": "Lib"}, | ||||||||||
| text=True, | ||||||||||
| capture_output=True, | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Compile the same Cargo target set before each execution step.
The execution commands add
--all-targets, but the preceding no-run commands do not. Benchmark targets can compile during execution, so CI does not fully separate compilation from execution. (doc.rust-lang.org).github/workflows/ci.yaml#L129-L129: add--all-targetsto the Rust test build command..github/workflows/ci.yaml#L150-L150: add--all-targetsto the C-API test build command.Proposed fix
📍 Affects 1 file
.github/workflows/ci.yaml#L129-L129(this comment).github/workflows/ci.yaml#L150-L150🤖 Prompt for AI Agents