gnu tests: limit make jobs to nproc output - #13991
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR fixes run-gnu-test.sh to correctly limit make parallelism by executing nproc, validating its output, and passing the job count via -j<N>. It also adds a regression test and updates CI to run all Python tests under util/.
Changes:
- Execute
nproc, validate numeric output, and append-j<JOBS>toMAKEFLAGSinutil/run-gnu-test.sh. - Add an end-to-end Python unittest that asserts the effective
MAKEFLAGScontains-j<N>and rejects invalidnprocoutput. - Update GitHub Actions to discover and run all
util/test_*.pyPython unit tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| util/test_run_gnu_test.py | Adds an end-to-end regression test covering MAKEFLAGS and invalid nproc output behavior. |
| util/run-gnu-test.sh | Fixes job count handling by executing nproc, validating the result, and forming -j<N> correctly. |
| .github/workflows/code-quality.yml | Expands CI to run all Python unit tests in util/ via unittest discovery. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # Use GNU nproc for *BSD | ||
| NPROC=$(command -v ${path_GNU}/src/nproc||command -v nproc) | ||
| MAKEFLAGS="${MAKEFLAGS} -j ${NPROC}" | ||
| NPROC_COMMAND=$(command -v "${path_GNU}/src/nproc" || command -v nproc) |
| return subprocess.run( | ||
| ["bash", str(RUN_GNU_TEST)], | ||
| cwd=REPO_ROOT, | ||
| env=env, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=10, |
|
|
||
| def run_script(self, nproc_output): | ||
| nproc = self.gnu_dir / "src" / "nproc" | ||
| nproc.write_text(f"#!/bin/sh\nprintf '%s\\n' '{nproc_output}'\n") |
faf2d37 to
9a825e3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
util/run-gnu-test.sh:38
- With
set -e, the script can exit without a clear error message when neither${path_GNU}/src/nprocis executable nornprocexists on PATH (and likewise if invoking nproc itself fails). Consider explicitly selecting an executable path (using-xfor the in-tree binary), handling the “not found” case, and checking the nproc exit status so failures are reported deterministically.
NPROC_COMMAND=$(command -v "${path_GNU}/src/nproc" || command -v nproc)
JOBS=$("${NPROC_COMMAND}")
9a825e3 to
a7d375f
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
| NPROC_COMMAND=$(command -v "${path_GNU}/src/nproc" || command -v nproc) | ||
| JOBS=$("${NPROC_COMMAND}") | ||
| case "${JOBS}" in | ||
| '' | 0 | *[!0-9]*) | ||
| echo "Error: '${NPROC_COMMAND}' returned an invalid job count: '${JOBS}'" >&2 | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| MAKEFLAGS="${MAKEFLAGS:+${MAKEFLAGS} }-j${JOBS}" |
a7d375f to
0bb859b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
util/run-gnu-test.sh:42
- The job-count validation rejects
0but still accepts values like00/000(these are effectively zero and would re-enable unlimited make parallelism). Since the intent is to require a positive integer, tighten the check to only allow1..and digits.
JOBS=$("${NPROC_COMMAND}")
case "${JOBS}" in
'' | 0 | *[!0-9]*)
echo "Error: '${NPROC_COMMAND}' returned an invalid job count: '${JOBS}'" >&2
exit 1
;;
esac
|
GNU testsuite comparison: |
|
I don't think it is worth to have 100+ lines just for this. How about removing regression test which is unrelated with actual production? |
|
+1, please make this shorter |
run-gnu-test.sh put the nproc executable path after a bare -j. GNU make treated -j as unlimited parallelism and the path as another target, oversubscribing the test runner. Resolve and execute nproc, reject a missing command or invalid output, and attach the resulting count to -j.
0bb859b to
e7ba126
Compare
|
OK - @oech3 @sylvestre I agree, removed all of the Python tests, there wasn't a good, short way to write them. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
util/run-gnu-test.sh:35
util/build-gnu.shfalls back tognprocon platforms where GNU coreutils are prefixed (e.g., macOS/Homebrew). This updated lookup only checksnproc, sorun-gnu-test.shcan now hard-fail in environments that previously had a usablegnprocin PATH.
NPROC_COMMAND=$(command -v "${path_GNU}/src/nproc" || command -v nproc || :)
if [ -z "${NPROC_COMMAND}" ] || [ ! -x "${NPROC_COMMAND}" ]; then
echo "Error: unable to find an executable nproc command at '${path_GNU}/src/nproc' or in PATH" >&2
exit 1
util/run-gnu-test.sh:45
- The PR description says an end-to-end regression test was added for the effective
MAKEFLAGS, but this PR only changesrun-gnu-test.shand there is no test coverage protecting the new-j$(nproc)behavior from regressing (e.g., back to-j <path-to-nproc>or bare-j). Please add an integration test that runsutil/run-gnu-test.shagainst a minimal fake GNU tree + fakemake, and asserts theMAKEFLAGSseen bymakeincludes-j<N>where<N>is the mockednprocoutput.
MAKEFLAGS="${MAKEFLAGS:+${MAKEFLAGS} }-j${JOBS}"
export MAKEFLAGS
echo "GNU test make job count: ${JOBS}"
|
@kevinburke thanks |
run-gnu-test.shpassed thenprocexecutable path after a bare-j,which enabled unlimited make parallelism and treated the path as a target.
Execute
nproc, validate its output, and pass the count as an attached-jargument. Add an end-to-end regression test for the effective
MAKEFLAGS.