Skip to content

Explicitly specify build and test jobs parallelism count #1959

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.

Sign up for GitHub

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

Open
Rangi42 wants to merge 6 commits into
base: master
Choose a base branch
from
+73 −24
Open

Explicitly specify build and test jobs parallelism count #1959

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2b62d6f
Explicitly specify build and test jobs parallelism count
Rangi42 Apr 20, 2026
9853b92
Updates based on review
Rangi42 May 11, 2026
9414173
Use `CTEST_PARALLEL_LEVEL` in test/CMakeLists.txt
Rangi42 May 11, 2026
91c008a
Use `$ENV{CTEST_PARALLEL_LEVEL}`
Rangi42 May 11, 2026
0d4adc4
Add arguments to coverage.bash
Rangi42 May 12, 2026
8902f6b
Fixes from review
Rangi42 May 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/coverage.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ jobs:
test/fetch-test-deps.sh --get-deps ubuntu
- name: Generate coverage report
run: |
contrib/coverage.bash ubuntu-ci
contrib/coverage.bash --os ubuntu-ci --jobs $(getconf _NPROCESSORS_ONLN)
- name: Upload coverage report
uses: actions/upload-artifact@v7
with:
Expand Down
16 changes: 8 additions & 8 deletions .github/workflows/testing.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ env:
CMAKE_CONFIG_TYPE: Debug # `cmake --build` now implies `--config Debug`.
# Approximate number of CPU cores in GitHub's runners as of 2026-03-18:
# https://docs.github.com/en/actions/reference/runners/github-hosted-runners#standard-github-hosted-runners-for-public-repositories
CTEST_PARALLEL_LEVEL: 0 # `ctest` now implies `--parallel 0` (number of logical CPUs).
CTEST_PARALLEL_LEVEL: 4 # `ctest` now implies `--parallel 4`.
Copy link
Copy Markdown
Contributor Author

@Rangi42 Rangi42 May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--parallel 0 was not the "number of logical CPUs", it's "unbounded", same as make -j.

https://cmake.org/cmake/help/latest/manual/ctest.1.html#cmdoption-ctest-j:

Otherwise, if the value is omitted, parallelism is limited by the number of processors, or 2, whichever is larger.
Otherwise, if the value is 0, parallelism is unbounded.

Copy link
Copy Markdown
Member

@ISSOtm ISSOtm May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annoying how this is inconsistent with other instances of -j 0 or -j. Thanks for noticing.

Copy link
Copy Markdown
Member

@ISSOtm ISSOtm May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaah, wait, the behaviour is different for the environment variable!

Changed in version 3.29: The value may be empty, or 0, to let ctest use a default level of parallelism, or unbounded parallelism, respectively, as documented by the ctest --parallel option.

CTest will interpret a whitespace-only string as empty.

In CMake 3.28 and earlier, an empty or 0 value was equivalent to 1.

Copy link
Copy Markdown
Member

@ISSOtm ISSOtm May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CTEST_PARALLEL_LEVEL: 4 # `ctest` now implies `--parallel 4`.
CTEST_PARALLEL_LEVEL: '' # `ctest` now implies `--parallel` (number of logical CPUs).

Copy link
Copy Markdown
Contributor Author

@Rangi42 Rangi42 May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A blank CTEST_PARALLEL_LEVEL value wouldn't be reusable for passing to the individual third-party repos as make -j$ENV{CTEST_PARALLEL_LEVEL}. If you'd still prefer this to be empty as the number of logical CPUs, do you have any suggestion for what to do about that? (Also, should CMAKE_BUILD_PARALLEL_LEVEL and CMAKE_INSTALL_PARALLEL_LEVEL be set to '' instead of 4 in create-release-artifacts.yml?)

Copy link
Copy Markdown
Member

@ISSOtm ISSOtm May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The $ENV concern is obsoleted by #1959 (comment). As for those other two variables, they have different behaviour... BUILD_ uses the native tool's default (number of CPUs for Ninja, but 1 for Make); INSTALL_... doesn't specify, but I expect that it would reject 0 like BUILD_ does (I just tested that).

Note also that since we aren't enabling the INSTALL_PARALLEL global property, the latter actually turns out to do nothing lol

CTEST_NO_TESTS_ACTION: error # Make CTest fail if it cannot find any tests. (That should never happen.)
CTEST_OUTPUT_ON_FAILURE: ON # CTest reports test program output on failure.

Expand Down Expand Up @@ -52,7 +52,7 @@ jobs:
if: matrix.buildsys == 'make'
run: |
make develop -kj Q= CXX=${{ matrix.cxx }}
sudo make install -j Q=
sudo make install Q=
Comment thread
ISSOtm marked this conversation as resolved.
- name: Build & install using CMake
if: matrix.buildsys == 'cmake'
# Since GitHub's runners are basically kitchen sinks,
Expand Down Expand Up @@ -98,7 +98,7 @@ jobs:
- name: Run tests using our script
if: matrix.buildsys == 'make'
run: |
CXX=${{ matrix.cxx }} test/run-tests.sh --os ${{ matrix.os }}
CXX=${{ matrix.cxx }} test/run-tests.sh --os ${{ matrix.os }} --jobs $(getconf _NPROCESSORS_ONLN)
- name: Run tests using CTest
if: matrix.buildsys == 'cmake'
run: |
Expand Down Expand Up @@ -311,7 +311,7 @@ jobs:
test/fetch-test-deps.sh --get-deps ${{ matrix.os }}
- name: Run tests
run: |
test/run-tests.sh --os ${{ matrix.os }}
test/run-tests.sh --os ${{ matrix.os }} --jobs $(getconf _NPROCESSORS_ONLN)

cygwin:
strategy:
Expand Down Expand Up @@ -344,11 +344,11 @@ jobs:
pkg-config
- name: Build & install using Make
run: | # Cygwin does not support `make develop` sanitizers ASan or UBSan
make -kj Q=
make install -j Q=
make -k -j $(getconf _NPROCESSORS_ONLN) Q=
make install Q=
- name: Run tests
run: |
test/run-tests.sh --only-internal
test/run-tests.sh --jobs $(getconf _NPROCESSORS_ONLN) --only-internal

freebsd:
runs-on: ubuntu-latest
Expand All @@ -371,6 +371,6 @@ jobs:
.github/scripts/install_deps.sh freebsd
run: | # Leak detection is not supported on FreeBSD, so disable it.
cmake -B build --preset develop -DTESTS_OS_NAME=freebsd
cmake --build build --verbose -- -k -j 4
cmake --build build --verbose -- -k -j $(getconf NPROCESSORS_ONLN)
ASAN_OPTIONS=detect_leaks=0 ctest --test-dir build --schedule-random --label-exclude external
cmake --install build --verbose
21 changes: 5 additions & 16 deletions contrib/coverage.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,12 @@
set -e

# Build RGBDS with gcov support
make coverage -j
make coverage -j $(getconf _NPROCESSORS_ONLN)
Comment thread
Rangi42 marked this conversation as resolved.
Outdated

# Run the tests
# Run the tests, forwarding all script arguments
pushd test
./fetch-test-deps.sh
if [[ $# -eq 0 ]]; then
./run-tests.sh
else
./run-tests.sh --os "$1"
fi
./run-tests.sh "$@"
popd

# Generate coverage logs
Expand All @@ -24,12 +20,5 @@ lcov -c --no-external -d . -o "$COVERAGE_INFO"
lcov -r "$COVERAGE_INFO" src/asm/parser.{hpp,cpp} src/link/script.{hpp,cpp} -o "$COVERAGE_INFO"
genhtml --dark-mode --num-spaces 4 -f -s -o coverage/ "$COVERAGE_INFO"

# Check whether running from coverage.yml workflow
if [ "$1" != "ubuntu-ci" ]; then
# Open report in web browser
if [ "$(uname)" == "Darwin" ]; then
open coverage/index.html
else
xdg-open coverage/index.html
fi
fi
# Output the path to the report
echo "Open $PWD/coverage/index.html"
6 changes: 3 additions & 3 deletions test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ foreach(component "asm" "link" "fix" "gfx")
COMMAND bash -- test.sh
WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/${component}")
set_tests_properties("rgb${component}" PROPERTIES REQUIRED_FILES "$<TARGET_FILE:rgb${component}>"
PROCESSORS 1
PROCESSORS $ENV{CTEST_PARALLEL_LEVEL}
Comment thread
Rangi42 marked this conversation as resolved.
Outdated
LABELS "rgb${component}")
endforeach()
set_tests_properties(rgbgfx PROPERTIES REQUIRED_FILES "$<TARGET_FILE:rgbgfx>;$<TARGET_FILE:randtilegen>;$<TARGET_FILE:rgbgfx_test>")
Expand All @@ -44,9 +44,9 @@ add_test(NAME fetch-test-deps
set_tests_properties(fetch-test-deps PROPERTIES FIXTURES_SETUP "external-repos"
LABELS "external")
add_test(NAME external
COMMAND bash -- run-tests.sh --only-external ${ONLY_FREE} ${OS_NAME}
COMMAND bash -- run-tests.sh --jobs $ENV{CTEST_PARALLEL_LEVEL} --only-external ${ONLY_FREE} ${OS_NAME}
WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}")
set_tests_properties(external PROPERTIES DEPENDS "rgbasm;rgblink;rgbfix;rgbgfx" # Only attempt building whole projects if each tool passes muster on its own.
PROCESSORS 4
PROCESSORS $ENV{CTEST_PARALLEL_LEVEL}
Comment on lines +47 to +50
Copy link
Copy Markdown
Member

@ISSOtm ISSOtm May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot rely on this environment variable, as the parallelism may be specified via the CLI only, and this would plainly error out without the env var. We don't have access to that parameter at configure time, anyway, and the env var could also be set only when running ctest itself.

We could try querying the number of logical CPUs (cmake_host_system_information(RESULT n QUERY NUMBER_OF_LOGICAL_CORES) according to CMake's docs) and using that, but I don't know how that would interact with ctest -j 1.

Copy link
Copy Markdown
Contributor Author

@Rangi42 Rangi42 May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably the worst that could happen is the config-time NUMBER_OF_LOGICAL_CORES value being used instead of the run-time -j value... but that's still better than what we have now, using PROCESSORS 4 no matter what -j is.

FIXTURES_REQUIRED "external-repos"
LABELS "external") # Allow filtering out external tests.
10 changes: 8 additions & 2 deletions test/run-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Options:
--os <os> skip tests known to fail on <os> (e.g. `macos-14`)
--installed-rgbds use the system installed RGBDS
(only compatible with external codebases)
--jobs <n> build external codebases with `make -j<n>`
EOF
}

Expand All @@ -29,6 +30,7 @@ internal=true
external=true
installedrgbds=false
osname=
make_jobs=
FETCH_TEST_DEPS="fetch-test-deps.sh"
RGBDS_PATH="RGBDS=../../"
while [[ $# -gt 0 ]]; do
Expand All @@ -52,6 +54,10 @@ while [[ $# -gt 0 ]]; do
installedrgbds=true
RGBDS_PATH=
;;
--jobs)
shift
make_jobs="-j$1"
;;
--os)
shift
osname="$1"
Expand Down Expand Up @@ -117,8 +123,8 @@ test_downstream() { # owner repo make-target build-file build-hash
echo >&2 'Please run `'"$FETCH_TEST_DEPS"'` before running the test suite'
return 1
fi
make clean $RGBDS_PATH
make -j4 "$3" $RGBDS_PATH
make clean "$RGBDS_PATH"
make "$make_jobs" "$3" "$RGBDS_PATH"
Comment thread
Rangi42 marked this conversation as resolved.
Outdated
hash="$(sha1sum -b "$4" | head -c 40)"
if [ "$hash" != "$5" ]; then
echo >&2 'SHA-1 hash of '"$4"' did not match: '"$hash"
Expand Down
Loading