Chore(core-debug): debugged utils to hide std::cout output from production builds by Prachi-Gupta2808 · Pull Request #524 · Ryan-Millard/Img2Num

Prachi-Gupta2808 · 2026-07-16T06:57:58Z

Changes & Reason

Changes

Replaced raw std::cout debug output in the GPU source files with the project's debug logging macros.
Removed leftover commented std::cout/std::cerr debug statements.
Added the required internal/debug.h includes.
Added a GitHub Actions workflow to detect raw std::cout and std::cerr usage in C++ files (excluding third_party/ and example-apps/).
Added internal documentation explaining the new debug logging approach and how to use it.

Reason

This change ensures that debug logging goes through a centralized logging system instead of raw console output. It also prevents accidental debug prints from being committed in the future by enforcing the rule through CI.

Related Issues

Fixes: #240

Testing & Verification

Built the project after making the changes.
Verified that the modified source files no longer contain raw std::cout/std::cerr statements.
Confirmed that the CI workflow checks for raw debug output in C++ source files.

Additional Resources

N/A

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

Ryan-Millard

Hi. I'm sorry for letting you implement all of this and then changing my mind about some things. That's just how it goes sometimes. :(

Please see this conversation with ChatGPT - it shows how to:

Manage spdlog via CMake, which is much safer than macros,
Enforce the no console printing rule more effectively via clang-tidy

Please will you also update it to ensure that console logging is always off in Release mode unless the user explicitly passed a flag to enable it.

Additionally, why did you update the Dawn submodule?

This is good work, though. Thank you!

Prachi-Gupta2808 · 2026-07-16T12:29:01Z

Hi @Ryan-Millard , no worries at all.

Regarding Dawn, it likely got updated accidentally when I ran git submodule update --init to pull the stb submodule for the build.

Let me ask you again, you want these right?

Use SPDLOG_ACTIVE_LEVEL managed via CMake instead of the macro approach
Default logging to OFF in Release mode unless explicitly enabled with a flag
Add clang-tidy enforcement for the no-console-printing rule

Ryan-Millard · 2026-07-16T12:35:59Z

Hi @Ryan-Millard , no worries at all.

Regarding Dawn, it likely got updated accidentally when I ran git submodule update --init to pull the stb submodule for the build.

Maybe reset third_party/Dawn back to the version on dev and explcitly add spdlog@79524ddd08a4ec981b7fea76afd08ee05f83755d (latest release right now).

Let me ask you again, you want these right?

Use SPDLOG_ACTIVE_LEVEL managed via CMake instead of the macro approach

Yes.

Default logging to OFF in Release mode unless explicitly enabled with a flag

Yes. I think you should throw an error when the use the enable logging flag and explain in the error that it is discouraged for release versions. Thereafter, create another flag that is explicitly for release logging and add it to the error message I mentioned.

That will keep things clean and communicate everything well.

Add clang-tidy enforcement for the no-console-printing rule

Yes.

Krasner · 2026-07-16T12:50:09Z

Yes please revert changes to Dawn. That can really break all the webgpu stuff

Ryan-Millard · 2026-07-16T13:19:56Z

@Krasner, maybe we can implement this in your upcoming PR.

Krasner · 2026-07-16T13:41:30Z

@Krasner, maybe we can implement this in your upcoming PR.

No let's keep this as a separate PR.

Ryan-Millard · 2026-07-16T13:51:05Z

@Krasner, maybe we can implement this in your upcoming PR.

No let's keep this as a separate PR.

That's not what I meant.😅😅

I should've explained it better. I meant that we should use spdlog in your PR once this is merged.

Krasner · 2026-07-16T13:52:24Z

@Krasner, maybe we can implement this in your upcoming PR.

No let's keep this as a separate PR.

That's not what I meant.😅😅

I should've explained it better. I meant that we should use spdlog in your PR once this is merged.

Haha in that case yes!

Krasner · 2026-07-16T14:07:46Z

@Prachi-Gupta2808 in CMakeLists.txt you need something like this:

add_compile_definitions(SPDLOG_USE_STD_FORMAT)
add_compile_definitions(
  $<$<CONFIG:Debug>:SPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_DEBUG>
  $<$<NOT:$<CONFIG:Debug>>:SPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_OFF>
)

in each file with logging you will need:

#include <spdlog/spdlog.h>

SPDLOG_DEBUG("my text"); // instead of std::cout

this way when the CMAKE_BUILD_TYPE=Release none of the debug prints will be compiled.

Ryan-Millard · 2026-07-16T15:54:46Z

@Prachi-Gupta2808 in CMakeLists.txt you need something like this:
add_compile_definitions(SPDLOG_USE_STD_FORMAT)
add_compile_definitions(
  $<$<CONFIG:Debug>:SPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_DEBUG>
  $<$<NOT:$<CONFIG:Debug>>:SPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_OFF>
)
in each file with logging you will need:
#include <spdlog/spdlog.h>

SPDLOG_DEBUG("my text"); // instead of std::cout
this way when the CMAKE_BUILD_TYPE=Release none of the debug prints will be compiled.

@Krasner, I think this would be better since we could build with settings that are not Debug and not Release:

add_compile_definitions(SPDLOG_USE_STD_FORMAT)
add_compile_definitions(
  $<$<CONFIG:Release>:SPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_OFF>
  $<$<NOT:$<CONFIG:Release>>:SPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_DEBUG>
)

I flipped it around to turn off in release and be on when not in release mode.

Krasner · 2026-07-16T15:56:41Z

@Prachi-Gupta2808 in CMakeLists.txt you need something like this:
add_compile_definitions(SPDLOG_USE_STD_FORMAT)
add_compile_definitions(
  $<$<CONFIG:Debug>:SPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_DEBUG>
  $<$<NOT:$<CONFIG:Debug>>:SPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_OFF>
)
in each file with logging you will need:
#include <spdlog/spdlog.h>

SPDLOG_DEBUG("my text"); // instead of std::cout
this way when the CMAKE_BUILD_TYPE=Release none of the debug prints will be compiled.
@Krasner, I think this would be better since we could build with settings that are not Debug and not Release:
add_compile_definitions(SPDLOG_USE_STD_FORMAT)
add_compile_definitions(
  $<$<CONFIG:Release>:SPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_OFF>
  $<$<NOT:$<CONFIG:Release>>:SPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_DEBUG>
)
I flipped it around to turn off in release and be on when not in release mode.

I'll let @Prachi-Gupta2808 implement this (and revert the dawn submodule change :) )

Prachi-Gupta2808 · 2026-07-17T04:00:46Z

@Ryan-Millard do we have any command like "validatepr" which can check formatting or lint checks before we raise a PR. if not can we implement this?

Krasner · 2026-07-17T04:02:11Z

@Ryan-Millard do we have any command like "validatepr" which can check formatting or lint checks before we raise a PR. if not can we implement this?

Run just format in the docker container

Ryan-Millard · 2026-07-17T04:04:30Z

@Ryan-Millard do we have any command like "validatepr" which can check formatting or lint checks before we raise a PR. if not can we implement this?

There's also editorconfig:check, but I'm not sure if it's in the Justfile yet, so use pnpm to run it.

…i-Gupta2808/Img2Num into chore/spdlog-debug-logging

Ryan-Millard · 2026-08-20T20:25:45Z

Hey @Prachi-Gupta2808!

Could you review this PR when you get a chance? It adds a configurable compile-time log level for the core (the thing behind the spdlog work you did - this should supersede/unblock that, so it's worth checking it fits what you had in mind).

What it does

New CMake cache var IMG2NUM_LOG_LEVEL (TRACE|DEBUG|INFO|WARN|ERROR|CRITICAL|OFF, default AUTO: Debug→TRACE, Release/MinSizeRel→OFF, else INFO)
Core now logs through its own named "img2num" spdlog logger via IMG2NUM_LOG_* macros (core/include/internal/log.h) instead of bare SPDLOG_* - so we never touch a consumer's default logger, and runtime level is auto-synced to the compiled level
Justfile recipes all take build_type and log_level positionally now
Side fixes: Debug wasm no longer fetches img2num.wasm.map at runtime (was breaking the react example with a JSON parse error), and the prebuild hook in packages/js is gone (it was clobbering non-default wasm builds with a Release rebuild)

How to test my changes

# C++: Release build with logs — the new headline combo
just clean cpp
just build-c-cpp Release TRACE
just console-cpp test.jpg        # should show trace/debug lines

# Confirm stripping still works
just build-c-cpp Release OFF
just console-cpp test.jpg        # no spdlog output at all

# Python (note: uv hides the cmake output; use -v if you want to see the level confirmation)
just build-py Release INFO
just console-py test.jpg

# JS/wasm — Debug should now work in the browser again
just build-packages-js Debug
just react-js start              # upload an image, no more SyntaxError

Args are positional (just quirk - log_level=TRACE after the recipe name silently becomes the build type!), so always just build-c-cpp Release TRACE, never log_level=TRACE.

Also a small quality-of-life thing you might like: example-apps/html-js now has a combined start script (pnpm start) that runs the esm/iife/umd servers all at once via pnpm's regex script matching (see the issue they had for that).

Thanks!

ripgrep exits 1 when no matches are found and 0 on matches, so the check failed on a clean tree and passed when violations existed. Handle the three exit codes explicitly (0 = violations -> fail with an error annotation, 1 = clean -> pass, 2+ = rg error -> propagate) instead of relying on the raw exit status under bash -e.

The explicit sdist manifest introduced when trimming the Dawn tarball never covered third_party/spdlog, so wheels built from the sdist (the path CI and plain `uv build` take, unlike `uv sync` which builds from the working tree) failed at configure: core's add_subdirectory pointed at a directory absent from the archive. Generalize the third_party excludes to recursive gitwildmatch patterns (**/.git, **/build/, **/test(s)/, ...), replacing the per-path Dawn entries. This also sweeps the nested submodule gitlinks under dawn/third_party that the old excludes never caught. The vk-gl-cts rationale from PR Ryan-Millard#562 still applies; it is now covered by the **/test/ pattern. Add wheel.license-files so the wheel's dist-info carries license texts for the statically linked third-party code (spdlog, Dawn and its vendored dependencies) alongside our own. Verified by building the wheel from the sdist (uv build) and inspecting the archive for spdlog, absence of .git entries, and collected licenses.

Replace the Node-based clang-format wrapper (scripts/format-cpp.js) with Python entry points in a new img2num_dev_scripts workspace member: - scripts/py/{lint_cpp,format_cpp}.py exposed as `uv run lint-cpp` / `uv run format_cpp`, with clang-tidy/clang-format 22.1.8 pinned as package dependencies instead of a root lint group - lint_cpp resolves TUs against one or more compile databases (-p, repeatable), verifies the clang-tidy config up front, skips files absent from every database with a warning, and serializes --fix runs to avoid concurrent header rewrites - img2num_root.py locates the repo root by marker files so scripts work from any CWD Config fixes shaken out by the new entry points: - .clang-tidy: restore identifier-length ignore patterns; split FunctionCase (snake_case free functions) from MethodCase (camelCase methods) - .clang-format: Standard c++20 -> c++17 to match the core's actual language level Remaining C++ diffs are mechanical reformatting from re-running clang-format 22 (braced-init closing brace placement, include ordering, macro line-length wrapping); no functional changes.

- Rename eslint/eslint:fix to lint:js/lint:js:fix - Add lint, lint:fix, lint:cpp, lint:cpp:fix meta-scripts using pnpm parallel regex dispatch (scoped to root via --filter=.) - Restore format:check as parallel dispatch of cpp/js check scripts; format_cpp now takes --fix for writes, default is check-only - Update CI to use lint:js (lint:cpp pending initial C++ format commit) - Remove no-raw-debug-output workflow (superseded by clang-tidy via lint:cpp)

The Python bindings were configured by scikit-build-core in a throwaway temp directory, so bindings/py/src/img2num_pybind.cpp never had a usable compile_commands.json and was silently skipped by lint-cpp. - Set tool.scikit-build.build-dir = "build-py/{wheel_tag}" so the CMake tree (and its exported compile database) survives the build. The {wheel_tag} placeholder keeps cibuildwheel's per-Python builds from clobbering each other's caches. - Teach lint_cpp to auto-discover all databases (build-c-cpp/, legacy build/, build-py/*/) and lint each TU against the first database that contains it. Explicit -p flags still override discovery. - Warn (instead of failing) on TUs absent from every database, e.g. bindings/js/src/wasm_wrapper.c, whose Emscripten flags clang-tidy can't consume. - Add build-py/ to .gitignore. img2num_pybind.cpp is now linted for the first time (passes clean). Note: the database may retain a stale include path from uv's isolated build env; harmless, since compilers skip nonexistent -I directories and pybind11 headers resolve from .venv instead. Rebuilding with `just build py` refreshes the database if resolution ever breaks.

bindings/js/src/wasm_wrapper.c was skipped because emcc injects its sysroot include paths inside the compiler driver, so they never appear in compile_commands.json and clang-tidy fails on the first standard-library include when replaying the recorded command. - Locate the Emscripten sysroot via $EMSDK, falling back to `em-config CACHE`, and pass it per-TU as --extra-arg=-isystem for files owned by build-wasm/. Native databases are unaffected. - Add build-wasm/ to auto-discovery, ordered last so TUs built for both native and wasm targets keep their native database mapping. - Degrade gracefully when no sysroot is found: build-wasm/ is excluded with a warning during discovery, but an explicit `-p build-wasm` is a hard error rather than a silent no-op. - Cache the sysroot lookup so em-config runs at most once across worker threads.

The build-docs job runs on a submodule-less checkout and consumes prebuilt wasm artifacts from the cmake-build job. But building the docs triggers packages/js's full `build`, whose prebuild hook runs `just build js` -> emcmake, which dies on the missing third_party/spdlog submodule. (The job's explicit `build:browser` step never hit this: pnpm pre-hooks match exact script names, so only the aggregate `build` has a prebuild.) Guard the prebuild behind IMG2NUM__CI_YML__PACKAGES_JS__SKIP_PREBUILD and set it on the Build Docusaurus step, so the Vite builds run against the downloaded build-wasm/ artifacts instead of regenerating them. Local `pnpm build` is unchanged (var unset -> full build runs).

This reverts commit 6e3dc31.

The prebuild script invoked `just`, which isn't installed in CI and would rebuild WASM with default flags, clobbering the workflow's explicit CMake configuration. The dependency is already orchestrated by `just build packages-js` locally and by the workflow's Configure/Build steps in CI, so the hook was redundant in both contexts (and a no-op under pnpm's default lifecycle settings). In its place, vite.config.js now fails fast at config time when build-wasm/<glue> is missing, replacing the cryptic "@wasm" alias resolution error with an actionable message. The closeBundle check in copyWasmPlugin remains for its narrower case: glue present without its sibling .wasm (mixed SINGLE_FILE states). Also: - hoist glueDir/outDir/WASM_BUILD_HINT as single sources of truth (fixes the stale "pnpm build:wasm" remediation hint) - gate wasmUrlPlugin and cjsWebgpuGuard at the plugins array instead of returning no-op plugin objects - document that LITERAL/MANGLED/mangledRe are intentionally not derived from one another

Ryan-Millard · 2026-08-26T20:24:07Z

These failing builds are tolerable.

Since the branch of this PR is on a fork, GitHub restricts the token rights to read-only, hence the failure.

…sting" This reverts commit c85c987.

chore(core-debug): add spdlog debug logging and ban raw std::cout

ae333a3

Prachi-Gupta2808 requested a review from Ryan-Millard as a code owner July 16, 2026 06:57

github-actions Bot added ci core build-system docs c/cpp labels Jul 16, 2026

This comment was marked as off-topic.

Sign in to view

This comment was marked as outdated.

Sign in to view

Prachi-Gupta2808 changed the base branch from main to dev July 16, 2026 07:09

Prachi-Gupta2808 and others added 2 commits July 16, 2026 12:47

Update core/include/internal/gpu.h

db35d9e

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

Update .github/workflows/no-raw-debug-output.yml

45e1a0f

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

coderabbitai Bot previously approved these changes Jul 16, 2026

View reviewed changes

Ryan-Millard requested a review from Krasner July 16, 2026 11:41

Ryan-Millard requested changes Jul 16, 2026

View reviewed changes

Prachi-Gupta2808 added 2 commits July 23, 2026 14:06

chore(core): add spdlog debug logging and ban raw std::cout

018bdcc

Merge branch 'chore/spdlog-debug-logging' of https://github.com/Prach…

2702dff

…i-Gupta2808/Img2Num into chore/spdlog-debug-logging

github-actions Bot added js/ts example-app Updates to code related to demonstration (example) applications labels Aug 20, 2026

Ryan-Millard added 4 commits August 20, 2026 22:28

temp

df2bcd6

github-actions Bot added the scripts label Aug 25, 2026

Ryan-Millard force-pushed the chore/spdlog-debug-logging branch from 14b0bd8 to 906a509 Compare August 25, 2026 17:37

Ryan-Millard and others added 15 commits August 25, 2026 20:27

revert: out of scope packages/js change for licensing

1eea3e4

fix(ci.yml): uv sync errors during lint

f8a361d

fix(editorconfig): ignore generated files

ff21cc8

fix: remove accidentally leftover file licenses.plugin.mjs

9915ab0

style: fix editorconfig errors

7959e05

revert: "ci: skip wasm CMake prebuild in docs job via env flag"

feafe09

This reverts commit 6e3dc31.

ci: fix docs build by pull spdlog

819935c

ci: fix docs build errors

fdd06b1

UNDO!!! -> trigger release.yml on this PR temporarily for testing

c85c987

Merge branch 'dev' into chore/spdlog-debug-logging

1adf366

style(vite.config.js): format the file

979b939

revert: "UNDO!!! -> trigger release.yml on this PR temporarily for te…

628d442

…sting" This reverts commit c85c987.

Ryan-Millard merged commit 47c8dd9 into Ryan-Millard:dev Aug 26, 2026
10 checks passed

This was referenced Aug 28, 2026

chore(main): release packages-js 0.5.0 #568

Open

chore(main): release cpp 0.3.0 #546

Open

Uh oh!

Conversation

Prachi-Gupta2808 commented Jul 16, 2026

Changes & Reason

Changes

Reason

Related Issues

Testing & Verification

Additional Resources

Uh oh!

This comment was marked as off-topic.

This comment was marked as outdated.

Uh oh!

Ryan-Millard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Prachi-Gupta2808 commented Jul 16, 2026

Uh oh!

Ryan-Millard commented Jul 16, 2026

Uh oh!

Krasner commented Jul 16, 2026

Uh oh!

Ryan-Millard commented Jul 16, 2026

Uh oh!

Krasner commented Jul 16, 2026

Uh oh!

Ryan-Millard commented Jul 16, 2026

Uh oh!

Krasner commented Jul 16, 2026

Uh oh!

Krasner commented Jul 16, 2026

Uh oh!

Ryan-Millard commented Jul 16, 2026

Uh oh!

Krasner commented Jul 16, 2026

Uh oh!

Prachi-Gupta2808 commented Jul 17, 2026

Uh oh!

Krasner commented Jul 17, 2026

Uh oh!

Ryan-Millard commented Jul 17, 2026

Uh oh!

Ryan-Millard commented Aug 20, 2026

Uh oh!

Ryan-Millard commented Aug 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ryan-Millard left a comment •

edited

Loading