Skip to content

[GLUTEN-11287][VL] Use IWYU tool to check code format#11287

Closed
xinghuayu007 wants to merge 1 commit into
apache:mainfrom
xinghuayu007:my/iwyu_format
Closed

[GLUTEN-11287][VL] Use IWYU tool to check code format#11287
xinghuayu007 wants to merge 1 commit into
apache:mainfrom
xinghuayu007:my/iwyu_format

Conversation

@xinghuayu007

@xinghuayu007 xinghuayu007 commented Dec 12, 2025

Copy link
Copy Markdown
Contributor

What changes are proposed in this pull request?

Use IWYU tool to check code format

How was this patch tested?

Related issue: #11287

@xinghuayu007 xinghuayu007 changed the title [GLUTEN-][Velox] Use IWYU tool to check code format [GLUTEN-11287][Velox] Use IWYU tool to check code format Dec 12, 2025
@xinghuayu007
xinghuayu007 force-pushed the my/iwyu_format branch 8 times, most recently from d4c97f3 to 6e2680a Compare December 12, 2025 09:35
@philo-he philo-he changed the title [GLUTEN-11287][Velox] Use IWYU tool to check code format [GLUTEN-11287][VL] Use IWYU tool to check code format Dec 12, 2025
Comment thread .github/workflows/iwyu.yml Outdated
@xinghuayu007
xinghuayu007 force-pushed the my/iwyu_format branch 4 times, most recently from a51b2eb to 4ac46bc Compare December 12, 2025 10:26
Comment thread .github/workflows/iwyu.yml Outdated
@xinghuayu007
xinghuayu007 force-pushed the my/iwyu_format branch 11 times, most recently from 1884480 to ba200ee Compare December 15, 2025 07:29
@xinghuayu007
xinghuayu007 force-pushed the my/iwyu_format branch 20 times, most recently from bbef1fc to 6aaab1a Compare December 17, 2025 09:46

@philo-he philo-he left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we enable IWYU in a way embed in cmake? We can enable the check only when code changes are made under cpp/ path.

if(ENABLE_IWYU)
    find_program(IWYU_PATH NAMES include-what-you-use iwyu)
    if(IWYU_PATH)
        set(CMAKE_CXX_INCLUDE_WHAT_YOU_USE ${IWYU_PATH})
    endif()
endif()

@xinghuayu007

Copy link
Copy Markdown
Contributor Author

Can we enable IWYU in a way embed in cmake? We can enable the check only when code changes are made under cpp/ path.

if(ENABLE_IWYU)
    find_program(IWYU_PATH NAMES include-what-you-use iwyu)
    if(IWYU_PATH)
        set(CMAKE_CXX_INCLUDE_WHAT_YOU_USE ${IWYU_PATH})
    endif()
endif()

Compiling include-what-you-use needs clang and source code. Currently design can ensure only check modified cpp files.

@philo-he philo-he left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the update. The current implementation looks a bit complex to me. Do you know how IWYU is enabled in Velox?

Comment thread dev/iwyu_tool.py Outdated
Comment thread dev/iwyu_tool.py Outdated
Comment thread dev/iwyu_tool.py Outdated
Comment thread dev/iwyu_tool.py Outdated
Comment thread dev/iwyu_tool.py Outdated
Comment thread dev/iwyu_tool.py Outdated
Comment thread .github/workflows/iwyu.yml

@philo-he philo-he left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some comments. Thanks.

Comment thread dev/builddeps-veloxbe.sh Outdated
Comment thread .github/workflows/iwyu.yml Outdated
Comment thread .github/workflows/iwyu.yml Outdated
Comment thread .github/workflows/iwyu.yml Outdated
Comment thread .github/workflows/iwyu.yml Outdated
Comment thread dev/builddeps-veloxbe.sh Outdated
@@ -237,7 +242,13 @@ function build_gluten_cpp {
rm -rf build
mkdir build
cd build

if [ $USE_CLANG = "ON" ]; then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems no need to add a new build option. Just let the compilation depend on the setting for CC and CXX in the env.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To separate the builds, we can run ./dev/builddeps-veloxbe.sh build_velox xxx with gcc to utilize vcpkg cache in docker. Then, run ./dev/builddeps-veloxbe.sh build_gluten_cpp with clang to build Gluten c++ code.

Comment thread dev/ci-velox-buildstatic-centos-8-clang.sh Outdated
Comment thread .github/workflows/iwyu.yml Outdated
Comment thread .github/workflows/iwyu.yml
@weiting-chen weiting-chen mentioned this pull request Mar 25, 2026
60 tasks
@philo-he

Copy link
Copy Markdown
Member

Errors reported by CI.

/usr/bin/clang++: /lib64/libtinfo.so.5: no version information available (required by /usr/bin/clang++)
/__w/gluten/gluten/cpp/velox/jni/VeloxJniWrapper.cc:1042:24: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
 1042 |       otherTables[t] = std::move(hashTableBuilders[t]->uniqueTable());
      |                        ^
/__w/gluten/gluten/cpp/velox/jni/VeloxJniWrapper.cc:1042:24: note: remove std::move call here
 1042 |       otherTables[t] = std::move(hashTableBuilders[t]->uniqueTable());
      |                        ^~~~~~~~~~                                   ~
1 error generated.
[57/94] Building CXX object velox/CMakeFiles/velox.dir/memory/VeloxMemoryManager.cc.o
/usr/bin/clang++: /lib64/libtinfo.so.5: no version information available (required by /usr/bin/clang++)
[58/94] Building CXX object velox/CMakeFiles/velox.dir/memory/VeloxColumnarBatch.cc.o
/usr/bin/clang++: /lib64/libtinfo.so.5: no version information available (required by /usr/bin/clang++)
[59/94] Building CXX object velox/CMakeFiles/velox.dir/operators/functions/RegistrationAllFunctions.cc.o
/usr/bin/clang++: /lib64/libtinfo.so.5: no version information available (required by /usr/bin/clang++)
ninja: build stopped: subcommand failed.

@xinghuayu007

Copy link
Copy Markdown
Contributor Author

Errors reported by CI.

/usr/bin/clang++: /lib64/libtinfo.so.5: no version information available (required by /usr/bin/clang++)
/__w/gluten/gluten/cpp/velox/jni/VeloxJniWrapper.cc:1042:24: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
 1042 |       otherTables[t] = std::move(hashTableBuilders[t]->uniqueTable());
      |                        ^
/__w/gluten/gluten/cpp/velox/jni/VeloxJniWrapper.cc:1042:24: note: remove std::move call here
 1042 |       otherTables[t] = std::move(hashTableBuilders[t]->uniqueTable());
      |                        ^~~~~~~~~~                                   ~
1 error generated.
[57/94] Building CXX object velox/CMakeFiles/velox.dir/memory/VeloxMemoryManager.cc.o
/usr/bin/clang++: /lib64/libtinfo.so.5: no version information available (required by /usr/bin/clang++)
[58/94] Building CXX object velox/CMakeFiles/velox.dir/memory/VeloxColumnarBatch.cc.o
/usr/bin/clang++: /lib64/libtinfo.so.5: no version information available (required by /usr/bin/clang++)
[59/94] Building CXX object velox/CMakeFiles/velox.dir/operators/functions/RegistrationAllFunctions.cc.o
/usr/bin/clang++: /lib64/libtinfo.so.5: no version information available (required by /usr/bin/clang++)
ninja: build stopped: subcommand failed.

Thanks. Done.

@github-actions

Copy link
Copy Markdown

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

This PR was auto-closed because it has been stalled for 10 days with no activity. Please feel free to reopen if it is still valid. Thanks.

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