-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Update com_google_absl: 20211102.0 (current: 17c954d) #19307
Conversation
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
/retest |
Retrying Azure Pipelines: |
seems like the failures are real on this one - the error seems to be ERROR: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/external/com_github_google_tcmalloc/tcmalloc/BUILD:290:11: in cc_library rule @com_github_google_tcmalloc//tcmalloc:***@com_google_absl//absl/debugging:debugging_internal' is not visible from target '@com_github_google_tcmalloc//tcmalloc:common'. Check the visibility declaration of the former target if you think the dependency is legitimate related upstream issue seems to be google/tcmalloc#103 which is resolved, so not sure if we need to update something else to fix, or not yet released |
current tcmalloc version is 9f385356c34d4fc11f76a000b609e2b446c20667 so might be worth looking at that |
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
i think you probs need to rebase to main - it sometimes does this with merging |
@@ -105,12 +105,12 @@ REPOSITORY_LOCATIONS_SPEC = dict( | |||
project_name = "Abseil", | |||
project_desc = "Open source collection of C++ libraries drawn from the most fundamental pieces of Google’s internal codebase", | |||
project_url = "https://abseil.io/", | |||
version = "17c954d90d5661e27db8fc5f086085690a8372d9", | |||
sha256 = "2e4ace2ed32a4ccfd29e856ad72b4fd1eae2ec060d3ba8646857fa170d6e8269", | |||
version = "20211102.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change includes serious regressions against msvc, which is required for consideration of adoption. You can review the azure build log for details. Not only should it be -std:c99 on windows vs -std=c99, but there are significant clib deviations that absl made contrary assumptions about. In our experience MS has a slightly different impl where the spec was left ambiguous. Does not lgtm, deps change cannot be accepted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wrowe any ideas on ways forward on this, grepping etc im not seeing where this flag is set, but i guess there are further issues anyway
im assuming that we do want to update, at least at some point, maybe that is incorrect
looking at the error log, it seems to be something to do with quic, not sure if that is worth looking at
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breakage noted above needs to be addressed upstream, or faulty envoy assumptions corrected within this PR.
Fixes envoyproxy#18972 Signed-off-by: Faseela K <[email protected]>
Fixes envoyproxy#18972 Signed-off-by: Faseela K <[email protected]>
957441c
to
ef78070
Compare
/retest to see if #19323 helps |
Retrying Azure Pipelines: |
Fixes #18972
Signed-off-by: Faseela K [email protected]