Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 25, 2025
Merged

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

merged 1 commit into from
Jun 25, 2025

Conversation

yjhjstz
Copy link
Member

@yjhjstz yjhjstz commented Jun 10, 2025

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
@tuhaihe tuhaihe requested a review from edespino June 10, 2025 06:29
edespino
edespino previously requested changes Jun 10, 2025
View reviewed changes
Copy link
Contributor

@edespino edespino left a comment

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
Copy link
Member Author

yjhjstz commented Jun 10, 2025

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
@tuhaihe
Copy link
Member

tuhaihe commented Jun 11, 2025

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
Copy link
Member Author

yjhjstz commented Jun 11, 2025

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
Copy link
Member

@tuhaihe tuhaihe left a comment

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
Copy link
Member

tuhaihe commented Jun 17, 2025

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
Copy link
Member Author

@yjhjstz yjhjstz left a comment •
edited
Loading

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
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