Skip to content

[VL] Include headers from INSTALL_PREFIX as system includes on Mac#12423

Merged
zhouyuan merged 1 commit into
apache:mainfrom
kevinwilfong:velox-system-include
Jul 24, 2026
Merged

[VL] Include headers from INSTALL_PREFIX as system includes on Mac#12423
zhouyuan merged 1 commit into
apache:mainfrom
kevinwilfong:velox-system-include

Conversation

@kevinwilfong

@kevinwilfong kevinwilfong commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

What changes are proposed in this pull request?

#12105 introduced the flag -DCMAKE_NO_SYSTEM_FROM_IMPORTED=ON to Velox builds. This has the unintended side effect of including dependencies' header files as non-system includes which means that warnings in the build are promoted to errors, e.g.

  In file included from t.cc:1:
  In file included from …/deps-install/include/re2/re2.h:220:
  In file included from …/deps-install/include/absl/base/call_once.h:40:
  In file included from …/deps-install/include/absl/base/nullability.h:153:
  In file included from …/deps-install/include/absl/base/internal/nullability_impl.h:22:
  …/deps-install/include/absl/meta/type_traits.h:511:36: error: builtin __is_trivially_relocatable is deprecated; use __builtin_is_cpp_trivially_relocatable instead
  [-Werror,-Wdeprecated-builtins]
    511 |     : std::integral_constant<bool, __is_trivially_relocatable(T)> {};
        |                                    ^
  1 error generated.

To fix this, this PR proposes explicitly including the header files under ${INSTALL_PREFIX}/include from dependencies as system includes, suppressing these errors which we have limited control over.

How was this patch tested?

Executed ./dev/builddeps-veloxbe.sh on a Mac with Clang 21, verified we no longer got errors from warnings in dependencies. Combined with my other recent changes
#12421
#12422
I was able to successfully run ./dev/builddeps-veloxbe.sh on an otherwise fresh checkout.

Was this patch authored or co-authored using generative AI tooling?

Co-authored with Claude Opus 4.8

Generated-by: Claude Opus 4.8

@kevinwilfong kevinwilfong changed the title [VL] Include headers from INSTALL_PREFIX as system includes [VL] Include headers from INSTALL_PREFIX as system includes on Mac Jul 1, 2026
@zhouyuan

zhouyuan commented Jul 2, 2026

Copy link
Copy Markdown
Member

@kevinwilfong thanks, please check if this fix from @jackylee-ch works
#12331

@kevinwilfong

Copy link
Copy Markdown
Collaborator Author

Looks like it fixes it, closing this out.

@kevinwilfong kevinwilfong reopened this Jul 21, 2026
@kevinwilfong
kevinwilfong force-pushed the velox-system-include branch from c03c85b to 2459258 Compare July 21, 2026 23:08
@kevinwilfong

Copy link
Copy Markdown
Collaborator Author

@zhouyuan I'm reopening this PR, I had visibly spot checked the PR you referenced and it looked like it fixed the issue, but because it uses -I instead of -isystem I ran into this issue again when I tried to do a build in a fresh checkout. Could you please take a look?

@zhouyuan
zhouyuan requested a review from jackylee-ch July 22, 2026 08:04
@kevinwilfong

Copy link
Copy Markdown
Collaborator Author

@zhouyuan thanks for the review! could you help me to merge the change, I'm afraid I don't have permissions.

@zhouyuan
zhouyuan merged commit a9638bf into apache:main Jul 24, 2026
51 checks passed
@prestodb-ci

prestodb-ci commented Jul 24, 2026

Copy link
Copy Markdown

Rebase job

zhouyuan added a commit to zhouyuan/gluten that referenced this pull request Jul 24, 2026
@zhouyuan

Copy link
Copy Markdown
Member

@kevinwilfong it looks like this patch will break the macos build: the system level fmt will be picked instead of the one under INSTALL_PREFIX
https://github.com/apache/gluten/actions/runs/30098729752/job/89499174522

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants