Skip to content
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

GH-43688: [C++] Prevent Snappy from disabling RTTI when bundled #43689

Closed
wants to merge 7 commits into from
Closed

GH-43688: [C++] Prevent Snappy from disabling RTTI when bundled #43689

wants to merge 7 commits into from

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Aug 14, 2024 •
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?

@github-actions GitHub Actions
Copy link

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

pitrou
pitrou commented Aug 14, 2024
View reviewed changes
cpp/cmake_modules/ThirdpartyToolchain.cmake
endif()
# See comments in snappy.diff.
find_program(PATCH patch REQUIRED)
set(SNAPPY_PATCH_COMMAND ${PATCH} -p1 -i ${CMAKE_CURRENT_LIST_DIR}/snappy.diff)
Copy link
Member Author

Choose a reason for hiding this comment

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

@kou Do we require patch to be present on all platforms, especially Windows? Or should I make this more specific?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, to answer myself: our emscripten builds don't have access to a patch executable...
https://github.com/ursacomputing/crossbow/actions/runs/10388720694/job/28764995095#step:7:826

Copy link
Member Author

Choose a reason for hiding this comment

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

And even some Linux builds such as test-r-rocker-r-ver-latest don't seem to have patch either. I'll try to make it optional.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, unfortunately the patch is required. We either have to make sure that patch is available on all images, or do things differently :-/

Copy link
Member

Choose a reason for hiding this comment

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

Yes. patch is required as you noticed.
I think that another approach is better. See also: #43688 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

OTOH, patch is useful and could be needed for other bundled builds.

Copy link
Member

Choose a reason for hiding this comment

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

I think that we should fix in upstream instead of using patch as much as possible :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with this, but as you know Snappy rarely takes outside contributions...

@pitrou
Copy link
Member Author

pitrou commented Aug 14, 2024

@github-actions crossbow submit -g cpp

@pitrou
Copy link
Member Author

pitrou commented Aug 14, 2024

@github-actions crossbow submit -g python

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 14, 2024
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@pitrou
Copy link
Member Author

pitrou commented Aug 14, 2024

@github-actions crossbow submit -g r

@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@pitrou
Copy link
Member Author

pitrou commented Aug 14, 2024

@github-actions crossbow submit emscripten

@pitrou
Copy link
Member Author

pitrou commented Aug 14, 2024

@github-actions crossbow submit test-r-rocker-r-ver-latest test-r-rstudio-r-base-4.2-focal test-r-versions r-binary-packages

@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@pitrou
Copy link
Member Author

pitrou commented Aug 14, 2024

@github-actions crossbow submit emscripten

@pitrou
Copy link
Member Author

pitrou commented Aug 14, 2024

@github-actions crossbow submit -g r

@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@github-actions GitHub Actions
Copy link

Revision: 58adcdb

Submitted crossbow builds: ursacomputing/crossbow @ actions-62889c8077

Task Status
r-binary-packages GitHub Actions
test-r-arrow-backwards-compatibility GitHub Actions
test-r-clang-sanitizer GitHub Actions
test-r-depsource-bundled Azure
test-r-depsource-system GitHub Actions
test-r-dev-duckdb GitHub Actions
test-r-devdocs GitHub Actions
test-r-gcc-11 GitHub Actions
test-r-gcc-12 GitHub Actions
test-r-install-local GitHub Actions
test-r-install-local-minsizerel GitHub Actions
test-r-linux-as-cran GitHub Actions
test-r-linux-rchk GitHub Actions
test-r-linux-valgrind GitHub Actions
test-r-minimal-build Azure
test-r-offline-maximal GitHub Actions
test-r-offline-minimal Azure
test-r-rhub-debian-gcc-devel-lto-latest Azure
test-r-rhub-debian-gcc-release-custom-ccache Azure
test-r-rhub-ubuntu-release-latest Azure
test-r-rocker-r-ver-latest Azure
test-r-rstudio-r-base-4.1-opensuse155 Azure
test-r-rstudio-r-base-4.2-focal Azure
test-r-ubuntu-22.04 GitHub Actions
test-r-versions GitHub Actions
test-ubuntu-r-sanitizer GitHub Actions

@pitrou
Copy link
Member Author

pitrou commented Aug 14, 2024

@github-actions crossbow submit emscripten

@github-actions GitHub Actions
Copy link

Revision: ae1e24b

Submitted crossbow builds: ursacomputing/crossbow @ actions-9f77ba6cb3

Task Status
test-conda-python-emscripten GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions

@pitrou
Copy link
Member Author

pitrou commented Aug 14, 2024

@github-actions crossbow submit r-binary-packages

@github-actions GitHub Actions
Copy link

Revision: e545bc3

Submitted crossbow builds: ursacomputing/crossbow @ actions-74e3a4cd44

Task Status
r-binary-packages GitHub Actions

@pitrou
Copy link
Member Author

pitrou commented Aug 14, 2024

@github-actions crossbow submit -g python -g cpp

@github-actions GitHub Actions
Copy link

Revision: 19228c5

Submitted crossbow builds: ursacomputing/crossbow @ actions-df81c467d5

Task Status
example-python-minimal-build-fedora-conda GitHub Actions
example-python-minimal-build-ubuntu-venv GitHub Actions
test-alpine-linux-cpp GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-valgrind GitHub Actions
test-conda-python-3.10 GitHub Actions
test-conda-python-3.10-cython2 GitHub Actions
test-conda-python-3.10-hdfs-2.9.2 GitHub Actions
test-conda-python-3.10-hdfs-3.2.1 GitHub Actions
test-conda-python-3.10-pandas-latest-numpy-1.26 GitHub Actions
test-conda-python-3.10-pandas-latest-numpy-latest GitHub Actions
test-conda-python-3.10-pandas-nightly-numpy-nightly GitHub Actions
test-conda-python-3.10-substrait GitHub Actions
test-conda-python-3.11 GitHub Actions
test-conda-python-3.11-dask-latest GitHub Actions
test-conda-python-3.11-dask-upstream_devel GitHub Actions
test-conda-python-3.11-hypothesis GitHub Actions
test-conda-python-3.11-pandas-upstream_devel-numpy-nightly GitHub Actions
test-conda-python-3.11-spark-master GitHub Actions
test-conda-python-3.12 GitHub Actions
test-conda-python-3.12-cpython-debug GitHub Actions
test-conda-python-3.8 GitHub Actions
test-conda-python-3.8-pandas-1.0-numpy-1.19 GitHub Actions
test-conda-python-3.9 GitHub Actions
test-conda-python-3.9-pandas-latest-numpy-latest GitHub Actions
test-conda-python-emscripten GitHub Actions
test-cuda-cpp GitHub Actions
test-cuda-python GitHub Actions
test-debian-12-cpp-amd64 GitHub Actions
test-debian-12-cpp-i386 GitHub Actions
test-debian-12-python-3-amd64 GitHub Actions
test-debian-12-python-3-i386 GitHub Actions
test-fedora-39-cpp GitHub Actions
test-fedora-39-python-3 GitHub Actions
test-ubuntu-20.04-cpp GitHub Actions
test-ubuntu-20.04-cpp-bundled GitHub Actions
test-ubuntu-20.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-20.04-cpp-thread-sanitizer GitHub Actions
test-ubuntu-20.04-python-3 GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-20 GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
test-ubuntu-22.04-python-3 GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-gcc-13-bundled GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions

@kou kou mentioned this pull request Aug 14, 2024
Closed
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Aug 14, 2024
@pitrou
Copy link
Member Author

pitrou commented Aug 16, 2024

Closing as outdated, see the alternative and merged PR #43706

@pitrou pitrou closed this Aug 16, 2024
@pitrou pitrou deleted the gh43688-snappy-remove-no-rtti branch August 16, 2024 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kou kou kou left review comments

@assignUser assignUser Awaiting requested review from assignUser assignUser will be requested when the pull request is marked ready for review assignUser is a code owner

@jonkeane jonkeane Awaiting requested review from jonkeane jonkeane will be requested when the pull request is marked ready for review jonkeane is a code owner

@raulcd raulcd Awaiting requested review from raulcd raulcd will be requested when the pull request is marked ready for review raulcd is a code owner

Assignees
No one assigned
Labels
awaiting changes Awaiting changes Component: C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pitrou @kou