Skip to content

Conversation

@xinghuayu007
Copy link
Contributor

@xinghuayu007 xinghuayu007 commented Dec 12, 2025

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
rpm --import http://opensource.wandisco.com/RPM-GPG-KEY-WANdisco
wget http://opensource.wandisco.com/centos/7/git/x86_64/wandisco-git-release-7-2.noarch.rpm
rpm -Uvh wandisco-git-release-7-2.noarch.rpm
yum install -y git
Copy link
Member

@PHILO-HE PHILO-HE Dec 12, 2025

Choose a reason for hiding this comment

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

You can just add source /opt/rh/rh-git227/enable to make git 2.27 used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@xinghuayu007 xinghuayu007 force-pushed the my/iwyu_format branch 4 times, most recently from a51b2eb to 4ac46bc Compare December 12, 2025 10:26
yum install -y llvm15 llvm15-devel clang15 lld15
clang15 --version
mkdir -p /work/.ccache
bash dev/ci-velox-buildstatic-centos-7-with-clang.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to build a new binary? can we reuse existing workflow and add the check there?

@zhouyuan

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xinghuayu007 it seems we could use below GHA to check IWYU? https://github.com/marketplace/actions/include-what-you-use-github-action

Thanks for your comments. I try to use GHA to check IWYU.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to build a new gluten binary here? can we add to

?

you may change dev/ci-velox-buildstatic-centos-7.sh as clang build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xinghuayu007 it seems we could use below GHA to check IWYU? https://github.com/marketplace/actions/include-what-you-use-github-action

I found this GHA can't only check modified files, it checks all the files which costs a lot of time.

@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 5 times, most recently from 67713da to 4cbd339 Compare December 19, 2025 07:06
Copy link
Member

@PHILO-HE PHILO-HE left a comment

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 xinghuayu007 force-pushed the my/iwyu_format branch 21 times, most recently from 6d5c423 to 9596758 Compare December 26, 2025 02:58
@xinghuayu007
Copy link
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.

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