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

Bump abseil-cpp version to 20220623.1 #18184

Closed
wants to merge 6 commits into from

Conversation

davido
Copy link
Contributor

@davido davido commented Apr 23, 2023

Closes #18167.

abseil is missing stdint.h include, that broke recent compiler versions. This problem was fixed in this commit upstream:

abseil/abseil-cpp@36a4b07

Note, that we cannot update to the latst abseil-cpp version, because this breaking change: "Abseil now requires at least C++14" starting from LTS release 20230125 and Bazel is passing per default -std=c++0x option, see:

#18181

Closes bazelbuild#18167.

abseil is missing stdint.h include, that broke recent compiler versions.
This problem was fixed in this commit upstream:

abseil/abseil-cpp@36a4b07

Note, that we cannot update to the latst abseil-cpp version, because
this breaking change: "Abseil now requires at least C++14" starting
from LTS release 20230125 and Bazel is passing per default -std=c++0x
option, see:

bazelbuild#18181
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 23, 2023
This is needed to fix this breakage in grpc in Windows:

external/com_github_grpc_grpc/src/core/lib/gprpp/status_helper.cc(234):
error C2338: absl::Time needs to be able to be memcopied
@davido
Copy link
Contributor Author

davido commented Apr 23, 2023

@meteorcloudy, @aiuto, @Wyverald

Could you please review this?

Also, could you upload abseil-cpp tarball 20220623.1.tar.gz to Bazel mirror?

From here:
"https://github.com/abseil/abseil-cpp/archive/refs/tags/20220623.1.tar.gz"
to here:
"https://mirror.bazel.build/github.com/abseil/abseil-cpp/archive/refs/tags/20220623.1.tar.gz"

Thanks.

@sgowroji sgowroji added the team-Rules-CPP Issues for C++ rules label Apr 24, 2023
@sgowroji
Copy link
Member

Hi @davido, Can you raise the mirror artifact request in this template .Thanks!

@davido
Copy link
Contributor Author

davido commented Apr 24, 2023

@sgowroji Done in #18188.

distdir_deps.bzl Outdated Show resolved Hide resolved
@@ -246,22 +246,24 @@ DIST_DEPS = {
],
},
"com_google_absl": {
"archive": "20211102.0.tar.gz",
"sha256": "dcf71b9cba8dc0ca9940c4b316a0c796be8fab42b070bb6b7cab62b48f0e66c4",
"archive": "20220623.1.tar.gz",
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind also help submitting this version to https://github.com/bazelbuild/bazel-central-registry/tree/main/modules/abseil-cpp? So that we can also upgrade abseil to the same version with Bzlmod enabled.

@davido
Copy link
Contributor Author

davido commented Apr 25, 2023

@meteorcloudy

I've found a better (and less intrusive) approach: instead of patching vanilla abseil release 20220623.1 I bumped this line in gRpc (that is already patched in Bazel build anyway):

Subject: [PATCH] Bump MSVC version check regarding compile-time initialization
 to 1930

This is needed to be compatible with this change in abseil-cpp:
b8bbe92f84ffe1e249016cfe8b79efdffb7a35c1
---
 src/core/lib/gprpp/status_helper.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/core/lib/gprpp/status_helper.cc b/src/core/lib/gprpp/status_helper.cc
index f981df8601..f6e5a211db 100644
--- a/src/core/lib/gprpp/status_helper.cc
+++ b/src/core/lib/gprpp/status_helper.cc
@@ -224,7 +224,7 @@ absl::optional<std::string> StatusGetStr(const absl::Status& status,
 
 void StatusSetTime(absl::Status* status, StatusTimeProperty key,
                    absl::Time time) {
-#if !defined(__clang__) && defined(_MSC_VER) && _MSC_VER < 1910
+#if !defined(__clang__) && defined(_MSC_VER) && _MSC_VER < 1930
   // Abseil has a workaround for MSVC 2015 which prevents absl::Time
   // from being is_trivially_copyable but it's still safe to be
   // memcopied.

That way, the vanilla version of abseil-cpp 20220623.1 is used now, so that blzmod should just work, right?

PTAL.

@meteorcloudy
Copy link
Member

@davido Thanks! However, 20220623.1 abseil-cpp isn't available in the BCR yet, do you mind submitting this version so that we can also upgrade abseil-cpp in MODULE.bazel file?

@buildbreaker2021 buildbreaker2021 self-assigned this Apr 25, 2023
@davido
Copy link
Contributor Author

davido commented Apr 25, 2023

@meteorcloudy

However, 20220623.1 abseil-cpp isn't available in the BCR yet, do you mind submitting this version so that we can also upgrade abseil-cpp in MODULE.bazel file?

Sure, will upload the PR to BCR project in a moment.

Update: Done in bazelbuild/bazel-central-registry#589

@davido
Copy link
Contributor Author

davido commented Apr 25, 2023

@meteorcloudy

[...] so that we can also upgrade abseil-cpp in MODULE.bazel file?

Bazel depends transitvely on abseil-cpp through grpc. Root bazel's MODULE.bazel doesn't reference abseil-cpp directly.

Only grpc 1.47.0 module references it, in:

modules/grpc/1.47.0/MODULE.bazel

bazel_dep(name = "abseil-cpp", repo_name = "com_google_absl", version = "20211102.0")

I guess we would need to bump abseil in existing module, that way deviating from the upstream's dependent version?

Alternative approach would be to publish yet another grpc module version, say 1.47.1,
except that this is a real grpc release.

Any advice how to proceed in that case?

@meteorcloudy
Copy link
Member

I think we can just add an direct dependency on abseil-cpp in Bazel's MODULE.bazel file with a comment explaining why we need that.

@davido
Copy link
Contributor Author

davido commented Apr 25, 2023

I think we can just add an direct dependency on abseil-cpp in Bazel's MODULE.bazel file with a comment explaining why we need that.

But we would also need to patch grpc in addition to updating abseil-cpp, like it was done in this PR to be compatible with newer absei-cpp version: https://github.com/bazelbuild/bazel/pull/18184/commits/024347e35e83a604a9c0c136b6fb0aec28b40d26m right?

@meteorcloudy
Copy link
Member

meteorcloudy commented Apr 25, 2023

Yeah, you can use single_version_override to patch grpc with Bzlmod enabled, but in that case, it's better to use a separate patch file because we don't need other changes that's needed in WORKSPACE.

@meteorcloudy
Copy link
Member

BTW, are those patches still needed in newer version of grpc or abseil-cpp? Should we upstream the patch?

@davido
Copy link
Contributor Author

davido commented Apr 25, 2023

BTW, are those patches still needed in newer version of grpc or abseil-cpp? Should we upstream the patch?

I don't think though, because as I said in my previous comment, the gRpc code in question was re-written in this CL, that is included in official releases, starting from gRpc v1.48.0, but Bazel is currently using 1.47.0.

There are no other changes between those two releases, except abseil-cpp update:

  $ git log --oneline v1.47.0...v1.48.1 bazel/grpc_deps.bzl
dcf9612186 Upgrade Abseil to LTS 20220623.0  (#30155)
1be6e2c9eb Update bazel toolchain mirror (#29980)

So that a different option to avoid the patching altogether is to just bump grpc from 1.47.0 to 1.48.1.

@davido
Copy link
Contributor Author

davido commented Apr 25, 2023

@meteorcloudy

I am working on bumping grpc version to 1.48.1 in bazel. Will upload another PR in a moment.

@davido
Copy link
Contributor Author

davido commented Apr 27, 2023

Superseded by #18216.

@davido davido closed this Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't bootstrap with clang >= 15 and gcc >= 13
4 participants