GH-37535: [C++][Parquet] Add missing "thrift" dependency in parquet.pc #37603

kou · 2023-09-07T08:02:28Z

Rationale for this change

GH-37399 removed "thrift" pkg-config package dependency from "parquet" pkg-config package accidentally.
Before GH-37399, "arrow" pkg-config packages has "thrift" pkg-config package dependency as Requires.private.

What changes are included in this PR?

Add "thrift" to Requires.private of parquet.pc.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

Closes: [R][CI] Nightly CI jobs failing with Thrift error #37535

kou · 2023-09-07T08:02:45Z

@github-actions crossbow submit test-r-dev-duckdb

github-actions · 2023-09-07T08:03:00Z

⚠️ GitHub issue #37535 has been automatically assigned in GitHub to PR creator.

github-actions · 2023-09-07T08:05:17Z

Revision: e69b819

Submitted crossbow builds: ursacomputing/crossbow @ actions-ab2eef7c21

Task	Status
test-r-dev-duckdb

kou · 2023-09-07T09:13:28Z

@github-actions crossbow submit test-r-gcc-*

github-actions · 2023-09-07T09:16:16Z

Revision: e69b819

Submitted crossbow builds: ursacomputing/crossbow @ actions-2be519e29b

Task	Status
test-r-gcc-11
test-r-gcc-12

mapleFU · 2023-09-07T13:23:55Z

Might now related, after #37461 , I changed some parquet generated thrift. Don't know would it affect the parquet.pc

wgtmac · 2023-09-07T15:15:01Z

Might now related, after #37461 , I changed some parquet generated thrift. Don't know would it affect the parquet.pc

I don't think so.

wgtmac · 2023-09-07T15:19:06Z

cpp/CMakeLists.txt

@@ -533,6 +533,9 @@ set(ARROW_TESTING_PC_LIBS "")
 set(ARROW_TESTING_PC_REQUIRES "")

 # For parquet.pc.
+set(PARQUET_PC_CFLAGS "")
+set(PARQUET_PC_CFLAGS_PRIVATE " -DPARQUET_STATIC")


Is this a hack? Or Cflags.private requires only static linking?

This is not a hack.
This is a normal .pc usage.

Or Cflags.private requires only static linking?

Right.
pkg-config --cflags parquet uses only Cflags and
pkg-config --cflags --static parquet uses both of Cflags and Cflags.private.

kou · 2023-09-08T01:44:46Z

#37461 isn't related. :-)

…quet.pc

kou · 2023-09-08T02:17:18Z

@github-actions crossbow submit test-r-dev-duckdb test-r-gcc-*

github-actions · 2023-09-08T02:20:11Z

Revision: b349d80

Submitted crossbow builds: ursacomputing/crossbow @ actions-4c4b8bc409

Task	Status
test-r-dev-duckdb
test-r-gcc-11
test-r-gcc-12

kou · 2023-09-08T02:56:52Z

r/configure

@assignUser FYI: Changes for this file are for using pkg-config for not only arrow.pc but also parquet.pc/arrow-dataset.pc/arrow-acero.pc/arrow-substrait.pc. libarrow.a doesn't depend on Thrift. libparquet.a depends on Thrift. So I removed thrift from Requires.private of arrow.pc by #37399. But I forgot to add thrift to Requires.private of parquet.pc. This is fixed by other changes in this PR. But R failures weren't fixed because r/configure uses pkg-config only for arrow.pc. Requires.private: thrift in parquet.pc wasn't used. So I also changed r/configure to use pkg-config for parquet.pc/arrow-dataset.pc/arrow-acero.pc/arrow-substrait.pc too.

kou · 2023-09-08T22:29:21Z

I'll merge this in the next week if nobody objects this.

assignUser · 2023-09-08T22:33:20Z

@github-actions crossbow submit -g r

github-actions · 2023-09-08T22:37:14Z

Revision: b349d80

Submitted crossbow builds: ursacomputing/crossbow @ actions-1ca961daae

Task	Status
conda-linux-aarch64-cpu-r41
conda-linux-aarch64-cpu-r42
conda-linux-x64-cpu-r41
conda-linux-x64-cpu-r42
conda-osx-arm64-cpu-r41
conda-osx-arm64-cpu-r42
conda-osx-x64-cpu-r41
conda-osx-x64-cpu-r42
conda-win-x64-cpu-r41
homebrew-r-autobrew
homebrew-r-brew
r-binary-packages
test-fedora-r-clang-sanitizer
test-r-arrow-backwards-compatibility
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-library-r-base-latest
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-gcc-release-latest
test-r-rstudio-r-base-4.1-opensuse153
test-r-rstudio-r-base-4.2-centos7-devtoolset-8
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-r-sanitizer

assignUser

The r failures are unrelated e.g. due to #37639

kou · 2023-09-11T00:15:07Z

Thanks.
I merge this.

conbench-apache-arrow · 2023-09-11T03:35:37Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit c0c56e8.

There were 5 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-09-11 01:00:17Z
- file-read (R) with compression=lz4, dataset=fanniemae_2016Q4, file_type=feather, language=R, output_type=table
- dataframe-to-table (R) with dataset=type_floats, language=R
and 3 more (see the report linked below)

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

…quet.pc (apache#37603) ### Rationale for this change apacheGH-37399 removed "thrift" pkg-config package dependency from "parquet" pkg-config package accidentally. Before apacheGH-37399, "arrow" pkg-config packages has "thrift" pkg-config package dependency as `Requires.private`. ### What changes are included in this PR? Add "thrift" to `Requires.private` of `parquet.pc`. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * Closes: apache#37535 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

kou requested a review from wgtmac as a code owner September 7, 2023 08:02

github-actions bot added Component: Parquet Component: C++ awaiting committer review Awaiting committer review labels Sep 7, 2023

wgtmac reviewed Sep 7, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Sep 8, 2023

wgtmac approved these changes Sep 8, 2023

View reviewed changes

kou requested a review from assignUser as a code owner September 8, 2023 02:15

github-actions bot added Component: R awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 8, 2023

kou added 2 commits September 8, 2023 11:16

apacheGH-37535: [C++][Parquet] Add missing "thrift" dependency in par…

0d496cd

…quet.pc

Use pkg-config for parquet/arrow-dataset/arrow-acero/arrow-substrait too

b349d80

kou force-pushed the cpp-parquet-pc-thrift branch from bf110e9 to b349d80 Compare September 8, 2023 02:16

kou commented Sep 8, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 8, 2023

assignUser approved these changes Sep 9, 2023

View reviewed changes

kou merged commit c0c56e8 into apache:main Sep 11, 2023
32 checks passed

kou deleted the cpp-parquet-pc-thrift branch September 11, 2023 00:15

kou removed the awaiting changes Awaiting changes label Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-37535: [C++][Parquet] Add missing "thrift" dependency in parquet.pc #37603

GH-37535: [C++][Parquet] Add missing "thrift" dependency in parquet.pc #37603

kou commented Sep 7, 2023 •

edited by github-actions bot

Loading

kou commented Sep 7, 2023

github-actions bot commented Sep 7, 2023

github-actions bot commented Sep 7, 2023

kou commented Sep 7, 2023

github-actions bot commented Sep 7, 2023

mapleFU commented Sep 7, 2023

wgtmac commented Sep 7, 2023

wgtmac Sep 7, 2023

kou Sep 8, 2023

kou commented Sep 8, 2023

kou commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

kou Sep 8, 2023

kou commented Sep 8, 2023 •

edited

Loading

assignUser commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

assignUser left a comment

kou commented Sep 11, 2023

conbench-apache-arrow bot commented Sep 11, 2023

GH-37535: [C++][Parquet] Add missing "thrift" dependency in parquet.pc #37603

GH-37535: [C++][Parquet] Add missing "thrift" dependency in parquet.pc #37603

Conversation

kou commented Sep 7, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

kou commented Sep 7, 2023

github-actions bot commented Sep 7, 2023

github-actions bot commented Sep 7, 2023

kou commented Sep 7, 2023

github-actions bot commented Sep 7, 2023

mapleFU commented Sep 7, 2023

wgtmac commented Sep 7, 2023

wgtmac Sep 7, 2023

Choose a reason for hiding this comment

kou Sep 8, 2023

Choose a reason for hiding this comment

kou commented Sep 8, 2023

kou commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

kou Sep 8, 2023

Choose a reason for hiding this comment

kou commented Sep 8, 2023 • edited Loading

assignUser commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

assignUser left a comment

Choose a reason for hiding this comment

kou commented Sep 11, 2023

conbench-apache-arrow bot commented Sep 11, 2023

kou commented Sep 7, 2023 •

edited by github-actions bot

Loading

kou commented Sep 8, 2023 •

edited

Loading