build: enable release builds on Windows by sesmith177 · Pull Request #5812 · envoyproxy/envoy

sesmith177 · 2019-02-01T21:03:33Z

Description:

Now that #5218 has been merged, we can enable release builds on Windows. This PR updates ci/do_ci.ps1 to build release + fastbuild versions in addition to debug versions. It also removes support for build scripts in ci/build_container/build_recipes/ on Windows, as there is no clean way to pass information from Bazel regarding the type of build to those scripts

Risk Level:
Low

Testing:
bazel build //source/... && bazel test //test/...

Docs Changes:
N/A
Release Notes:
N/A

Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 · 2019-02-01T21:28:01Z

Note: on our fork, we've actually commented out the CMAKE_BUILD_TYPE cache_entries in bazel/foreign_cc -- it looks like Bazel / rules_foreign_cc are correctly passing the right arguments through on Windows. Similarly, if we don't comment out those lines, they would have to switch between "Debug" and "'Release" depending on the build type.

Since select doesn't work great with dicts (can't be used inside one to select a single value), we've held off on solving this problem (which would probably require wrapping cmake_external with some sort of macro) for now

htuch · 2019-02-05T23:08:30Z

bazel/foreign_cc/BUILD

+    }),
+)
+
+cmake_external(


Can we create an envoy_cmake_external macro that hides the boiler plate?

The real boiler plate part is generate_crosstool_file, why this is needed @sesmith177? If it is a general issue probably upstream to rules_foreign_cc?

According to the README (and our testing), you need to explicitly specify the option to generate the CMake crosstool file for rules_foreign_cc to work on Windows

The boilerplate to me is both the postfix_script library retrieval of PDB and the crosstool code. Is there a better way to support them in rules_foreign_cc? CC @irengrig.

We also have the ninja configuration as boilerplate. Hence why I reckon it would be useful to make a envoy_cmake_external and hide some of this away, but I agree with @lizan we could push further back into rules_foreign_cc some more standard things.

htuch · 2019-02-05T23:10:14Z

bazel/foreign_cc/nghttp2.patch

+  endif()
+ endif()
+ # AC_TYPE_UINT8_T
+ # AC_TYPE_UINT16_T


What are the plans to upstream this? As it stands, having this patch will make version bumps hard as we will need to treat the patch manually.

A previous attempt to fix the ssize_t issue on Windows: nghttp2/nghttp2#616 was reverted, since adding the typedef could break projects that have code that also #define ssize_t on Windows.

For the rest of the patch, there's been no movement on the PR we submitted back in July

Thanks for the update. It would be great if we can get this Windows support landed upstream, CC @tatsuhiro-t. Do we at least have a roadmap for how to do this?

How about adding a cmake flag to define ssize_t as int64_t? By default, ssize_t is int for backward compatibility.

Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 · 2019-02-06T16:11:26Z

@htuch I've added a macro that hides at least some of the boilerplate (and also handles removing CMAKE_BUILD_TYPE on Windows). I think it should be possible to push the PDB selection in there as well, though handling the fact that benchmark has its own postfix_script might be a little tricky

Signed-off-by: Sam Smith <sesmith177@gmail.com>

htuch

Yep, this is a good direction, thanks.

htuch · 2019-02-07T03:25:02Z

bazel/envoy_build_system.bzl

+    cache_entries_no_build_type = {}
+    for key in cache_entries.keys():
+        if key != "CMAKE_BUILD_TYPE":
+            cache_entries_no_build_type[key] = cache_entries[key]


Does Starlark not have dictionary comprehensions? Should be 1 line in Python.

htuch · 2019-02-07T03:26:36Z

bazel/foreign_cc/BUILD

-    ],
-    static_libraries = ["libcares.a"],
+    postfix_script = select({
+        "//bazel:windows_dbg_build": "cp $BUILD_TMPDIR/CMakeFiles/c-ares.dir/c-ares.pdb $INSTALLDIR/lib/c-ares.pdb",


Yeah, to the extent you can push in most of this that make sense and parameterize by path it'd be good. We can still add a postfix script for additional stuff as needed on each target.

Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 · 2019-02-07T14:33:42Z

@htuch ready for another look

irengrig · 2019-02-07T16:14:02Z

bazel/envoy_build_system.bzl

+    # rules_foreign_cc will figure it out for us
+    cache_entries_no_build_type = {}
+    for key in cache_entries.keys():
+        if key != "CMAKE_BUILD_TYPE":


CMAKE_BUILD_TYPE is calculated by cmake_external internally, from the type of Bazel build.
I added the way how to avoid that: bazel-contrib/rules_foreign_cc#224

Please pass "CMAKE_BUILD_TYPE": "" (empty string) to cmake_external, and then the computed value for CMAKE_BUILD_TYPE will not be used (there would be no "-DCMAKE_BUILD_TYPE=" on the cmake call.)

On Windows, we want to use the computed value for CMAKE_BUILD_TYPE so we can get the desired release / debug builds

htuch · 2019-02-08T05:03:47Z

bazel/envoy_build_system.bzl

+    pf = ""
+    if copy_pdb:
+        name_to_pdb = {"ares": "c-ares", "nghttp2": "nghttp2_static", "zlib": "zlibstatic"}
+        pdb_name = name_to_pdb.get(name, name)


I like all of this PR except this bit :) Can we pass in the pdf_name as an arg to envoy_cmake_external, to avoid having target specific logic/defines in the macro?

makes sense, done

Signed-off-by: Sam Smith <sesmith177@gmail.com>

htuch

Rad, thanks!

lizan · 2019-02-08T22:47:08Z

bazel/foreign_cc/BUILD

 )

-cmake_external(
+envoy_cmake_external(


Hmm, I missed this, why did you need to add this? repositories.bzl doesn't add the :all target to benchmark and it is built with upstream BUILD.bazel file. #5890

Yeah, @sesmith177 could you followup on this one? Thanks.

Yeah, that's definitely a mistake -- I hadn't noticed that when 7fa5513 removed the benchmark.sh script, it did not add a corresponding cmake_external rule. So when I merged our branch in to fix the Windows issues, this extra rule came with it.

Sorry for the confusion.

Now that envoyproxy#5218 has been merged, we can enable release builds on Windows. This PR updates `ci/do_ci.ps1` to build release + fastbuild versions in addition to debug versions. It also removes support for build scripts in `ci/build_container/build_recipes/` on Windows, as there is no clean way to pass information from Bazel regarding the type of build to those scripts Risk Level: Low Testing: `bazel build //source/... && bazel test //test/...` Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Fred Douglas <fredlas@google.com>

build: enable release builds on Windows

3d97e4b

Signed-off-by: Sam Smith <sesmith177@gmail.com>

mattklein123 assigned htuch Feb 1, 2019

lizan self-assigned this Feb 2, 2019

htuch reviewed Feb 5, 2019

View reviewed changes

sesmith177 added 2 commits February 6, 2019 10:34

add macro

33a0146

Signed-off-by: Sam Smith <sesmith177@gmail.com>

Merge branch 'master' into windows-release

bddecb5

Signed-off-by: Sam Smith <sesmith177@gmail.com>

fix spelling

60aad87

Signed-off-by: Sam Smith <sesmith177@gmail.com>

htuch suggested changes Feb 7, 2019

View reviewed changes

move pdb copy into macro

f06c4e3

Signed-off-by: Sam Smith <sesmith177@gmail.com>

irengrig reviewed Feb 7, 2019

View reviewed changes

htuch reviewed Feb 8, 2019

View reviewed changes

htuch added the waiting label Feb 8, 2019

convert pdb_name + cmake_files_dir to arguments

b882b51

Signed-off-by: Sam Smith <sesmith177@gmail.com>

repokitteh-read-only bot removed the waiting label Feb 8, 2019

htuch approved these changes Feb 8, 2019

View reviewed changes

htuch merged commit 8d89d77 into envoyproxy:master Feb 8, 2019

lizan reviewed Feb 8, 2019

View reviewed changes

Conversation

sesmith177 commented Feb 1, 2019

Uh oh!

sesmith177 commented Feb 1, 2019

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!

sesmith177 commented Feb 6, 2019

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!

sesmith177 commented Feb 7, 2019

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants