Skip to content

Add configure-time check for protobuf when --enable-pax is specified - #1151

Merged
tuhaihe merged 1 commit into
apache:mainfrom
yjhjstz:fix_pax_check
Jun 25, 2025
Merged

Add configure-time check for protobuf when --enable-pax is specified#1151
tuhaihe merged 1 commit into
apache:mainfrom
yjhjstz:fix_pax_check

Conversation

@yjhjstz

@yjhjstz yjhjstz commented Jun 10, 2025

Copy link
Copy Markdown
Member

When building with --enable-pax, the build now checks for the presence of protobuf (version >= 3.5.0) using pkg-config. If protobuf is not found, configure will fail with an appropriate error message. This ensures that missing dependencies are caught early in the build process.

Fixes #1148

What does this PR do?

Type of Change

  • Bug fix (non-breaking change)
  • New feature (non-breaking change)
  • Breaking change (fix or feature with breaking changes)
  • Documentation update

Breaking Changes

Test Plan

  • Unit tests added/updated
  • Integration tests added/updated
  • Passed make installcheck
  • Passed make -C src/test installcheck-cbdb-parallel

Impact

Performance:

User-facing changes:

Dependencies:

Checklist

Additional Context

CI Skip Instructions

tuhaihe
tuhaihe reviewed Jun 10, 2025
View reviewed changes
Comment thread configure.ac
@tuhaihe
tuhaihe requested a review from edespino June 10, 2025 06:29
edespino
edespino previously requested changes Jun 10, 2025
View reviewed changes

@edespino edespino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The check should not be verbose when the --enable-pax checks for the dependencies (example: protobuf) pass. It should only be verbose when the dependencies are not met.

The following:

checking whether to build with PAX support ... yes
configure: Checking for protobuf >= 3.5.0 for PAX support...
checking for protobuf >= 3.5.0... yes
configure: protobuf found

Should only have:

checking whether to build with PAX support ... yes

@yjhjstz

yjhjstz commented Jun 10, 2025

Copy link
Copy Markdown
Member Author

The check should not be verbose when the --enable-pax checks for the dependencies (example: protobuf) pass. It should only be verbose when the dependencies are not met.

The following:

checking whether to build with PAX support ... yes
configure: Checking for protobuf >= 3.5.0 for PAX support...
checking for protobuf >= 3.5.0... yes
configure: protobuf found

Should only have:

checking whether to build with PAX support ... yes

simplify，done.

@yjhjstz
yjhjstz requested a review from edespino June 10, 2025 14:59
@yjhjstz
yjhjstz requested a review from tuhaihe June 10, 2025 15:09
tuhaihe
tuhaihe reviewed Jun 11, 2025
View reviewed changes
Comment thread configure.ac
@tuhaihe

tuhaihe commented Jun 11, 2025

Copy link
Copy Markdown
Member

Hey @yjhjstz, Just curious if there's any chance we could implement the function ourselves, instead of introducing the new file config/ax_compare_version.m4?

@yjhjstz

yjhjstz commented Jun 11, 2025

Copy link
Copy Markdown
Member Author

Hey @yjhjstz, Just curious if there's any chance we could implement the function ourselves, instead of introducing the new file config/ax_compare_version.m4?

you can see config/*.m4, for example config/ax_cxx_compile_stdcxx.m4, from https://www.gnu.org/software/autoconf-archive/ax_cxx_compile_stdcxx.html, it's common practice.

tuhaihe
tuhaihe approved these changes Jun 12, 2025
View reviewed changes

@tuhaihe tuhaihe 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.

Tested and ran well. Thanks!

@tuhaihe

tuhaihe commented Jun 17, 2025

Copy link
Copy Markdown
Member

Updates: #1165 - this PR will support building Cloudberry with PAX directly under g++ 8 under Rocky Linux 8. So, no need to compare the g++ version for PAX.

@yjhjstz
yjhjstz requested review from gfphoenix78 and jiaqizho June 20, 2025 03:20
yjhjstz
yjhjstz commented Jun 23, 2025
View reviewed changes

@yjhjstz yjhjstz left a comment •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@edespino Could you please review the updated changes and approve if it looks good?

@yjhjstz @tuhaihe
Add configure-time check for protobuf, zstd, cmake when --enable-pax …
cdfea80 
…is specified

When building with --enable-pax, the build now checks for the presence
of protobuf (version >= 3.5.0) using pkg-config. If protobuf is not found,
configure will fail with an appropriate error message. This ensures that
missing dependencies are caught early in the build process.
@tuhaihe
tuhaihe dismissed edespino’s stale review June 25, 2025 07:55

Already has help reviewed this PR.

@tuhaihe
tuhaihe merged commit 358eb55 into apache:main Jun 25, 2025
26 checks passed
@yjhjstz yjhjstz mentioned this pull request Jul 16, 2025
Open
8 tasks
@yjhjstz
yjhjstz deleted the fix_pax_check branch November 17, 2025 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tuhaihe tuhaihe tuhaihe approved these changes
@my-ship-it my-ship-it my-ship-it approved these changes
@edespino edespino Awaiting requested review from edespino
@jiaqizho jiaqizho Awaiting requested review from jiaqizho
@gfphoenix78 gfphoenix78 Awaiting requested review from gfphoenix78

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] --enable_pax fails to detect missing protobuf development libraries

4 participants

@yjhjstz @tuhaihe @edespino @my-ship-it