[WIP] bazel: use experimental cmake_external for cares/nghttp2. by htuch · Pull Request #4516 · envoyproxy/envoy

htuch · 2018-09-24T18:23:58Z

This demonstrates the use of the new cmake_external() rule provided by rules_foreign_cc.

There are a number of issues to address and discuss in this PR before merging.

Risk level: Low
Testing: bazel test //test/... on Linux.

Signed-off-by: Harvey Tuch htuch@google.com

This demonstrates the use of the new cmake_external() rule provided by rules_foreign_cc. There are a number of issues to address and discuss in this PR before merging. Risk level: Low Testing: bazel test //test/... on Linux. Signed-off-by: Harvey Tuch <htuch@google.com>

htuch

@irengrig FYI. Thanks so much for driving this effort, it "just worked" for us out of the box.

htuch · 2018-09-24T18:25:07Z

WORKSPACE

 load("//bazel:cc_configure.bzl", "cc_configure")

 envoy_dependencies()
+load("@rules_foreign_cc//:workspace_definitions.bzl", "rules_foreign_cc_dependencies")


I would like to figure out how to roll this into envoy_dependencies(), but it depends on some of the setup there. load can't be used inside a macro, only at file level or in WORKSPACE. Might be possibly with a multi-phase envoy_dependencies() setup.

afaik, unfortunately you can only call the initialization function directly in WORKSPACE file. Should be resolved when recursive workspace paring be implemented.

htuch · 2018-09-24T18:25:31Z

bazel/foreign_cc/BUILD

+    tools_deps = NINJA_DEP,
+)
+
+cmake_external(


@sesmith177 any chance you can try out this PR and let me know if it does the right thing on Windows?

we can take a look at it when we get a chance. One thing we noticed is that the names of the static libraries are different on Windows and Linux ( cares.lib and nghttp2.lib vs libcares.a and libnghttp2.a)

Hi, here's my modified cares build for Windows (I just completed it, will update the examples):

cmake_external(
name = "cares",
cache_entries = {
"CARES_SHARED": "no",
"CARES_STATIC": "on",
},
defines = ["CARES_STATICLIB"],
cmake_options = ["-GNinja"],
lib_source = "@cares//:all",
make_commands = [
"ninja",
"ninja install",
],
)

Since on Wndows cares.lib is created, "static_libraries" can be omitted (cares.lib is the calculated default)

defines = ["CARES_STATICLIB"], is very important, for some reason CMake script does not set this up because of corresponding CMake variable

I used the preinstalled ninja, seems one should not build it on Windows

Hope it helps. Will be happy to answer any questions.

htuch · 2018-09-24T18:26:11Z

bazel/EXTERNAL_DEPS.md

+   `envoy_dependencies()` function.
+2. Add a `cmake_external` rule to [`bazel/foreign_cc/BUILD`](bazel/foreign_cc/BUILD). This will
+   reference the source repository in step 1.
+3. Reference your new external dependency in some `envoy_cc_library` via Y in the


Will improve the writeup before we merge here. This is now like the 5th way of handling external deps (obligatory https://xkcd.com/927/).

htuch · 2018-09-24T18:28:04Z

bazel/foreign_cc/BUILD

+    ],
+    static_libraries = ["libnghttp2.a"],
+    tools_deps = NINJA_DEP,
+)


We should add some more of the cmake deps as well once we have agreement on when we will be merging this.

htuch · 2018-09-24T18:28:57Z

tools/bazel.rc

 # Envoy specific Bazel build/test options.

 build --workspace_status_command=bazel/get_workspace_status
+build --experimental_cc_skylark_api_enabled_packages=@rules_foreign_cc//tools/build_defs,tools/build_defs


This is the main blocker to use merging this PR; we don't want folks to have to be setting this when importing Envoy to their own projects. CC @irengrig.

@irengrig this aged out of our review process, because we haven't figured out these last few steps and weren't able to verify Windows support (@sesmith177). Do you think there is any way to resolve these?

@htuch we have used a version of bazel that has the fix for bazelbuild/bazel#6292 and verified that we can build Envoy with it.

We have not had a chance yet to take another look at this PR

Hi @htuch, I asked the people from Bazel C++ team, they estimate the work for getting rid of the --experimental_cc_skylark_api_enabled_packages flag as at least two weeks.
The task is their priority, but it takes time.

Nice to hear about Windows build.

Thanks @irengrig. Please ping me when this is done and we will try and switch this on in master (hopefully @sesmith177 and MSFT will have time to evaluate by then).

it gets farther on windows with the newer bazel, but still fails to build:

ERROR: C:/vmfiles/envoy/bazel/foreign_cc/BUILD:25:1: no such package '@com_github_nghttp2_nghttp2//': Traceback (most recent call last): File "C:/_eb/external/bazel_tools/tools/build_defs/repo/http.bzl", line 55 patch(ctx) File "C:/_eb/external/bazel_tools/tools/build_defs/repo/utils.bzl", line 85, in patch fail(("Error applying patch command %...))) Error applying patch command find . -name '*.sh' -exec sed -i.orig '1s|#!/usr/bin/env sh\$|/bin/sh\$|' {} +: ---------- LTMAIN.SH Access denied - . File not found - -NAME File not found - -EXEC File not found - SED File not found - -I.ORIG File not found - {} File not found - + and referenced by '//bazel/foreign_cc:nghttp2'

we haven't had too much time to debug this

Signed-off-by: Harvey Tuch <htuch@google.com>

stale · 2018-10-01T22:35:14Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

sesmith177 · 2018-10-02T16:03:05Z

@htuch as is, it doesn't look like this works on Windows, bazel threw an exception while trying to build (see attached java.log)

irengrig · 2018-10-02T16:58:29Z

@htuch as is, it doesn't look like this works on Windows, bazel threw an exception while trying to build (see attached java.log)

Hi, I was able to reproduce it and recorded an issue: bazelbuild/bazel#6292.
The problem is not relevant to the cmake_external...
On Thursday I will ping the guys, tomorrow we are having a public holiday in Germany.

sesmith177 · 2018-10-02T17:45:54Z

@irengrig thanks for reporting the issue -- we saw the same thing with bazel 0.17.2 and without these cmake_external changes.

As a side note, the master branch of envoyproxy/envoy doesn't currently build on Windows -- we are currently working on this. If you need a branch that does build on Windows, we maintain one here: https://github.com/greenhouse-org/envoy/tree/windows-linux-build

stale · 2018-10-09T18:27:33Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2018-10-16T18:33:28Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

sesmith177 · 2018-11-12T22:48:24Z

@htuch @irengrig We've taken another look at this and were able to get it working on Windows, so I think we can remove Windows from the blockers here

htuch · 2018-11-12T23:36:30Z

@sesmith177 appreciated, thanks for the update.

htuch · 2018-12-04T19:57:21Z

FWIW, I'm resuming this work now, hope to have a PR out in next day @sesmith177 @mattklein123

moderation · 2018-12-04T20:16:17Z

bazel/repository_locations.bzl

    ),
+    com_github_nghttp2_nghttp2 = dict(
+        sha256 = "42fff7f290100c02234ac3b0095852e4392e6bfd95ebed900ca8bd630850d88c",
+        strip_prefix = "nghttp2-1.33.0",


@htuch nghttp2 version 1.35.0 is out if you would like to bump to that version. Master is currently set at 1.34.0
SHA256 for 1.35.0 is ea04e4e749df13d60a88b706752e04f6f907227bf380aefd7aeeb4aa0db43407

Yep, thanks, already bumped the versions.

This revives envoyproxy#4516, replacing all external cmake deps with the pending bazel foreign_cc support. To make use of this, with the current release Bazel version, it's necessary to set --experimental_cc_skylark_api_enabled_packages=@rules_foreign_cc//tools/build_defs,tools/build_defs on the CLI or in .bazelrc. Risk Level: Low Testing: bazel test //test/... Signed-off-by: Harvey Tuch <htuch@google.com>

This revives #4516, replacing all external cmake deps with the pending bazel foreign_cc support. Risk Level: Low Testing: bazel test //test/... Signed-off-by: Harvey Tuch <htuch@google.com>

This revives envoyproxy#4516, replacing all external cmake deps with the pending bazel foreign_cc support. Risk Level: Low Testing: bazel test //test/... Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: htuch <htuch@users.noreply.github.com>

This revives envoyproxy#4516, replacing all external cmake deps with the pending bazel foreign_cc support. Risk Level: Low Testing: bazel test //test/... Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

htuch requested a review from jmillikin-stripe September 24, 2018 18:24

htuch assigned jmillikin-stripe Sep 24, 2018

htuch commented Sep 24, 2018

View reviewed changes

htuch added 3 commits September 24, 2018 14:30

Fix CI WORKSPACE.

f57ff18

Signed-off-by: Harvey Tuch <htuch@google.com>

ASAN/TSAN build fix.

0248526

Signed-off-by: Harvey Tuch <htuch@google.com>

Fix coverage.

0f36267

Signed-off-by: Harvey Tuch <htuch@google.com>

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 1, 2018

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Oct 2, 2018

irengrig mentioned this pull request Oct 2, 2018

IllegalStateException while building C++ on Windows bazelbuild/bazel#6292

Closed

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 9, 2018

stale bot closed this Oct 16, 2018

sesmith177 mentioned this pull request Nov 12, 2018

build: Envoy has release builds on Windows #5006

Closed

moderation reviewed Dec 4, 2018

View reviewed changes

htuch mentioned this pull request Dec 4, 2018

build: external_cmake for cmake deps. #5218

Merged

Conversation

htuch commented Sep 24, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Oct 1, 2018

Uh oh!

sesmith177 commented Oct 2, 2018

Uh oh!

irengrig commented Oct 2, 2018

Uh oh!

sesmith177 commented Oct 2, 2018

Uh oh!

stale bot commented Oct 9, 2018

Uh oh!

stale bot commented Oct 16, 2018

Uh oh!

sesmith177 commented Nov 12, 2018

Uh oh!

htuch commented Nov 12, 2018

Uh oh!

htuch commented Dec 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants