Skip to content

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

Closed
kevinwilfong wants to merge 1 commit into
apache:mainfrom
kevinwilfong:velox-system-include
Closed

[VL] Include headers from INSTALL_PREFIX as system includes on Mac#12423
kevinwilfong wants to merge 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.

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.

2 participants