Skip to content

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

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

Closed
mattklein123 wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95441bb
Revert "Revert "bazel: update DEPS on googleurl (#17794)" (#17958)"
mattklein123 Sep 17, 2021
799972c
fix
mattklein123 Sep 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 54 additions & 12 deletions bazel/external/googleurl.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,22 @@
# project using clang-cl. Tracked in https://github.com/envoyproxy/envoy/issues/11974.

diff --git a/base/compiler_specific.h b/base/compiler_specific.h
index 0cd36dc..8c4cbd4 100644
index 6651220..a469c19 100644
--- a/base/compiler_specific.h
+++ b/base/compiler_specific.h
@@ -7,10 +7,6 @@

#include "build/build_config.h"

-#if defined(COMPILER_MSVC) && !defined(__clang__)
-#error "Only clang-cl is supported on Windows, see https://crbug.com/988071"
-#endif
-
// Annotate a variable indicating it's ok if the variable is not used.
// (Typically used to silence a compiler warning when the assignment
// is important for some other reason.)
@@ -55,8 +51,12 @@
// prevent code folding, see gurl_base::debug::Alias.
// This is a wrapper around `__has_cpp_attribute`, which can be used to test for
// the presence of an attribute. In case the compiler does not support this
// macro it will simply evaluate to 0.
@@ -75,8 +71,12 @@
// prevent code folding, see NO_CODE_FOLDING() in base/debug/alias.h.
// Use like:
// void NOT_TAIL_CALLED FooBar();
-#if defined(__clang__) && __has_attribute(not_tail_called)
Expand All @@ -30,18 +30,18 @@ index 0cd36dc..8c4cbd4 100644
#else
#define NOT_TAIL_CALLED
#endif
@@ -226,7 +226,9 @@
@@ -273,7 +273,9 @@
#endif
#endif

-#if defined(__clang__) && __has_attribute(uninitialized)
+#if defined(__clang__)
+#if defined(__has_attribute)
+#if __has_attribute(uninitialized)
// Attribute "uninitialized" disables -ftrivial-auto-var-init=pattern for
// the specified variable.
// Library-wide alternative is
@@ -257,6 +259,8 @@
@@ -304,6 +306,8 @@
// E.g. platform, bot, benchmark or test name in patch description or next to
// the attribute.
#define STACK_UNINITIALIZED __attribute__((uninitialized))
Expand All @@ -50,13 +50,55 @@ index 0cd36dc..8c4cbd4 100644
#else
#define STACK_UNINITIALIZED
#endif
@@ -365,8 +369,12 @@ inline constexpr bool AnalyzerAssumeTrue(bool arg) {
#endif // defined(__clang_analyzer__)

// Use nomerge attribute to disable optimization of merging multiple same calls.
-#if defined(__clang__) && __has_attribute(nomerge)
+#if defined(__clang__)
+#if defined(__has_attribute)
+#if __has_attribute(nomerge)
#define NOMERGE [[clang::nomerge]]
+#endif
+#endif
#else
#define NOMERGE
#endif
@@ -392,8 +400,12 @@ inline constexpr bool AnalyzerAssumeTrue(bool arg) {
// See also:
// https://clang.llvm.org/docs/AttributeReference.html#trivial-abi
// https://libcxx.llvm.org/docs/DesignDocs/UniquePtrTrivialAbi.html
-#if defined(__clang__) && __has_attribute(trivial_abi)
+#if defined(__clang__)
+#if defined(__has_attribute)
+#if __has_attribute(trivial_abi)
#define TRIVIAL_ABI [[clang::trivial_abi]]
+#endif
+#endif
#else
#define TRIVIAL_ABI
#endif
@@ -401,8 +413,12 @@ inline constexpr bool AnalyzerAssumeTrue(bool arg) {
// Marks a member function as reinitializing a moved-from variable.
// See also
// https://clang.llvm.org/extra/clang-tidy/checks/bugprone-use-after-move.html#reinitialization
-#if defined(__clang__) && __has_attribute(reinitializes)
+#if defined(__clang__)
+#if defined(__has_attribute)
+#if __has_attribute(reinitializes)
#define REINITIALIZES_AFTER_MOVE [[clang::reinitializes]]
+#endif
+#endif
#else
#define REINITIALIZES_AFTER_MOVE
#endif

# TODO(dio): Consider to remove the following patch when we have IDN-free optional build for URL
# library from the upstream Chromium project. This is tracked in:
# https://github.com/envoyproxy/envoy/issues/14743.

diff --git a/url/BUILD b/url/BUILD
index f2ec8da..4e2d55b 100644
index f2ec8da..951039b 100644
--- a/url/BUILD
+++ b/url/BUILD
@@ -52,3 +52,27 @@ cc_library(
Expand Down
8 changes: 4 additions & 4 deletions bazel/repository_locations.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,13 +834,13 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_name = "Chrome URL parsing library",
project_desc = "Chrome URL parsing library",
project_url = "https://quiche.googlesource.com/googleurl",
# 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.

@dio dio Sep 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@dio dio Sep 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@mattklein123 mattklein123 Sep 18, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

@htuch htuch Sep 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@bencebeky bencebeky Sep 20, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@mattklein123 mattklein123 Sep 20, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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?

version = "561705e0066ff11e6cb97b8092f1547835beeb92",
sha256 = "7ce00768fea1fa4c7bf658942f13e41c9ba30e9cff931a6cda2f9fd02289f673",
urls = ["https://storage.googleapis.com/quiche-envoy-integration/googleurl_{version}.tar.gz"],
use_category = ["controlplane", "dataplane_core"],
extensions = [],
release_date = "2020-07-30",
release_date = "2021-08-31",
cpe = "N/A",
),
com_google_cel_cpp = dict(
Expand Down
Binary file added googleurl_578e68d9ae4c25e37bc0d23460f3e2f183fe526a.tar.gz
View file
Open in desktop
Binary file not shown.