ARROW-17795: [C++][R] Add missing PKG_CONFIG_PATH to use system zstd #14202

kou · 2022-09-22T06:08:15Z

No description provided.

kou · 2022-09-22T06:08:38Z

@github-actions crossbow submit r-binary-packages

github-actions · 2022-09-22T06:28:03Z

https://issues.apache.org/jira/browse/ARROW-17795

github-actions · 2022-09-22T06:28:05Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

kou · 2022-09-22T19:51:38Z

@github-actions crossbow submit r-binary-packages

kou · 2022-09-23T00:20:23Z

@github-actions crossbow submit r-binary-packages

kou · 2022-09-23T02:18:33Z

@github-actions crossbow submit r-binary-packages

kou · 2022-09-23T02:47:56Z

@github-actions crossbow submit r-binary-packages

kou · 2022-09-23T03:14:57Z

@github-actions crossbow submit r-binary-packages

… link

kou · 2022-09-23T04:30:06Z

@github-actions crossbow submit r-binary-packages

kou · 2022-09-23T07:08:40Z

@github-actions crossbow submit -g r

github-actions · 2022-09-23T07:56:25Z

Revision: c04e0da

Submitted crossbow builds: ursacomputing/crossbow @ actions-4116018602

Task	Status
conda-linux-gcc-py37-cpu-r40
conda-linux-gcc-py37-cpu-r41
conda-osx-clang-py37-r40
conda-osx-clang-py37-r41
conda-win-vs2019-py37-r40
conda-win-vs2019-py37-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-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-rocker-r-base-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-18.04-r-sanitizer

kou · 2022-09-23T19:43:35Z

@github-actions crossbow submit test-r-arrow-backwards-compatibility test-r-install-local test-r-offline-maximal

github-actions · 2022-09-23T19:45:04Z

Revision: 3cef9bf

Submitted crossbow builds: ursacomputing/crossbow @ actions-48ab0ff1eb

Task	Status
test-r-arrow-backwards-compatibility
test-r-install-local
test-r-offline-maximal

kou · 2022-09-23T21:27:51Z

@assignUser You may want to review this.

assignUser

I am still not sure about the changes to FinzstdAlt.cmake (it worked before and now needs special casing -> a regression in my eyes) but as I don't quite understand them and your fix passes ci... 🤷

Should we add a note in the documentation about this?

assignUser · 2022-09-26T09:39:36Z

r/configure

+          export PKG_CONFIG_PATH=${LIB_DIR}/pkgconfig${PKG_CONFIG_PATH:+:${PKG_CONFIG_PATH}}
+          PKG_LIBS="`pkg-config --libs --static --silence-errors ${PKG_CONFIG_NAME}`"


These changes are not required for configure.win because we use the mysys stuff anyway right?

I didn't see configure.win but it seems that configure.win doesn't use pkg-config on release build. configure.win uses pkg-config on dev build but it already uses pkg-config --libs ....

assignUser · 2022-09-26T09:40:00Z

dev/tasks/r/github.packages.yml

@@ -324,6 +329,7 @@ jobs:
        env:
          LIBARROW_BINARY: "FALSE"
          ARROW_R_DEV: "TRUE"
+          CMAKE_FIND_DEBUG_MODE: "ON"


Should we remove this before merge?

I want to keep this to debug a problem in the future.
This step is executed only when the above build step is failed. Is it accepted?

👍 makes sense!

kou

I am still not sure about the changes to FinzstdAlt.cmake (it worked before and now needs special casing -> a regression in my eyes) but as I don't quite understand them and your fix passes ci... 🤷

Sorry. I explain it:

ARROW-17728 / ARROW-17728: [C++][Gandiva] Accept LLVM 15.0 #14125 adds support for system zstdConfig.cmake (the CMake package for Zstandard) to find system Zstandard
GitHub Actions' Ubuntu hosted runner provides Zstandard installed by Homebrew by default
GitHub Actions' Ubuntu hosted runner has $(brew --prefix)/bin in $PATH by default
CMake's find_package can find $(brew --prefix)/lib/cmake/zstd/zstdConfig.cmake because $(brew --prefix)/bin exists in $PATH
See also: https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure

"5. Search the standard system environment variables. This can be skipped if NO_SYSTEM_ENVIRONMENT_PATH is passed or by setting the CMAKE_FIND_USE_SYSTEM_ENVIRONMENT_PATH to FALSE. Path entries ending in /bin or /sbin are automatically converted to their parent directories"
We can use pkg-config --static ... to get build flags suitable for static linking
pkg-config uses Requires.private and Libs.private in .pc file only when pkg-config --static ... is used
arrow.pc is generated by our CMake config and our CMake config tries setting Requires.private and Libs.private for static linking
Our CMake config adds pkg-config package name to Requires.private for system libraries ( such as Zstandard (installed by Homebrew by default) in this case) only if our CMake config can find .pc for the system libraries
But our CMake config couldn't add pkg-config package name for the system Zstandard (installed by Homebrew by default) because our CMake config couldn't find zstd.pc for the system Zstandard
Our CMake config can find zstd.pc for the system Zstandard by setting PKG_CONFIG_PATH=$(brew --prefix)/lib/pkgconfig
- GitHub Actions' Ubuntu hosted runner adds $(brew --prefix)/bin to $PATH but doesn't set PKG_CONFIG_PATH=$(brew --prefix)/lib/pkgconfig
- I think that this is a GitHub Actions' Ubuntu hosted runner's problem not our CMake config's problem. So I set it manually in this pull request.

Does the explanation help you?

Should we add a note in the documentation about this?

Hmm. Do you have any idea which documentation page is suitable?
I think that this is a GitHub Actions' Ubuntu hosted runner's specific problem not a general problem. Generally, users will set all related environment variables (PATH, PKG_CONFIG_PATH, LD_LIBRARY_PATH and so on) when they add a new package prefix such as $(brew --prefix), /usr/local and so on.

kou · 2022-09-26T21:23:11Z

dev/tasks/r/github.packages.yml

@@ -324,6 +329,7 @@ jobs:
        env:
          LIBARROW_BINARY: "FALSE"
          ARROW_R_DEV: "TRUE"
+          CMAKE_FIND_DEBUG_MODE: "ON"


I want to keep this to debug a problem in the future.
This step is executed only when the above build step is failed. Is it accepted?

kou · 2022-09-26T21:25:25Z

r/configure

+          export PKG_CONFIG_PATH=${LIB_DIR}/pkgconfig${PKG_CONFIG_PATH:+:${PKG_CONFIG_PATH}}
+          PKG_LIBS="`pkg-config --libs --static --silence-errors ${PKG_CONFIG_NAME}`"


I didn't see configure.win but it seems that configure.win doesn't use pkg-config on release build. configure.win uses pkg-config on dev build but it already uses pkg-config --libs ....

assignUser

@kou Thanks for the detailed explanation, it helped me a lot! So this issue did not happen before because this is new functionality.

I think that this is a GitHub Actions' Ubuntu hosted runner's specific problem not a general problem.

Now that I understand the issue I agree.

kou · 2022-09-27T20:34:40Z

OK. I merge this.

ursabot · 2022-09-28T07:53:14Z

Benchmark runs are scheduled for baseline = b75e9e9 and contender = 83ad54c. 83ad54c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.38% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.11% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 83ad54c6 ec2-t3-xlarge-us-east-2
[Finished] 83ad54c6 test-mac-arm
[Failed] 83ad54c6 ursa-i9-9960x
[Finished] 83ad54c6 ursa-thinkcentre-m75q
[Finished] b75e9e9b ec2-t3-xlarge-us-east-2
[Finished] b75e9e9b test-mac-arm
[Failed] b75e9e9b ursa-i9-9960x
[Finished] b75e9e9b ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

…pache#14202) Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

github-actions bot added Component: C++ Component: R labels Sep 22, 2022

This comment was marked as outdated.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as outdated.

Sign in to view

kou added 7 commits September 23, 2022 13:27

ARROW-17795: WIP: [C++][R] Fix a bug that zstd isn't found for static…

bc9541a

… link

Set PKG_CONFIG_PATH for brew

b4256f6

Use bundled zstd

e3cbf41

Debug

7adcb5b

Debug

2d78246

Don't ignore existing PKG_CONFIG_PATH

2a596cb

Use --libs

5653d0b

kou force-pushed the r-zstd branch from 9dcf43b to ab7a88e Compare September 23, 2022 04:29

kou changed the title ~~ARROW-17795: WIP: [C++][R] Fix a bug that zstd isn't found for static link~~ ARROW-17795: [C++][R] Fix a bug that zstd isn't found for static link Sep 23, 2022

kou changed the title ~~ARROW-17795: [C++][R] Fix a bug that zstd isn't found for static link~~ ARROW-17795: [C++][R] Add missing PKG_CONFIG_PATH to use system zstd Sep 23, 2022

Clean

6c4c384

kou force-pushed the r-zstd branch from ab7a88e to 6c4c384 Compare September 23, 2022 04:30

This comment was marked as outdated.

Sign in to view

Remove debug print

c04e0da

Set PKG_CONFIG_PATH

3cef9bf

assignUser reviewed Sep 26, 2022

View reviewed changes

kou commented Sep 26, 2022

View reviewed changes

assignUser approved these changes Sep 27, 2022

View reviewed changes

kou merged commit 83ad54c into apache:master Sep 27, 2022

kou deleted the r-zstd branch September 27, 2022 20:34

fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022

ARROW-17795: [C++][R] Add missing PKG_CONFIG_PATH to use system zstd (a…

0c3b48e

…pache#14202) Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-17795: [C++][R] Add missing PKG_CONFIG_PATH to use system zstd #14202

ARROW-17795: [C++][R] Add missing PKG_CONFIG_PATH to use system zstd #14202

kou commented Sep 22, 2022

kou commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

This comment was marked as outdated.

kou commented Sep 22, 2022

This comment was marked as outdated.

kou commented Sep 23, 2022

This comment was marked as outdated.

kou commented Sep 23, 2022

This comment was marked as off-topic.

kou commented Sep 23, 2022

This comment was marked as outdated.

kou commented Sep 23, 2022

This comment was marked as outdated.

kou commented Sep 23, 2022

This comment was marked as outdated.

kou commented Sep 23, 2022

github-actions bot commented Sep 23, 2022

kou commented Sep 23, 2022

github-actions bot commented Sep 23, 2022

kou commented Sep 23, 2022

assignUser left a comment

assignUser Sep 26, 2022

kou Sep 26, 2022

assignUser Sep 26, 2022

kou Sep 26, 2022

assignUser Sep 27, 2022

kou left a comment

kou Sep 26, 2022

kou Sep 26, 2022

assignUser left a comment

kou commented Sep 27, 2022

ursabot commented Sep 28, 2022

		export PKG_CONFIG_PATH=${LIB_DIR}/pkgconfig${PKG_CONFIG_PATH:+:${PKG_CONFIG_PATH}}
		PKG_LIBS="`pkg-config --libs --static --silence-errors ${PKG_CONFIG_NAME}`"

ARROW-17795: [C++][R] Add missing PKG_CONFIG_PATH to use system zstd #14202

ARROW-17795: [C++][R] Add missing PKG_CONFIG_PATH to use system zstd #14202

Conversation

kou commented Sep 22, 2022

kou commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

This comment was marked as outdated.

kou commented Sep 22, 2022

This comment was marked as outdated.

kou commented Sep 23, 2022

This comment was marked as outdated.

kou commented Sep 23, 2022

This comment was marked as off-topic.

kou commented Sep 23, 2022

This comment was marked as outdated.

kou commented Sep 23, 2022

This comment was marked as outdated.

kou commented Sep 23, 2022

This comment was marked as outdated.

kou commented Sep 23, 2022

github-actions bot commented Sep 23, 2022

kou commented Sep 23, 2022

github-actions bot commented Sep 23, 2022

kou commented Sep 23, 2022

assignUser left a comment

Choose a reason for hiding this comment

assignUser Sep 26, 2022

Choose a reason for hiding this comment

kou Sep 26, 2022

Choose a reason for hiding this comment

assignUser Sep 26, 2022

Choose a reason for hiding this comment

kou Sep 26, 2022

Choose a reason for hiding this comment

assignUser Sep 27, 2022

Choose a reason for hiding this comment

kou left a comment

Choose a reason for hiding this comment

kou Sep 26, 2022

Choose a reason for hiding this comment

kou Sep 26, 2022

Choose a reason for hiding this comment

assignUser left a comment

Choose a reason for hiding this comment

kou commented Sep 27, 2022

ursabot commented Sep 28, 2022