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.

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

GH-43228: [C++] Fix Abseil compile error on GCC 13 #43157

Merged
merged 21 commits into from
Jul 26, 2024

Conversation

amol-
Copy link
Member

@amol- amol- commented Jul 5, 2024

Rationale for this change

When trying to compile Arrow with GCC 13, it fails due to ABSEIL missing a <cstdint> include, this PR addresses the issue by adding the missing include.

There have been past reports for this issue too: #36969

This is a more minimal fix that tries to avoid the complexity of previous attempts like #43147 and #37066 which involved updating Abseil and facing additional issues to fix.

What changes are included in this PR?

Add the missing include when GCC>=13

Are these changes tested?

They are tested by the existing compile infrastructure and testsuite and by adding a new GCC-13 based CPP test environment for bundled builds.

Are there any user-facing changes?

No, all behaviours should remain the same

@amol-
Copy link
Member Author

amol- commented Jul 5, 2024

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

Copy link

github-actions bot commented Jul 5, 2024

Revision: 8f503ae

Submitted crossbow builds: ursacomputing/crossbow @ actions-defadac854

Task Status
r-binary-packages 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-cuda-cpp GitHub Actions
test-debian-12-cpp-amd64 GitHub Actions
test-debian-12-cpp-i386 GitHub Actions
test-fedora-39-cpp 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-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-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-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions
test-ubuntu-r-sanitizer GitHub Actions

@amol- amol- marked this pull request as ready for review July 5, 2024 18:34
@amol-
Copy link
Member Author

amol- commented Jul 5, 2024

@raulcd @assignUser this should be ready for review, I tried a more conservative approach as upgrading Abseil introduced more problems than it fixed even on the most conservative version upgrade.

@kou kou changed the title GH-36969: [C++] Fix ABSEIL compile error on GCC 13 GH-36969: [C++] Fix Abseil compile error on GCC 13 Jul 5, 2024
@kou
Copy link
Member

kou commented Jul 5, 2024

Could you open a new issue for this instead of reusing existing closed issue?

@amol-
Copy link
Member Author

amol- commented Jul 11, 2024

Could you open a new issue for this instead of reusing existing closed issue?

Given it's the same exact error message shouldn't we preserve a consolidated history by keeping the original issue?
Happy to open a new issue if that's the preference

@raulcd
Copy link
Member

raulcd commented Jul 11, 2024

The problem is that the original issue is in the 13.0.0 milestone and all our current release scripts take that into account to generate release notes, etcetera. If we reuse the same issue it would be as if this PR was released on 13.0.0 and if we require to back port this issue it gets messy.
It has it's pros and cons but the current process doesn't account for two PRs pointing to the same GH issue.

@amol-
Copy link
Member Author

amol- commented Jul 11, 2024

The problem is that the original issue is in the 13.0.0 milestone

Makes sense, I didn't check and thought it was closed without being done. I'll open a new one.

@amol- amol- changed the title GH-36969: [C++] Fix Abseil compile error on GCC 13 GH-43228: [C++] Fix Abseil compile error on GCC 13 Jul 11, 2024
Copy link

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

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

Can we reproduce this by the following change?

diff --git a/dev/tasks/tasks.yml b/dev/tasks/tasks.yml
index 45417acf85..cd5f119b77 100644
--- a/dev/tasks/tasks.yml
+++ b/dev/tasks/tasks.yml
@@ -1418,6 +1418,16 @@ tasks:
         GCC_VERSION: 12
       image: ubuntu-r-only-r
 
+  # This also has -flto=auto
+  test-r-gcc-13:
+    ci: github
+    template: docker-tests/github.linux.yml
+    params:
+      env:
+        UBUNTU: 24.04
+        GCC_VERSION: 13
+      image: ubuntu-r-only-r
+
   test-r-minimal-build:
     ci: azure
     template: r/azure.linux.yml

cpp/cmake_modules/ThirdpartyToolchain.cmake Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting review Awaiting review awaiting changes Awaiting changes awaiting committer review Awaiting committer review and removed awaiting review Awaiting review awaiting changes Awaiting changes awaiting committer review Awaiting committer review labels Jul 11, 2024
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 23, 2024
@amol-
Copy link
Member Author

amol- commented Jul 25, 2024

@github-actions crossbow submit test-ubuntu-24.04-cpp-gcc-13-bundled

@apache apache deleted a comment from github-actions bot Jul 25, 2024
Copy link

Revision: d59f39b

Submitted crossbow builds: ursacomputing/crossbow @ actions-a91e2018b6

Task Status
test-ubuntu-24.04-cpp-gcc-13-bundled GitHub Actions

@amol-
Copy link
Member Author

amol- commented Jul 25, 2024

@kou test-ubuntu-24.04-cpp-gcc-13-bundled now compiles correctly. It fails the test, but it seems unrelated as they are timezone related tests. I'll kick-off a check on ubuntu-20.04 to confirm the test failures are unrelated.

I think that this is ready for final review.

@amol-
Copy link
Member Author

amol- commented Jul 25, 2024

@github-actions crossbow submit test-ubuntu-20.04-cpp-bundled

Copy link

Revision: 70e6e7d

Submitted crossbow builds: ursacomputing/crossbow @ actions-3d162f80dd

Task Status
test-ubuntu-20.04-cpp-bundled GitHub Actions

@amol-
Copy link
Member Author

amol- commented Jul 25, 2024

@github-actions crossbow submit test-ubuntu-24.04-cpp-gcc-13-bundled

Copy link

Revision: 943baa0

Submitted crossbow builds: ursacomputing/crossbow @ actions-ecde5a2c6e

Task Status
test-ubuntu-24.04-cpp-gcc-13-bundled GitHub Actions

@amol-
Copy link
Member Author

amol- commented Jul 25, 2024

The timestamp tests now pass too by installing tzdata-legacy

@amol- amol- requested a review from kou July 25, 2024 17:24
Copy link
Member

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Could you also remove the workaround we added to r/configure?

@apache apache deleted a comment from github-actions bot Jul 26, 2024
@amol-
Copy link
Member Author

amol- commented Jul 26, 2024

@github-actions crossbow submit test-r-depsource-bundled

Copy link

Revision: 0a6e367

Submitted crossbow builds: ursacomputing/crossbow @ actions-c9cac2c09c

Task Status
test-r-depsource-bundled Azure

@amol-
Copy link
Member Author

amol- commented Jul 26, 2024

Looks good, thanks! Could you also remove the workaround we added to r/configure?

Done and tested on test-r-depsource-bundled

@assignUser
Copy link
Member

@github-actions crossbow submit r-binary-packages
Nice, thanks. I'll run this as it has a build on 24.04 which comes with the new gcc. I'll merge after

Copy link

Revision: 0a6e367

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

Task Status
r-binary-packages GitHub Actions

@assignUser
Copy link
Member

Ugh, turns out ubuntu-latest is still 22.04, I modified the crossbow workflow to re-run on 24.04

@assignUser assignUser merged commit aaeff72 into apache:main Jul 26, 2024
49 of 52 checks passed
@assignUser assignUser removed the awaiting change review Awaiting change review label Jul 26, 2024
Copy link

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

There were no benchmark performance regressions. 🎉

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

jonkeane pushed a commit that referenced this pull request Jul 31, 2024
### Rationale for this change

When trying to compile Arrow with GCC 13, it fails due to ABSEIL missing a `<cstdint>` include, this PR addresses the issue by adding the missing include.

There have been past reports for this issue too: #36969 

This is a more minimal fix that tries to avoid the complexity of previous attempts like  #43147 and #37066 which involved updating Abseil and facing additional issues to fix.

### What changes are included in this PR?

Add the missing include when GCC>=13

### Are these changes tested?

They are tested by the existing compile infrastructure and testsuite and by adding a new GCC-13 based CPP test environment for bundled builds.

### Are there any user-facing changes?

No, all behaviours should remain the same
* GitHub Issue: #43228

Lead-authored-by: Alessandro Molina <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants