Skip to content

Test/fix clang link workflow#212

Open
iaojnh wants to merge 12 commits intomainfrom
test/fix-clang-link-workflow
Open

Test/fix clang link workflow#212
iaojnh wants to merge 12 commits intomainfrom
test/fix-clang-link-workflow

Conversation

@iaojnh
Copy link
Collaborator

@iaojnh iaojnh commented Mar 11, 2026

Greptile Summary

This PR makes Clang's C++ standard library selection platform-aware in cmake/bazel.cmake (using libc++ on Apple/Android and libstdc++ elsewhere) to fix link failures when building with Clang on Linux. It also cleans up trailing whitespace across _target_link_libraries. However, a mxschmitt/action-tmate@v3 debug step was accidentally left in the CI workflow, which is a blocking and security-sensitive issue that must be resolved before merge.

Key changes:

  • cmake/bazel.cmake: Introduces CLANG_USE_LIBCXX option and CLANG_STDLIB_OPTION variable so that -stdlib=libc++ is no longer unconditionally applied to all Clang builds — Linux Clang now defaults to -stdlib=libstdc++.
  • cmake/bazel.cmake: Whitespace-only cleanup (trailing spaces removed) inside _target_link_libraries.
  • .github/workflows/main.yml: Inadvertent addition of an interactive tmate debug step — this will stall all three matrix CI builds after the Python test step and exposes the runner environment (including secrets) to anyone who connects during the wait window.

Confidence Score: 1/5

  • Not safe to merge — a tmate debug session is embedded in the CI workflow, blocking all builds and exposing secrets.
  • The cmake changes are logically sound, but the accidentally committed tmate step in main.yml is a hard blocker: every CI run triggered after merge would stall indefinitely and give anyone who connects SSH access to the runner environment and its secrets.
  • .github/workflows/main.yml requires immediate attention to remove the tmate debug step before this PR can be merged.

Important Files Changed

Filename Overview
.github/workflows/main.yml Added a mxschmitt/action-tmate@v3 debug step that will block every CI run indefinitely and exposes runner secrets — must be removed before merge.
cmake/bazel.cmake Makes Clang stdlib selection platform-aware (libc++ on Apple/Android, libstdc++ elsewhere) instead of hard-coding libc++; also strips trailing whitespace throughout the function. Minor issue: the -stdlib flag is still passed to C (not just C++) compilation units via the shared _COMPILER_FLAGS variable.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[CMake Configure] --> B{APPLE OR ANDROID?}
    B -- Yes --> C[CLANG_USE_LIBCXX = ON\nCLANG_STDLIB_OPTION = -stdlib=libc++]
    B -- No --> D[CLANG_USE_LIBCXX = OFF\nCLANG_STDLIB_OPTION = -stdlib=libstdc++]
    C --> E[_COMPILER_FLAGS\nClang: -fcolor-diagnostics; CLANG_STDLIB_OPTION]
    D --> E
    E --> F[BAZEL_CC_STRICT_LINK_FLAGS\nClang: CLANG_STDLIB_OPTION]
    E --> G[BAZEL_CC_UNSTRICT_LINK_FLAGS\nClang: CLANG_STDLIB_OPTION]
    F --> H[Targets using strict linking]
    G --> I[Targets using unstrict linking]
Loading

Last reviewed commit: b0b1a09

Greptile also left 2 inline comments on this PR.

Comment on lines +154 to +155
- name: Setup tmate session
uses: mxschmitt/action-tmate@v3
Copy link

Choose a reason for hiding this comment

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

Debug tmate session should not be merged

The mxschmitt/action-tmate step is a debugging tool that opens an interactive SSH/tmux session into the CI runner. Merging this into main will:

  1. Block every CI run — the step waits indefinitely (until the job timeout) for an SSH connection, stalling all three matrix builds after the Python tests complete.
  2. Expose secrets — the tmate session has access to the full runner environment, including any repository secrets mounted in GITHUB_ENV or available via the GitHub token. Anyone who connects during the wait window can inspect or exfiltrate those values.

This is clearly a temporary debugging aid that was accidentally left in the PR. It must be removed before merging.

Suggested change
- name: Setup tmate session
uses: mxschmitt/action-tmate@v3

(Remove the entire step.)

set(
_COMPILER_FLAGS
"$<$<C_COMPILER_ID:Clang>:-fcolor-diagnostics;-stdlib=libc++>"
"$<$<C_COMPILER_ID:Clang>:-fcolor-diagnostics;${CLANG_STDLIB_OPTION}>"
Copy link

Choose a reason for hiding this comment

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

-stdlib flag incorrectly applied to C compilation

The generator expression $<$<C_COMPILER_ID:Clang>:-fcolor-diagnostics;${CLANG_STDLIB_OPTION}> passes -stdlib=libc++ (or -stdlib=libstdc++) to the C compiler via the $<$<COMPILE_LANGUAGE:C>:...> wrapper a few lines below. The -stdlib flag is meaningful only for C++ compilation; when passed to a C compile unit it is silently ignored by Clang today but may produce a warning with stricter driver flags or future compiler versions.

The C-specific generator expression should only include -fcolor-diagnostics:

Suggested change
"$<$<C_COMPILER_ID:Clang>:-fcolor-diagnostics;${CLANG_STDLIB_OPTION}>"
"$<$<C_COMPILER_ID:Clang>:-fcolor-diagnostics>"

The CXX line (which also uses _COMPILER_FLAGS) can keep ${CLANG_STDLIB_OPTION}. If you want to keep a single variable for the whole block, split it into separate C and CXX variables instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants