Add a public-header CI gate matching stricter warning flags#1503
Open
bmehta001 wants to merge 7 commits into
Open
Add a public-header CI gate matching stricter warning flags#1503bmehta001 wants to merge 7 commits into
bmehta001 wants to merge 7 commits into
Conversation
…f-contained Consumers that embed the SDK (e.g. ONNX Runtime / Foundry Local) compile their own translation units -- which include our public headers -- under strict warning flags: -Wall -Wextra -Werror on GCC/Clang (plus -Wshorten-64-to-32 on Clang) and /W4 /WX on MSVC, suppressing third-party headers via -isystem / /external:W0. When that suppression is defeated (include order, PCH, or NO_SYSTEM_FROM_IMPORTED), any warning or missing include in our headers breaks the consumer build. This adds a CI gate that compiles every public header on its own, with no -isystem suppression, under those flags on GCC, Clang, and MSVC, so header issues surface here instead of at integration time: - tests/headers/check_public_headers.sh (GCC + Clang) - tests/headers/check_public_headers.cmd (MSVC) - .github/workflows/public-header-gate.yml The gate caught one real self-containment bug: CompliantByDefaultFilterApi.hpp uses std::vector<uint8_t> but did not include <cstdint>, so it only compiled when something else pulled <cstdint> in first. Added the include. VariantType.hpp is excluded: it is an implementation fragment included by Variant.hpp (which defines VariantMap/VariantArray and the needed std headers first), not a standalone header. Validated locally: all 40 public headers pass (39 compiled + VariantType excluded) on g++, clang++, and MSVC cl /W4 /WX. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a CI “public header gate” intended to catch warning-as-error and self-containment problems in exported headers (matching strict consumer builds like ONNX Runtime), so issues surface in this repo before downstream integration failures.
Changes:
- Add GCC/Clang standalone public-header compilation gate (
tests/headers/check_public_headers.sh). - Add MSVC standalone public-header compilation gate (
tests/headers/check_public_headers.cmd). - Add GitHub Actions workflow to run the gate on Linux and Windows, plus fix a missing
<cstdint>include in a public header.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
tests/headers/check_public_headers.sh |
New Linux header-compilation gate script for GCC/Clang. |
tests/headers/check_public_headers.cmd |
New Windows header-compilation gate script for MSVC. |
lib/include/public/CompliantByDefaultFilterApi.hpp |
Adds missing <cstdint> include for header self-containment. |
.github/workflows/public-header-gate.yml |
New CI workflow to run the public-header gate on Ubuntu + Windows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…lags, fix MSVC includes - Gate both *.hpp and *.h so the flat C API headers (mat.h, CommonFields.h) are covered, not just *.hpp. Both compile clean standalone under the gate flags. - Drop -Wno-unused-but-set-variable from the GCC/Clang flags: no public header relies on it, so removing it makes the gate stricter. Kept -Wno-unused-parameter (mirrors this repo's WARN_FLAGS; intentional unused params use UNREFERENCED_PARAMETER). - MSVC gate: add /I lib\include so headers that conditionally pull mat/config.h (e.g. CsProtocol_types.hpp) exercise the same include graph as the GCC/Clang gate. Validated locally: 41/41 public headers pass on g++, clang++, and MSVC cl /W4 /WX. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Instead of a standalone workflow, add the header gate as an isolated job in the workflows that already run on the same triggers: - build-posix-latest.yml gains a 'public-headers' job (GCC/Clang on ubuntu). - test-win-latest.yml gains a 'public-headers' job (MSVC on windows). Removes .github/workflows/public-header-gate.yml. The gate scripts under tests/headers/ are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bmehta001
changed the title
…lags The gate suppressed -Wunused-parameter, which is not part of the strict consumer flag set it claims to mirror (-Wall -Wextra -Werror). That hid a real break: NullObjects.hpp overrides left parameters unused because the old UNREFERENCED_PARAMETER macro expanded to nothing on GCC/Clang, so a consumer including LogManager.hpp with plain -Wall -Wextra -Werror failed to compile while the gate passed. With the macro now expanding to (void)(...) (merged from main), the headers are clean without the suppression; drop it so the gate actually catches this class of consumer break. Verified: 41/41 public headers pass standalone on g++ and clang++ without -Wno-unused-parameter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The gate could silently pass without compiling anything: with nullglob the shell header globs expand to nothing (and the batch FOR loop runs zero times) when the computed public-header directory is wrong, so a miscomputed path reported success while testing nothing. Both scripts now validate the public-header directory exists and fail if zero headers were compiled. Also give the MSVC script a unique per-invocation work directory (%TEMP%\pubhdrgate_<rand>) so concurrent runs on the same machine cannot clobber each other's temporary translation unit, and clean it up on exit. Files: tests/headers/check_public_headers.sh, tests/headers/check_public_headers.cmd Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
Prompted by @baijumeswani's review comment on #1501:
This PR does exactly that: it adds a CI gate that compiles our public headers under the same strict warning flags ONNX Runtime applies, so header issues surface in this repo's pipeline instead of at ORT integration time.
Summary
Consumers that embed the SDK (e.g. ONNX Runtime / Foundry Local) compile their own translation units — which
#includeour public headers — under strict warning flags. From ONNX Runtime's CMake, its C++ targets use:-Wall -Wextra+COMPILE_WARNING_AS_ERROR(=-Werror); Clang also-Wshorten-64-to-32/W4 /WXThird-party headers are normally suppressed (
-isystem//external:W0) only if the SDK is consumed as a proper external/imported target. When that suppression is defeated — include order, precompiled headers, orNO_SYSTEM_FROM_IMPORTED— any warning or missing include in our headers breaks the consumer build.This PR adds a CI gate that compiles every public header on its own, with no
-isystemsuppression, under those flags on GCC, Clang, and MSVC — so header issues surface here instead of at integration time.What's added
tests/headers/check_public_headers.sh— GCC + Clang (-std=c++17 -Wall -Wextra -Werror; Clang also-Wshorten-64-to-32).tests/headers/check_public_headers.cmd— MSVC (/W4 /WX, STL suppressed via/external:W0so only our headers gate).public-headersjob added to the existingbuild-posix-latest.yml(GCC/Clang) andtest-win-latest.yml(MSVC) workflows — no new standalone workflow file.Bug the gate caught
CompliantByDefaultFilterApi.hppusesstd::vector<uint8_t>but did not include<cstdint>, so it only compiled when something else pulled<cstdint>in first — exactly the kind of latent self-containment issue that breaks a consumer depending on include order. Added the include.VariantType.hppis excluded from the standalone gate: it is an implementation fragment included byVariant.hpp(which definesVariantMap/VariantArrayand the neededstdheaders before including it), not a standalone header.Validation
All 40 public headers pass (39 compiled +
VariantType.hppexcluded) locally on:-Wall -Wextra -Werror): 39/39-Wshorten-64-to-32): 39/39/W4 /WX, STL external): 39/39Relationship to #1501
Complementary. #1501 makes the headers warning-clean and marks the exported include dir
SYSTEM. This PR locks that in with a gate matching ORT's actual flags and additionally verifies self-containment (which surfaced the<cstdint>bug).Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com