Skip to content

Explicitly specify build and test jobs parallelism count#1959

Open
Rangi42 wants to merge 6 commits into
gbdev:masterfrom
Rangi42:job-count
Open

Explicitly specify build and test jobs parallelism count#1959
Rangi42 wants to merge 6 commits into
gbdev:masterfrom
Rangi42:job-count

Conversation

@Rangi42
Copy link
Copy Markdown
Contributor

@Rangi42 Rangi42 commented Apr 20, 2026

Fixes #1944

This PR uses getconf _NPROCESSORS_ONLN to set the job counts because nproc is not available on macOS.

@Rangi42 Rangi42 added this to the 1.0.2 milestone Apr 20, 2026
@Rangi42 Rangi42 requested a review from ISSOtm April 20, 2026 13:22
@Rangi42 Rangi42 added tests This affects the test suite builds This affects the build process or release artifacts labels Apr 20, 2026
Copy link
Copy Markdown
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

Soon we'll be free from the shackles of these tiny incremental improvements. 🫂

Comment thread contrib/coverage.bash Outdated
Comment thread contrib/coverage.bash Outdated
@ISSOtm
Copy link
Copy Markdown
Member

ISSOtm commented Apr 20, 2026

[EDIT] The OP that this responds to has been edited.

Or you can use the nproc command (possibly outputting it to an env var to . I think it's not available on all platforms, but those that lack it should have equivalents. (Finding those platforms and their alternatives is left as an exercise to the reader because I'm feeling lazy.)

  • In coverage.bash, I could add a --jobs flag like run-tests.sh now has, and update coverage.yml to count the CPU cores and pass ./run-tests.sh --jobs ${{ steps.cpu-cores.outputs.count }} too.

Well, this adds extra CLI parsing complexity to that script, and at this point I suggested in my review that coverage.bash instead pass all of its args through to run-tests.sh, and we stop trying to open automatically in order to drop the need to parse those args. (There's precedent, too: LLVM's scan-build also prints a scan-view command to run when it's done. Feel free to add a --open flag if you wanna keep the convenience, and you can require it to be the first flag for parsing simplicity.)

  • In run-tests.sh, the make_jobs=4 is just the default level if no --jobs count is specified. We could default to 1 instead, or to an empty string if we want make -j$make_jobs to become make -j and parallelize as much as the CPU can.

Actually, make -j doesn't put any limits on concurrent jobs, so it's more “parallelize until the OS scheduler gets angry” :P IMO it's an anti-pattern, which I've just never bothered to correct thus far.

If we don't want to use that SimenB/github-actions-cpu-cores@v2 action, then I don't know how our CI actions should be handling this.

As mentioned above, let's use nproc or similar commands.

# 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

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

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

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

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

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

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

Comment thread .github/workflows/testing.yml
@Rangi42 Rangi42 marked this pull request as ready for review May 11, 2026 15:36
@Rangi42 Rangi42 requested a review from ISSOtm May 11, 2026 15:36
Copy link
Copy Markdown
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

Thanks for soldiering on!

Comment thread contrib/coverage.bash Outdated
Comment thread test/CMakeLists.txt Outdated
Comment thread test/CMakeLists.txt
Comment on lines +47 to +50
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}
Copy link
Copy Markdown
Member

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

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.

Comment thread test/run-tests.sh Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

builds This affects the build process or release artifacts tests This affects the test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use CMake ProcessorCount in CI

2 participants