Skip to content

GH-40751: [C++] Fix protobuf package name setting for builds with substrait#40753

Merged
kou merged 3 commits intoapache:mainfrom
zanmato1984:fix-cmake-protobuf
Mar 24, 2024
Merged

GH-40751: [C++] Fix protobuf package name setting for builds with substrait#40753
kou merged 3 commits intoapache:mainfrom
zanmato1984:fix-cmake-protobuf

Conversation

@zanmato1984
Copy link
Copy Markdown
Contributor

@zanmato1984 zanmato1984 commented Mar 22, 2024 •
edited by github-actions bot
Loading

Rationale for this change

The problem #40751 seems to be introduced by #40399. Though I'm not entirely sure about the purpose of that, it seems to be missing an OR ARROW_SUBSTRAIT in the if branch in 5baca0f#diff-5cdc95f4e1b618f2f3ef10d370ce05a1ac05d9d401aecff3ccbb3d76bd366b6aR1815

Because other than ARROW_ORC, ARROW_WITH_OPENTELEMETRY and ARROW_FLIGHT, ARROW_SUBSTRAIT also implies ARROW_WITH_PROTOBUF:

arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake

Lines 421 to 423 in 5baca0f

if(ARROW_SUBSTRAIT)
set(ARROW_WITH_PROTOBUF ON)
endif()

What changes are included in this PR?

Add the possible missing condition of ARROW_SUBSTRAIT for the questioning if branch.

Are these changes tested?

Manually tested.

Are there any user-facing changes?

None.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 22, 2024

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

@zanmato1984
Copy link
Copy Markdown
Contributor Author

zanmato1984 commented Mar 22, 2024

cc @tobim @kou

kou
kou approved these changes Mar 22, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

Good catch!

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Mar 22, 2024
@zanmato1984 @kou
Update ThirdpartyToolchain.cmake
03c29cf 
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
@vibhatha
Copy link
Copy Markdown
Contributor

vibhatha commented Mar 23, 2024

@kou shall we run the CIs for the related job?

@zanmato1984
Copy link
Copy Markdown
Contributor Author

zanmato1984 commented Mar 23, 2024

Thank you both for reviewing. Appreciate it.

Could anyone help with the CI failure? Seems unrelated to the change.

@llama90
Copy link
Copy Markdown
Contributor

llama90 commented Mar 23, 2024

It seems to be related to that issue.

R / rhub/debian-gcc-devel:latest (pull_request)

error message 
CMake Error at boost_ep-stamp/boost_ep-download-RELEASE.cmake:37 (message):
  Command failed: 1

   '/usr/bin/cmake' '-Dmake=' '-Dconfig=' '-P' '/tmp/Rtmp90Fqh8/file738ab7a567/boost_ep-prefix/src/boost_ep-stamp/boost_ep-download-RELEASE-impl.cmake'

  See also

    /tmp/Rtmp90Fqh8/file738ab7a567/boost_ep-prefix/src/boost_ep-stamp/boost_ep-download-*.log


-- stdout output is:
...skipping to end...
/boost_1_81_0.tar.gz'

C++ / AMD64 Windows 2019 C++17 AVX2 (pull_request)

error message 
FAILED: boost_ep-prefix/src/boost_ep-stamp/boost_ep-download D:/a/arrow/arrow/build/cpp/boost_ep-prefix/src/boost_ep-stamp/boost_ep-download 
C:\Windows\system32\cmd.exe /C "cd /D D:\a\arrow\arrow\build\cpp\boost_ep-prefix\src && "C:\Program Files\CMake\bin\cmake.exe" -P D:/a/arrow/arrow/build/cpp/boost_ep-prefix/src/boost_ep-stamp/boost_ep-download-DEBUG.cmake && "C:\Program Files\CMake\bin\cmake.exe" -E touch D:/a/arrow/arrow/build/cpp/boost_ep-prefix/src/boost_ep-stamp/boost_ep-download"
CMake Error at boost_ep-stamp/boost_ep-download-DEBUG.cmake:37 (message):
  Command failed: 1

   'C:/Program Files/CMake/bin/cmake.exe' '-Dmake=' '-Dconfig=' '-P' 'D:/a/arrow/arrow/build/cpp/boost_ep-prefix/src/boost_ep-stamp/boost_ep-download-DEBUG-impl.cmake'

  See also

    D:/a/arrow/arrow/build/cpp/boost_ep-prefix/src/boost_ep-stamp/boost_ep-download-*.log


-- stdout output is:
...skipping to end...
projects/boost/files/boost/1.81.0/boost_1_81_0.tar.gz'

@llama90
Copy link
Copy Markdown
Contributor

llama90 commented Mar 23, 2024

In my opinion, the rest of the errors do not seem to be caused by the changes in that code.

R / Check minimum supported Arrow C++ Version (13.0.0) (pull_request)

error message 
--2024-03-22 23:28:28--  https://apache.jfrog.io/artifactory/arrow/ubuntu/apache-arrow-apt-source-latest-jammy.deb
Resolving apache.jfrog.io (apache.jfrog.io)... 18.214.194.113, 18.232.172.199, 3.95.117.170
Connecting to apache.jfrog.io (apache.jfrog.io)|18.214.194.113|:443... connected.
HTTP request sent, awaiting response... 302 Moved Temporarily
Location: https://landing.jfrog.com/reactivate-server/apache [following]
--2024-03-22 23:28:28--  https://landing.jfrog.com/reactivate-server/apache
Resolving landing.jfrog.com (landing.jfrog.com)... 18.214.194.113, 18.232.172.199, 3.95.117.170
Connecting to landing.jfrog.com (landing.jfrog.com)|18.214.194.113|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: 11533 (11K) [text/html]
Saving to: ‘apache-arrow-apt-source-latest-jammy.deb’

     0K .......... .                                          100% 4.20M=0.003s

2024-03-22 23:28:28 (4.20 MB/s) - ‘apache-arrow-apt-source-latest-jammy.deb’ saved [11533/11533]


WARNING: apt does not have a stable CLI interface. Use with caution in scripts.

Reading package lists...
E: Invalid archive signature
E: Internal error, could not locate member control.tar{.zst,.lz4,.gz,.xz,.bz2,.lzma,}
E: Could not read meta data from /home/runner/work/arrow/arrow/apache-arrow-apt-source-latest-jammy.deb
E: The package lists or status file could not be parsed or opened.
Error: Process completed with exit code 100.

@zanmato1984
Copy link
Copy Markdown
Contributor Author

zanmato1984 commented Mar 23, 2024

In my opinion, the rest of the errors do not seem to be caused by the changes in that code.

R / Check minimum supported Arrow C++ Version (13.0.0) (pull_request)

error message

--2024-03-22 23:28:28--  https://apache.jfrog.io/artifactory/arrow/ubuntu/apache-arrow-apt-source-latest-jammy.deb
Resolving apache.jfrog.io (apache.jfrog.io)... 18.214.194.113, 18.232.172.199, 3.95.117.170
Connecting to apache.jfrog.io (apache.jfrog.io)|18.214.194.113|:443... connected.
HTTP request sent, awaiting response... 302 Moved Temporarily
Location: https://landing.jfrog.com/reactivate-server/apache [following]
--2024-03-22 23:28:28--  https://landing.jfrog.com/reactivate-server/apache
Resolving landing.jfrog.com (landing.jfrog.com)... 18.214.194.113, 18.232.172.199, 3.95.117.170
Connecting to landing.jfrog.com (landing.jfrog.com)|18.214.194.113|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: 11533 (11K) [text/html]
Saving to: ‘apache-arrow-apt-source-latest-jammy.deb’

     0K .......... .                                          100% 4.20M=0.003s

2024-03-22 23:28:28 (4.20 MB/s) - ‘apache-arrow-apt-source-latest-jammy.deb’ saved [11533/11533]


WARNING: apt does not have a stable CLI interface. Use with caution in scripts.

Reading package lists...
E: Invalid archive signature
E: Internal error, could not locate member control.tar{.zst,.lz4,.gz,.xz,.bz2,.lzma,}
E: Could not read meta data from /home/runner/work/arrow/arrow/apache-arrow-apt-source-latest-jammy.deb
E: The package lists or status file could not be parsed or opened.
Error: Process completed with exit code 100.

Seems to be the same as #40759 and #40744.

@kou
Copy link
Copy Markdown
Member

kou commented Mar 24, 2024 •
edited
Loading

These failures are unrelated to this change. I'll merge this.

@kou kou merged commit d5e9e13 into apache:main Mar 24, 2024
@kou kou removed the awaiting merge Awaiting merge label Mar 24, 2024
@kou kou mentioned this pull request Mar 24, 2024
Closed
@conbench-apache-arrow
Copy link
Copy Markdown

conbench-apache-arrow bot commented Mar 25, 2024

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

There were 2 benchmark results indicating a performance regression:

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kou kou kou approved these changes

+1 more reviewer

@vibhatha vibhatha vibhatha approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zanmato1984 @vibhatha @llama90 @kou