Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
116 changes: 116 additions & 0 deletions bazel/external/googleurl.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
diff --git a/base/compiler_specific.h b/base/compiler_specific.h
index 2962537..6193b56 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.)
@@ -212,7 +208,9 @@
#endif
#endif

-#if defined(__clang__) && __has_attribute(uninitialized)
+#if defined(__clang__)
+#if defined(__has_attribute)
+#if __has_attribute(uninitialized)
Comment on lines +20 to +23

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.

Since we put /Za when compiling on Windows, MSVC's C4067 warning is treated as an error.

// Attribute "uninitialized" disables -ftrivial-auto-var-init=pattern for
// the specified variable.
// Library-wide alternative is
@@ -243,6 +241,8 @@
// E.g. platform, bot, benchmark or test name in patch description or next to
// the attribute.
#define STACK_UNINITIALIZED __attribute__((uninitialized))
+#endif
+#endif
#else
#define STACK_UNINITIALIZED
#endif
diff --git a/base/strings/BUILD b/base/strings/BUILD
index 7a06170..7c86a5f 100644
--- a/base/strings/BUILD
+++ b/base/strings/BUILD
@@ -6,23 +6,21 @@ load("//:build_config.bzl", "build_config")
cc_library(
name = "strings",
srcs = [
- "string16.cc",

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.

string16 doesn't make sense to use on 2-byte wchar_t systems (e.g. Windows).

"string_piece.cc",
"string_util.cc",
"string_util_constants.cc",
"utf_string_conversion_utils.cc",
"utf_string_conversions.cc",
- ],
+ ] + build_config.strings_srcs,
hdrs = [
"char_traits.h",
"string16.h",
"string_piece.h",
"string_piece_forward.h",
"string_util.h",
- "string_util_posix.h",

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.

What's the rationale for this patch to grow larger?

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.

This is for accommodating windows with msvc compiler that we have.

"utf_string_conversion_utils.h",
"utf_string_conversions.h",
- ],
+ ] + build_config.strings_hdrs,
copts = build_config.default_copts,
visibility = ["//visibility:public"],
deps = [
diff --git a/build_config.bzl b/build_config.bzl
index d5fca65..fc0d7e5 100644
--- a/build_config.bzl
+++ b/build_config.bzl
@@ -1,8 +1,25 @@
-_default_copts = [
- "-std=c++14",
- "-fno-strict-aliasing",
-]
+_default_copts = select({
+ "@envoy//bazel:windows_x86_64": [
+ "/std:c++14",
+ ],
+ "//conditions:default": [
+ "-std=c++14",
+ "-fno-strict-aliasing",
+ ],
+})
+
+_strings_srcs = select({
+ "@envoy//bazel:windows_x86_64": [],
+ "//conditions:default": ["string16.cc"],
+})
+
+_strings_hdrs = select({
+ "@envoy//bazel:windows_x86_64": ["string_util_win.h"],
+ "//conditions:default": ["string_util_posix.h"],
+})

build_config = struct(
default_copts = _default_copts,
+ strings_srcs = _strings_srcs,
+ strings_hdrs = _strings_hdrs,
)
diff --git a/url/BUILD b/url/BUILD
index 0126bdc..5d1a171 100644
--- a/url/BUILD
+++ b/url/BUILD
@@ -43,11 +43,11 @@ cc_library(
"url_util.h",
],
copts = build_config.default_copts,
- linkopts = ["-licuuc"],

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.

Why remove this if we now have icuuc?

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.

Since we depend on "@org_unicode_icuuc//:icuuc" directly.

visibility = ["//visibility:public"],
deps = [
"//base",
"//base/strings",
"//polyfills",
+ "@org_unicode_icuuc//:common",
],
)
57 changes: 57 additions & 0 deletions bazel/external/icuuc.BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
licenses(["notice"]) # Apache 2

exports_files([
"icu4c/LICENSE",
"icu4j/main/shared/licenses/LICENSE",
])

icuuc_copts = [
"-DU_STATIC_IMPLEMENTATION",
"-DU_COMMON_IMPLEMENTATION",
"-DU_HAVE_STD_ATOMICS",
] + select({
"@envoy//bazel:apple": [
"-Wno-shorten-64-to-32",
"-Wno-unused-variable",
],
"@envoy//bazel:windows_x86_64": [
"/utf-8",
"/DLOCALE_ALLOW_NEUTRAL_NAMES=0",
],
# TODO(dio): Add "@envoy//bazel:android" when we have it.
# "@envoy//bazel:android": [
# "-fdata-sections",
# "-DU_HAVE_NL_LANGINFO_CODESET=0",
# "-Wno-deprecated-declarations",
# ],
"//conditions:default": [],
})

cc_library(
name = "headers",
hdrs = glob(["icu4c/source/common/unicode/*.h"]),
includes = ["icu4c/source/common"],
visibility = ["//visibility:public"],
)

cc_library(
name = "common",
hdrs = glob(["icu4c/source/common/unicode/*.h"]),
includes = ["icu4c/source/common"],
visibility = ["//visibility:public"],
deps = [":icuuc"],
)

cc_library(
name = "icuuc",
srcs = glob([
"icu4c/source/common/*.c",
"icu4c/source/common/*.cpp",
"icu4c/source/stubdata/*.cpp",
]),
hdrs = glob(["icu4c/source/common/*.h"]),
copts = icuuc_copts,
tags = ["requires-rtti"],
visibility = ["//visibility:private"],
deps = [":headers"],
)

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.

This is an autoconf dependency, can we just use foreign_cc to avoid having to hand-maintain a bespoke BUILD file for the project?

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'm testing it, but it seems the build time is significantly increasing.

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.

Interesting, I wonder if it's doing something like building tests or additional libraries. Might be possible to control with some autoconf flags...

@dio dio Apr 6, 2020

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 believe I have turned off everything (that makes sense, like examples, tests, even tools). And it is tricky to force using msvc (when using configure it tends to use mingw and it is failed at configure stage, and I don't think we want to mix mingw and msvc).

14 changes: 14 additions & 0 deletions bazel/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ def envoy_dependencies(skip_targets = []):
_com_googlesource_quiche()
_com_googlesource_googleurl()
_com_lightstep_tracer_cpp()
_org_unicode_icuuc()
_io_opentracing_cpp()
_net_zlib()
_upb()
Expand Down Expand Up @@ -227,6 +228,17 @@ def _com_github_cyan4973_xxhash():
actual = "@com_github_cyan4973_xxhash//:xxhash",
)

def _org_unicode_icuuc():
_repository_impl(
name = "org_unicode_icuuc",
build_file = "@envoy//bazel/external:icuuc.BUILD",
# TODO(dio): Consider patching udata when we need to embed some data.
)
native.bind(
name = "icuuc",
actual = "@org_unicode_icuuc//:common",
)

def _com_github_envoyproxy_sqlparser():
_repository_impl(
name = "com_github_envoyproxy_sqlparser",
Expand Down Expand Up @@ -660,6 +672,8 @@ def _com_googlesource_quiche():
def _com_googlesource_googleurl():
_repository_impl(
name = "com_googlesource_googleurl",
patches = ["@envoy//bazel/external:googleurl.patch"],
patch_args = ["-p1"],
)
native.bind(
name = "googleurl",
Expand Down
10 changes: 8 additions & 2 deletions bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -282,8 +282,9 @@ REPOSITORY_LOCATIONS = dict(
),
com_googlesource_googleurl = dict(
# Static snapshot of https://quiche.googlesource.com/quiche/+archive/googleurl_dbf5ad147f60afc125e99db7549402af49a5eae8.tar.gz
sha256 = "b40cd22cadba577b7281a76db66f6a66dd744edbad8cc2c861c2c976ef721e4d",
urls = ["https://storage.googleapis.com/quiche-envoy-integration/googleurl_dbf5ad147f60afc125e99db7549402af49a5eae8.tar.gz"],
sha256 = "5bdbecce22d522f03ac86c6a694d5ff76780a16d6d682910e87ac156b0a25b21",
strip_prefix = "googleurl-dbf5ad1",
urls = ["https://github.com/dio/googleurl/archive/dbf5ad1.tar.gz"],

@dio dio Apr 2, 2020

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.

A temporary fix, since it has a top-level BUILD file and a build directory (which is problematic when extracting on mac and windows). I renamed the BUILD file to BUILD.bazel, I think removing it is OK.

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 assume can just patch?

@dio dio Apr 6, 2020

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.

Seems like we can't, since it is failing when http_archive extracting the tarball, it is stopped when trying to extract BUILD when it saw build dir is already there. I'm not sure how we can have a patch on the downloaded tarball before extracting.

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.

Probably @danzh2010 could shed some light here.

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.

A temporary fix, since it has a top-level BUILD file and a build directory (which is problematic when extracting on mac and windows). I renamed the BUILD file to BUILD.bazel, I think removing it is OK.

The top level BUILD file is a dummy but needed for the build_config.bzl to setup default copts. But they probably can be moved to a subdirectory instead.

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.

As to build directory, it is from chromium, so nothing I can do there. What's the exact issue of having it under root directory?

@dio dio Apr 15, 2020

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.

@danzh2010 with the original archive, http_archive tries to extract BUILD file while the build directory is already extracted (the build directory is extracted before the BUILD file from the tarball) and http_archive throws error. It happens only for systems with case-insensitive filesystem like Windows and macOS.

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.

If I move the top level BUILD file and build_config.bzl to some sub-directory, would it help your case?

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.

Yes! Thanks @danzh2010!

@dio dio Jun 20, 2020

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.

@danzh2010 sorry. Can we still pick this up? (moving the top-level BUILD file sub-directory)

),
com_google_cel_cpp = dict(
sha256 = "326ec397b55e39f48bd5380ccded1af5b04653ee96e769cd4d694f9a3bacef50",
Expand Down Expand Up @@ -330,4 +331,9 @@ REPOSITORY_LOCATIONS = dict(
strip_prefix = "kafka-python-2.0.0",
urls = ["https://github.com/dpkp/kafka-python/archive/2.0.0.tar.gz"],
),
org_unicode_icuuc = dict(
strip_prefix = "icu-release-64-2",
sha256 = "524960ac99d086cdb6988d2a92fc163436fd3c6ec0a84c475c6382fbf989be05",
urls = ["https://github.com/unicode-org/icu/archive/release-64-2.tar.gz"],
),
)
14 changes: 13 additions & 1 deletion source/common/http/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,6 @@ envoy_cc_library(
hdrs = ["utility.h"],
external_deps = [
"abseil_optional",
"http_parser",
"nghttp2",
],
deps = [
Expand All @@ -361,6 +360,19 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "url_utility_lib",
srcs = ["url_utility.cc"],
hdrs = ["url_utility.h"],
external_deps = [
"googleurl",
],
deps = [
"//source/common/common:assert_lib",
"//source/common/common:empty_string",
],
)

envoy_cc_library(
name = "header_utility_lib",
srcs = ["header_utility.cc"],
Expand Down
1 change: 1 addition & 0 deletions source/common/http/http1/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ envoy_cc_library(
"//source/common/http:header_map_lib",
"//source/common/http:header_utility_lib",
"//source/common/http:headers_lib",
"//source/common/http:url_utility_lib",
"//source/common/http:utility_lib",
"//source/common/http/http1:header_formatter_lib",
"//source/common/runtime:runtime_lib",
Expand Down
5 changes: 3 additions & 2 deletions source/common/http/http1/codec_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "common/http/header_utility.h"
#include "common/http/headers.h"
#include "common/http/http1/header_formatter.h"
#include "common/http/url_utility.h"
#include "common/http/utility.h"
#include "common/runtime/runtime_impl.h"

Expand Down Expand Up @@ -758,9 +759,9 @@ void ServerConnectionImpl::handlePath(RequestHeaderMap& headers, unsigned int me
// request-target. A proxy that forwards such a request MUST generate a
// new Host field-value based on the received request-target rather than
// forward the received Host field-value.
headers.setHost(absolute_url.host_and_port());
headers.setHost(absolute_url.getHostAndPort());

headers.setPath(absolute_url.path_and_query_params());
headers.setPath(absolute_url.getPathAndQueryParams());
active_request.request_url_.clear();
}

Expand Down
48 changes: 48 additions & 0 deletions source/common/http/url_utility.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#include "common/http/url_utility.h"

#include "common/common/assert.h"
#include "common/common/empty_string.h"

#include "absl/strings/str_cat.h"
#include "url/gurl.h"

namespace Envoy {
namespace Http {
namespace Utility {

constexpr char COLON[] = ":";
constexpr char HASH[] = "#";

bool Url::initialize(absl::string_view absolute_url) {

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.

At some point, can we get rid of https://github.com/envoyproxy/envoy/tree/master/source/common/chromium_url by doing the normalization in this class via GURL?

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 think so yeah.

// TODO(dio): When we have access to base::StringPiece, probably we can convert absolute_url to
// that instead.
GURL parsed(std::string{absolute_url});
if (!parsed.is_valid() || (!parsed.SchemeIsHTTPOrHTTPS())) {
return false;
}

scheme_ = parsed.scheme();

// Only non-default ports will be rendered as part of host_and_port_. For example,
// http://www.host.com:80 has port component (i.e. 80). However, since 80 is a default port for
// http scheme, host_and_port_ will be rendered as www.host.com (without port). The same case with
// https scheme (with port 443) as well.
host_and_port_ =
absl::StrCat(parsed.host(), parsed.has_port() ? COLON : EMPTY_STRING, parsed.port());

const int port = parsed.EffectiveIntPort();
ASSERT(port <= std::numeric_limits<uint16_t>::max() && port >= 0);
port_ = static_cast<uint16_t>(port);

// RFC allows the absolute-uri to not end in "/", but the absolute path form must start with "/".
path_and_query_params_ = parsed.PathForRequest();
if (parsed.has_ref()) {
absl::StrAppend(&path_and_query_params_, HASH, parsed.ref());
}

return true;
}

} // namespace Utility
} // namespace Http
} // namespace Envoy
32 changes: 32 additions & 0 deletions source/common/http/url_utility.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#pragma once

#include <string>

#include "absl/strings/string_view.h"

namespace Envoy {
namespace Http {
namespace Utility {
/**
* Given a fully qualified URL, splits the string_view provided into scheme, host, port and path
* with query parameters components (including fragments).
*/
class Url {
public:
bool initialize(absl::string_view absolute_url);
absl::string_view getScheme() const { return scheme_; }
absl::string_view getHostAndPort() const { return host_and_port_; }
absl::string_view getPathAndQueryParams() const { return path_and_query_params_; }
uint64_t getPort() const { return port_; }

private:
std::string scheme_;
std::string host_and_port_;
std::string path_and_query_params_;
uint16_t port_{0};
};

} // namespace Utility

} // namespace Http
} // namespace Envoy
Loading