Revert "Revert "bazel: update DEPS on googleurl (#17794)" (#17958)" by mattklein123 · Pull Request #18167 · envoyproxy/envoy

mattklein123 · 2021-09-17T22:41:47Z

This reverts commit 338a42c.

Risk Level: Medium
Testing: Existing tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

This reverts commit 338a42c. Signed-off-by: Matt Klein <mklein@lyft.com>

repokitteh-read-only · 2021-09-17T22:41:52Z

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: #18167 was opened by mattklein123.

see: more, trace.

mattklein123 · 2021-09-17T22:42:23Z

Reopening to:

Make sure it still passes upstream.
Verify it fixes the xcode 13 build issues. cc @keith
Figure out what the Android build issues are. cc @keith

mattklein123 · 2021-09-17T22:44:43Z

cc @RenjieTang @wrowe

mattklein123 · 2021-09-17T22:47:39Z

cc @dio

keith · 2021-09-17T22:48:18Z

Looks like you accidentally got the tar.gz in here

wrowe

Reapproving, understanding that more work is to be done here for Android build to succeed in the -mobile fork, but Android is simply broken, and iOS is broken without this change.

mattklein123 · 2021-09-17T22:50:15Z

Oops thanks, fixing.

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2021-09-17T22:51:29Z

Reapproving, understanding that more work is to be done here for Android build to succeed in the -mobile fork, but Android is simply broken, and iOS is broken without this change.

We need to debug Android before merging as there may be more upstream fixes required, so let's figure that out in parallel before we merge this.

wrowe

Nice catch @keith, reapproved.

keith · 2021-09-17T23:06:24Z

Confirmed this fixes the Xcode 13 iOS build

dio · 2021-09-17T23:07:06Z

bazel/repository_locations.bzl

-        # Static snapshot of https://quiche.googlesource.com/quiche/+archive/ef0d23689e240e6c8de4c3a5296b209128c87373.tar.gz.
-        version = "ef0d23689e240e6c8de4c3a5296b209128c87373",
-        sha256 = "d769283fed1319bca68bae8bdd47fbc3a7933999329eee850eff1f1ea61ce176",
+        # Static snapshot of https://quiche.googlesource.com/quiche/+archive/561705e0066ff11e6cb97b8092f1547835beeb92.tar.gz.


Super nit, s/quiche/googleurl in the URL.

Suggested change

# Static snapshot of https://quiche.googlesource.com/quiche/+archive/561705e0066ff11e6cb97b8092f1547835beeb92.tar.gz.

# Static snapshot of https://quiche.googlesource.com/googleurl/+archive/561705e0066ff11e6cb97b8092f1547835beeb92.tar.gz.

keith · 2021-09-17T23:26:37Z

Confirmed this still has the same android build issue as before

dio · 2021-09-17T23:46:43Z

bazel/repository_locations.bzl

-        # Static snapshot of https://quiche.googlesource.com/quiche/+archive/ef0d23689e240e6c8de4c3a5296b209128c87373.tar.gz.
-        version = "ef0d23689e240e6c8de4c3a5296b209128c87373",
-        sha256 = "d769283fed1319bca68bae8bdd47fbc3a7933999329eee850eff1f1ea61ce176",
+        # Static snapshot of https://quiche.googlesource.com/quiche/+archive/561705e0066ff11e6cb97b8092f1547835beeb92.tar.gz.


Bringing the discussion on #17794 (comment) here. re: moving this googleurl repo to GH.

@danzh2010 @RenjieTang @moderation I think we need to move to GitHub, but that needs Google's decision? Since it will be hosted along with https://github.com/google/quiche (or if it is still only all about envoy, judging from the name: "envoy-integration", probably we may host it on envoyproxy org)? cc. @htuch @yanavlasov.

I agree we should move this similar to what we did with quiche. I'm not sure what is involved in doing this.

I think @DavidSchinazi @bencebeky helped out here for QUICHE. So they might also have some idea here.

The is an internal Google approval process for third party libraries. After that, Google's GitHub admin creates the repo under https://github.com/google/. Reach out to me though an internal channel if you decide to go down this route. At #17794 (comment) @dio mentions hosting it under https://github.com/envoyproxy/, that might be a better fit for this purpose.

Ultimately Google owns this code so we will need someone on the Google side to drive this. Between hosting it within the Envoy org or within a Google org I have no preference, as long as there is a single canonical source. We can quickly make you a repo within the Envoy org if you like. @yanavlasov is this something your team can help drive?

wrowe · 2021-09-21T00:01:13Z

Reading the most recent activity it sounds like we need to know the canonical origin of this code before we can merge this patch, no?

/wait

mattklein123 · 2021-09-21T15:22:13Z

Reading the most recent activity it sounds like we need to know the canonical origin of this code before we can merge this patch, no?

That's not the blocker, just a nice to have. The blocker is that this patch does not work on Android. That's what we are trying to sort out.

mattklein123 · 2021-09-24T01:45:17Z

Closing in favor of #18249

keith · 2021-09-24T01:46:29Z

I was targeting this branch with that PR, but I can fold it in if you'd prefer

keith · 2021-09-24T01:46:36Z

Just to make reviewing that easier

mattklein123 · 2021-09-24T01:51:42Z

Per my other comment IMO let's merge it all together into something that theoretically will work on all platforms.

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

95441bb

This reverts commit 338a42c. Signed-off-by: Matt Klein <mklein@lyft.com>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Sep 17, 2021

wrowe previously approved these changes Sep 17, 2021

View reviewed changes

fix

799972c

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 dismissed wrowe’s stale review via 799972c September 17, 2021 22:50

wrowe approved these changes Sep 17, 2021

View reviewed changes

dio reviewed Sep 17, 2021

View reviewed changes

alyssawilk assigned wrowe Sep 20, 2021

mattklein123 added the no stalebot Disables stalebot from closing an issue label Sep 20, 2021

repokitteh-read-only bot added the waiting label Sep 21, 2021

mattklein123 closed this Sep 24, 2021

junr03 deleted the gurl_update branch October 6, 2021 21:27

	# Static snapshot of https://quiche.googlesource.com/quiche/+archive/561705e0066ff11e6cb97b8092f1547835beeb92.tar.gz.
	# Static snapshot of https://quiche.googlesource.com/googleurl/+archive/561705e0066ff11e6cb97b8092f1547835beeb92.tar.gz.

Conversation

mattklein123 commented Sep 17, 2021

Uh oh!

repokitteh-read-only bot commented Sep 17, 2021

Uh oh!

mattklein123 commented Sep 17, 2021

Uh oh!

mattklein123 commented Sep 17, 2021

Uh oh!

mattklein123 commented Sep 17, 2021

Uh oh!

keith commented Sep 17, 2021

Uh oh!

wrowe left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Sep 17, 2021

Uh oh!

mattklein123 commented Sep 17, 2021

Uh oh!

wrowe left a comment

Choose a reason for hiding this comment

Uh oh!

keith commented Sep 17, 2021

Uh oh!

dio Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

keith commented Sep 17, 2021

Uh oh!

dio Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 Sep 18, 2021

Choose a reason for hiding this comment

Uh oh!

htuch Sep 19, 2021

Choose a reason for hiding this comment

Uh oh!

bencebeky Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

mattklein123 Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

wrowe commented Sep 21, 2021

Uh oh!

mattklein123 commented Sep 21, 2021

Uh oh!

mattklein123 commented Sep 24, 2021

Uh oh!

keith commented Sep 24, 2021

Uh oh!

keith commented Sep 24, 2021

Uh oh!

mattklein123 commented Sep 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dio Sep 17, 2021 •

edited

Loading