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

Revert "Revert "bazel: update DEPS on googleurl (#17794)" (#17958)" #18192

Conversation

buildbreaker
Copy link

@buildbreaker buildbreaker commented Sep 21, 2021

This reverts commit 338a42c.

The original PR which this is intending to replicate is: #17794

The original failure which prompted the revert is:

In file included from external/com_googlesource_googleurl/base/strings/string_util.cc:5:
In file included from external/com_googlesource_googleurl/base/strings/string_util.h:22:
In file included from external/com_googlesource_googleurl/base/containers/span.h:18:
external/com_googlesource_googleurl/base/containers/checked_iterators.h:241:8: error: explicit specialization of undeclared template struct '__is_cpp17_contiguous_iterator'
struct __is_cpp17_contiguous_iterator<::gurl_base::CheckedContiguousIterator<T>>
       ^                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

Envoy Slack thread context: https://envoyproxy.slack.com/archives/CKQ2LK23G/p1630533652001900

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Alan Chiu achiu@lyft.com

@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #18192 was opened by buildbreaker.

see: more, trace.

@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #18192 was opened by buildbreaker.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Sep 21, 2021
Comment on lines 112 to 115
-template <typename T>
-struct __is_cpp17_contiguous_iterator<::gurl_base::CheckedContiguousIterator<T>>
- : true_type {};
-
Copy link
Author

Choose a reason for hiding this comment

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

Deleted. Only difference between #17958

Copy link
Member

Choose a reason for hiding this comment

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

Any more info on this deletion? What impact does it have / are we tracking fixing this upstream somehow?

Copy link
Member

Choose a reason for hiding this comment

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

It fixes it since the android error is trying to specialize something that doesn't exist. I'm surprise this builds at all though, maybe it's just a performance optimization?

Signed-off-by: Alan Chiu <achiu@lyft.com>
@buildbreaker buildbreaker force-pushed the revert-revert-bazel-update-deps-on-googleurl-17794-17958 branch from 3eb179e to 335210b Compare September 21, 2021 02:42
@wrowe
Copy link
Contributor

wrowe commented Sep 21, 2021

I'm sorry, but I can't understand the purpose of this request unless @mattklein123 's patch is somehow wrong. Duplicate PR asks do not move issues forward but do lead to good contributors being ignored. Could you please explain?

@mattklein123
Copy link
Member

I'm sorry, but I can't understand the purpose of this request unless @mattklein123 's patch is somehow wrong. Duplicate PR asks do not move issues forward but do lead to good contributors being ignored. Could you please explain?

We can't merge the other PR because it's broken on Android. This is an attempt to fix the Android issue.

@mattklein123 mattklein123 self-assigned this Sep 21, 2021
@buildbreaker
Copy link
Author

@wrowe @mattklein123 Correct. Envoy Mobile is looking to update the googleurl to avoid an iOS build issue with an xcode update. The main issue with the update is that the method __is_cpp17_contiguous_iterator is not available in some NDK versions which Envoy Mobile builds against. The main reason for the deletion (might not be the right solution) is to ensure this builds with Envoy (I've tested this locally with Envoy Mobile).

Sorry if I opened this PR prematurely. I was hoping to collaborate more closely with @keith on this issue. He suggested that maybe that method is not necessarily used in Envoy (it builds in Envoy Mobile fwiw).

@keith
Copy link
Member

keith commented Sep 21, 2021

Does this change work on Android? Kinda surprised deleting this would work at all, since I assume they had to do this for a reason 🤔 I think the alternative here is you define a no-op struct, maybe only on android, for this.

@buildbreaker
Copy link
Author

I've tested this and this does build with Android in Envoy Mobile. I was hoping to add some #ifdefs instead but it seems like the args aren't passed all the way through for some reason. That's why I went with a straight deletion to verify with Envoy Mobile. I put up this PR as a check to see if Envoy CI would pass

@dio
Copy link
Member

dio commented Sep 22, 2021

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18192 (comment) was created by @dio.

see: more, trace.

Alan Chiu added 2 commits September 23, 2021 16:19
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
@buildbreaker buildbreaker deleted the revert-revert-bazel-update-deps-on-googleurl-17794-17958 branch September 24, 2021 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants