Skip to content

✨️ A medley of CMake improvements#1899

Open
ISSOtm wants to merge 16 commits intomasterfrom
issotm/cmake-max
Open

✨️ A medley of CMake improvements#1899
ISSOtm wants to merge 16 commits intomasterfrom
issotm/cmake-max

Conversation

@ISSOtm
Copy link
Member

@ISSOtm ISSOtm commented Mar 9, 2026

Also bump it to the version I just tested it with

@ISSOtm ISSOtm added this to the 1.0.2 milestone Mar 9, 2026
@ISSOtm ISSOtm requested a review from Rangi42 March 9, 2026 05:35
@ISSOtm ISSOtm added bug Unexpected behavior / crashes; to be fixed ASAP! builds This affects the build process or release artifacts labels Mar 9, 2026
Comment on lines +7 to +9
set(ONLY_FREE $<IF:$<BOOL:USE_NONFREE_TESTS>,,--only-free>)
set(ONLY_INTERNAL $<IF:$<BOOL:USE_EXTERNAL_TESTS>,,--only-internal>)
set(OS_NAME "$<IF:$<STREQUAL:${TESTS_OS_NAME},>,,--os;${TESTS_OS_NAME}>")
Copy link
Contributor

Choose a reason for hiding this comment

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

That's some abstruse ternary syntax, but sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it might be breaking in FreeBSD?

  Run command: bash -c 'cd /home/runner/work/rgbds/rgbds/test; ./fetch-test-deps.sh $<IF:$<BOOL:USE_NONFREE_TESTS>,,--only-free> $<IF:$<BOOL:USE_EXTERNAL_TESTS>,,--only-internal>'
  bash: -c: line 1: syntax error near unexpected token `newline'
  bash: -c: line 1: `cd /home/runner/work/rgbds/rgbds/test; ./fetch-test-deps.sh $<IF:$<BOOL:USE_NONFREE_TESTS>,,--only-free> $<IF:$<BOOL:USE_EXTERNAL_TESTS>,,--only-internal>'
  Problem running command: bash -c 'cd /home/runner/work/rgbds/rgbds/test; ./fetch-test-deps.sh $<IF:$<BOOL:USE_NONFREE_TESTS>,,--only-free> $<IF:$<BOOL:USE_EXTERNAL_TESTS>,,--only-internal>'
  Problem executing pre-test command(s).
  Errors while running CTest

I'd be fine with using ordinary ifs here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree this ternary synax looks a little icky, but I was trying to avoid five lines of conditional logic per option here, for readability. Tradeoffs tradeoffs...

Comment on lines +29 to +35
add_compile_options(
/MP # Build multiple files in parallel.
/wd5105 # MSVC's own standard library triggers warning C5105, "macro expansion producing 'defined' has undefined behavior".
/wd5030 # Warning C5030 is about unknown attributes (`[[gnu::ATTR]]`), none of ours being load-bearing.
/wd4996 # Warning C4996 is about using POSIX names, which we want to do for portability.
/Zc:preprocessor # Opt into the C++20-conformant preprocessor.
)
Copy link
Contributor

Choose a reason for hiding this comment

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

These are long enough that they could be header-style not inline-style (like the comment on -Wno-gnu-zero-variadic-macro-arguments below):

Suggested change
add_compile_options(
/MP # Build multiple files in parallel.
/wd5105 # MSVC's own standard library triggers warning C5105, "macro expansion producing 'defined' has undefined behavior".
/wd5030 # Warning C5030 is about unknown attributes (`[[gnu::ATTR]]`), none of ours being load-bearing.
/wd4996 # Warning C4996 is about using POSIX names, which we want to do for portability.
/Zc:preprocessor # Opt into the C++20-conformant preprocessor.
)
add_compile_options(
# Build multiple files in parallel
/MP
# MSVC's own standard library triggers warning C5105,
# "macro expansion producing 'defined' has undefined behavior"
/wd5105
# Warning C5030 is about unknown attributes (`[[gnu::ATTR]]`),
# none of ours being load-bearing.
/wd5030
# Warning C4996 is about using POSIX names,
# which we want to do for portability.
/wd4996
# Opt into the C++20-conformant preprocessor.
/Zc:preprocessor
)

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the CMakeLists is quite long, I was trying to keep it more compact. But if you think that this is nonetheless better, I'd be fine with you committing this.

@ISSOtm ISSOtm changed the title Properly specify our max CMake version supported ✨️ A medley of CMake improvements Mar 9, 2026
@ISSOtm ISSOtm force-pushed the issotm/cmake-max branch 3 times, most recently from ede1f65 to 31637e3 Compare March 10, 2026 03:46
ISSOtm added 16 commits March 10, 2026 02:27
Also bump it to the version I just tested it with
This can help bring attention to the MSVC difference (no UBSan there)
Turns out the status messages use 3.17
We can use them now that the minimum version has been bumped
Also switch to heredoc syntax for ease of editing
Make the common files into an object library, which lets them
be compiled only once (saving 41 build steps)
This also lends itself well to removing the per-program loop,
which simplifies the code somewhat.
Hopefully Microsoft fixed their stuff by now, riiiight?
@ISSOtm ISSOtm force-pushed the issotm/cmake-max branch from 31637e3 to 0c713db Compare March 10, 2026 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Unexpected behavior / crashes; to be fixed ASAP! builds This affects the build process or release artifacts

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants