Skip to content

Conversation

@ChinYikMing
Copy link
Collaborator

@ChinYikMing ChinYikMing commented Nov 8, 2025

Description

  • Add make format target for easier formatting for developers.
  • Intruduce version checking utils: version_{eq, lt, lte, gt, gte} and refactor with them for tools: emcc, browsers(Chrome, Firefox, Safari), black
  • Update CONTRIBUTING.md to reflect the make format target

Summary by cubic

Adds a make format target to auto-format C/C++, shell, and Python, and introduces version-compare helpers to simplify build-time checks. This streamlines developer formatting and reduces brittle version logic.

  • New Features

    • make format runs clang-format, shfmt, and black; skips submodules and OUT.
    • Validates tool presence and versions (clang-format 18, shfmt, black 25.1.0+), with helpful warnings.
    • CONTRIBUTING.md updated to document the new command.
  • Refactors

    • Added version_{eq,lt,lte,gt,gte} helpers in mk/common.mk.
    • Replaced nested bc checks in mk/toolchain.mk and mk/wasm.mk:
      • SDL music: warn unless emcc == 3.1.51.
      • Enable mimalloc when emcc >= 3.1.50.
      • TCO checks: Chrome/Firefox by major version, Safari >= 18.2.

Written for commit b1ba59b. Summary will update automatically on new commits.

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

4 issues found across 5 files

Prompt for AI agents (all 4 issues)
 Understand the root cause of the following 4 issues and fix them. <file name="mk/wasm.mk"> <violation number="1" location="mk/wasm.mk:86"> By splitting the Safari requirement into major/minor constants without also defining SAFARI_SUPPORT_TCO_AT_MAJOR_MINOR (or updating the warning string), the warning now expands to an empty version number, leaving developers without the Safari version requirement.</violation> </file> <file name="Makefile"> <violation number="1" location="Makefile:497"> Guard the black version query so it isn’t executed when black is absent, otherwise every make invocation emits `/bin/sh: 1: --version: not found`.</violation> <violation number="2" location="Makefile:508"> Ensure the generated prune expression always has a left-hand operand; when SUBMODULES is empty the current command becomes `find . ( -o … )` and fails.</violation> <violation number="3" location="Makefile:515"> Failures in formatters are suppressed. The formatting commands are wrapped in `$(shell ...)` which prevents `make` from seeing their exit codes. If a formatter fails, the `make format` target will not fail, leading developers to believe formatting was successful when it was not. The commands should be executed directly, not inside `$(shell ...)`, so that `make` can properly handle their failure.</violation> </file> 

React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.

@ChinYikMing ChinYikMing force-pushed the make-format-tgt branch 2 times, most recently from 8e059f1 to a1ffaf2 Compare November 8, 2025 12:49
Makefile Outdated
Comment on lines 491 to 492
# If clang-format-x (x > 18) is used, change the 'clang-format-18' to 'clang-format-x' accordingly
CLANG_FORMAT := $(shell which clang-format-18 2>/dev/null)
Copy link
Contributor

Choose a reason for hiding this comment

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

It is a known issue that clang-format versions 18 and 20+ produce inconsistent indentation, which prevents us from standardizing the formatter version across developers.

Copy link
Collaborator Author

@ChinYikMing ChinYikMing Nov 9, 2025

Choose a reason for hiding this comment

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

It is a known issue that clang-format versions 18 and 20+ produce inconsistent indentation, which prevents us from standardizing the formatter version across developers.

Since clang-format-18 is still used in check-format.sh, we can stick with clang-format-18 for now IIUC?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since clang-format-18 is still use in check-format.sh, we can stick with clang-format-18 for now IIUC?

Yes, it is specific to clang-format-18 for preferable indention.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it, I would then remove the comment and force to use clang-format-18 instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The CONTRIBUTING.md is also modified to use clang-format-18 forcibly.

Simplifies the formatting before creating PR, just 'make format'. Version checking is needed for tools, e.g., emcc, browser, and black(python formatting tool). Thus, introduce and refactor with version_{eq,lt,lte,gt,gte} utils for better readability and maintainability (no more 'Pyramid of Doom' checking for MAJOR, MINOR and PATCH).
- To reflect the 'make format' new target. - Remove 'or later' to use clang-format-18 for consistency.
@jserv jserv merged commit 6a8958d into sysprog21:master Nov 12, 2025
20 of 28 checks passed
@jserv
Copy link
Contributor

jserv commented Nov 12, 2025

Thank @ChinYikMing for contributing!

@jserv jserv added this to the release-2025.2 milestone Nov 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants