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.

Sign up for GitHub

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

Jump to bottom

bazel: update DEPS on googleurl #17794

Merged
merged 5 commits into from
Sep 1, 2021
Merged

bazel: update DEPS on googleurl #17794

merged 5 commits into from
Sep 1, 2021

Conversation

RenjieTang
Copy link
Contributor

Commit Message: update DEPS on googleurl
Risk Level: Low
Testing: Current test suites.
Docs Changes: n/a

@RenjieTang
update DEPS on googleurl
f2124db 
Signed-off-by: Renjie Tang <renjietang@google.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: #17794 was opened by RenjieTang.

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: #17794 was opened by RenjieTang.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Aug 20, 2021
@RenjieTang RenjieTang marked this pull request as ready for review August 23, 2021 16:46
@RenjieTang
Copy link
Contributor Author

/assign @wrowe
Hi William, in this PR I added "-Wno-c++11-narrowing" in bazel compile options. It seems that Windows doesn't support it. Do you have any suggestion on this?

moderation
moderation reviewed Aug 23, 2021
View reviewed changes
bazel/repository_locations.bzl
sha256 = "d769283fed1319bca68bae8bdd47fbc3a7933999329eee850eff1f1ea61ce176",
# Static snapshot of https://quiche.googlesource.com/quiche/+archive/578e68d9ae4c25e37bc0d23460f3e2f183fe526a.tar.gz.
version = "578e68d9ae4c25e37bc0d23460f3e2f183fe526a",
sha256 = "dd13bd6732941a2a869e930503daff44c84eabe92788125bebcbe6b1260587c1",
urls = ["https://storage.googleapis.com/quiche-envoy-integration/googleurl_{version}.tar.gz"],
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering with the recent switch of Quiche deps from quiche.googlesource.com (#17732) I was wondering if this dependency can change too? It doesn't look like the envoy-integration branch is being updated at the Github location - https://github.com/google/quiche/commits/envoy-integration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think we can probably move googleurl into Github too. @danzh2010 for more comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chatted with Dan, and it seems that @dio has been in charge of this dep.
I currently need to update this dep to do some URL sanitization work. After this PR, @dio can decide how he wants to proceed with the repo location.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

PS. Sorry for the very late reply was out for a long time.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to move to GH like we did for quiche, but let's discuss on the unrevert PR?

@wrowe
Copy link
Contributor

wrowe commented Aug 30, 2021

Waiting on moving this source to GitHub? Or ready to fly as-is?

wrowe
wrowe suggested changes Aug 30, 2021
View reviewed changes
bazel/external/googleurl.patch Outdated
--- a/url/BUILD
+++ b/url/BUILD
@@ -52,3 +52,27 @@ cc_library(
@@ -54,3 +54,29 @@ cc_library(
"//polyfills",
] + build_config.icuuc_deps,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unclear what causes the compilation failure here on windows, I see nothing specific, could you merge main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this PR can merge as is, and I will let @dio decide on the repo migration.
Currently the updated googleurl library has -Wno-c++11-narrowing.
Windows CI is throwing "cl : Command line error D8021 : invalid numeric argument '/Wno-c++11-narrowing'"

Copy link
Contributor

Choose a reason for hiding this comment

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

/wait

This cannot be merged as is, see the bazel/*.bzl logic on how we toggle flags by compiler.

@junr03
Copy link
Member

junr03 commented Aug 30, 2021

/wait

@RenjieTang
try to fix MSVC complier issue
0fd85f5 
Signed-off-by: Renjie Tang <renjietang@google.com>
@RenjieTang
try to supress msvc 2397 warning
5e81f1f 
Signed-off-by: Renjie Tang <renjietang@google.com>
@RenjieTang
update to a later version
935c892 
Signed-off-by: Renjie Tang <renjietang@google.com>
@RenjieTang
fix some MSVC compiler errors
c13c88e 
Signed-off-by: Renjie Tang <renjietang@google.com>
@RenjieTang
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #17794 (comment) was created by @RenjieTang.

see: more, trace.

@RenjieTang
Copy link
Contributor Author

A newer version of googleurl removes the use of -Wno-c++11-narrowing.
I updated the DEPS and this PR should be good to go.

@moderation
Copy link
Contributor

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Sep 1, 2021
wrowe
wrowe approved these changes Sep 1, 2021
View reviewed changes
Copy link
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

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

LGTM, that last change to avoid compiler overrides is terrific, ty.

@wrowe wrowe merged commit c76c951 into envoyproxy:main Sep 1, 2021
RenjieTang added a commit to RenjieTang/envoy that referenced this pull request Sep 1, 2021
@RenjieTang
Revert "bazel: update DEPS on googleurl (envoyproxy#17794)"
bd736cd 
This reverts commit c76c951.
@RenjieTang RenjieTang mentioned this pull request Sep 1, 2021
Closed
RenjieTang added a commit to RenjieTang/envoy that referenced this pull request Sep 2, 2021
@RenjieTang
Revert "bazel: update DEPS on googleurl (envoyproxy#17794)"
78c0bf7 
This reverts commit c76c951.

Signed-off-by: Renjie Tang <renjietang@google.com>
@RenjieTang RenjieTang mentioned this pull request Sep 2, 2021
Merged
junr03 pushed a commit that referenced this pull request Sep 2, 2021
@RenjieTang
Revert "bazel: update DEPS on googleurl (#17794)" (#17958)
338a42c 
This reverts commit c76c951.

Signed-off-by: Renjie Tang <renjietang@google.com>
mattklein123 added a commit that referenced this pull request Sep 17, 2021
@mattklein123
Revert "Revert "bazel: update DEPS on googleurl (#17794)" (#17958)"
95441bb 
This reverts commit 338a42c.

Signed-off-by: Matt Klein <mklein@lyft.com>
@dio dio mentioned this pull request Sep 17, 2021
Closed
buildbreaker pushed a commit to buildbreaker/envoy that referenced this pull request Sep 21, 2021
Revert "Revert "bazel: update DEPS on googleurl (envoyproxy#17794)" (e…
3eb179e 
…nvoyproxy#17958)"

This reverts commit 338a42c.
@buildbreaker buildbreaker mentioned this pull request Sep 24, 2021
Closed
keith pushed a commit to keith/envoy that referenced this pull request Sep 29, 2021
@mattklein123 @keith
Revert "Revert "bazel: update DEPS on googleurl (envoyproxy#17794)" (e…
379ce3f 
…nvoyproxy#17958)"

This reverts commit 338a42c.

Signed-off-by: Matt Klein <mklein@lyft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 left review comments

@moderation moderation moderation left review comments

@dio dio dio left review comments

@wrowe wrowe wrowe approved these changes

Assignees

@wrowe wrowe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@RenjieTang @wrowe @junr03 @moderation @dio @mattklein123