build: enable OSX builds by zuercher · Pull Request #1346 · envoyproxy/envoy

zuercher · 2017-07-27T23:43:50Z

Copies in bazel/tools/osx for xcode support. Fixes bugs related to ar and strip flags.

Modifies build-related shell scripts to account for flag differences between GNU and
OSX command line tools.

Disables tcmalloc on OSX and removes various linker flags incompatible linker flags.

Partially fixes #128.

Copies in bazel/tools/osx for xcode support. Fixes bugs related to ar and strip flags. Modifies build-related shell scripts to account for flag differences between GNU and OSX command line tools. Disables tcmalloc on OSX and removes various linker flags incompatible linker flags.

mattklein123

Thanks @zuercher. LGTM. @htuch or @lizan can you take a look?

lizan

@zuercher what should I expect after this PR? sucessful build for deps?

One missing shell script fix is the use of realpath, OSX doesn't have it, if it is needed to be installed, document it.

lizan · 2017-07-28T00:59:32Z

bazel/osx/BUILD

+      -framework Foundation -o $@ $<
+"""
+
+genrule(


Looks like this genrule hasn't been changed from the original one, and original one has public visibility. Can it just be referred from @bazel_tools//tools/osx ?

This combined with the code in #1348 should let you build and test on OSX. By itself it just shouldn't break the linux build, I guess.

I'll try to remove some stuff from bazel/osx again -- I had trouble before that I though was related to sandboxing, but I may have been mistaken.

Will add a note about installing realpath.

I don't think @bazel_tools is defined by Bazel. The only way to do this would be to include the Bazel git tree as a git_repository and then reference these files. I think that would be reasonable to do for unchanged subtrees. Looking below, I think it might make sense to do this.

It is fine if it doesn't work, assuming it is just a temporary workaround until upstream solves it.

@htuch @bazel_tools is definitely defined by bazel itself, try run bazel build @bazel_tools//tools/cpp:stl ;)

Oh, neat. I could have sworn that in the past that didn't work. In that case, I agree with what you say, we should use references to @bazel_tools for the unmodified files (where they don't have modified dependencies).

FWIW, I could revert the change that enables the darwin compiler toolchain and turn that on in the PR with all the other necessary changes to get it compiling under OSX.

I was able to remove most of the copied bazel files. I've left the configure_osx_toolchain invocation in cc_configure.bzl.

htuch

Awesome @zuercher. Thanks for getting this to build. My main feedback is that in places that we do things differently or can't support what's done on Linux, that we document and file tracking issues or add TODOs so we can fix things later if possible.

htuch · 2017-07-28T01:23:00Z

bazel/envoy_build_system.bzl

+# Compute the final linkopts based on various options.
+def envoy_linkopts():
+    return select({
+        "//bazel/osx:darwin": [],


How come we aren't providing pertinent link options here on OS X? Can we still get the appropriate build hash and libraries linked statically? I assume so as tests pass, but I'm confused why we don't need to provide explicit link options.

OS X system libraries are distributed exclusively as dynamically loaded libraries. So, all the thirdparty dependencies (e.g. ci/build_container/build_recipes) are linked statically, but the binary links libc++ and libSystem dynamically. That second library dynamically links libraries for libpthread and libuuid.

As far as build hash goes, the --build-id flag to ld requests create of a ".note.gnu.build-id" ELF note section. Not sure if OS X's Mach-O format supports something similar or not.

I think one could inject a custom section into the mach-o executable by adding a flag to ld. Something like -sectcreate __TEXT __build_id file. And then git_rewrite_sha.py could be modified to update it with the git SHA.

Personally I would just put in a TODO. This is really only useful in a production setting when dealing with core dumps.

Added a TODO.

htuch · 2017-07-28T01:24:06Z

bazel/envoy_build_system.bzl

        repository + "//bazel:dbg_build": ["-ggdb3"],
    }) + select({
        repository + "//bazel:disable_tcmalloc": [],
+        "//bazel/osx:darwin": [],


Is this the permanent plan? Does gperftools support OS X? tcmalloc has some performance advantages in addition to performance insight via the tools.

I cribbed this from @Reflejo's old branch. I believe he encountered problems with tcmalloc deadlocking (c.f. #128 (comment)).

I'm game to try turning it on.

Seems to work, so I put it back in.

htuch · 2017-07-28T01:26:00Z

bazel/osx/BUILD

+      -framework Foundation -o $@ $<
+"""
+
+genrule(


I don't think @bazel_tools is defined by Bazel. The only way to do this would be to include the Bazel git tree as a git_repository and then reference these files. I think that would be reasonable to do for unchanged subtrees. Looking below, I think it might make sense to do this.

htuch · 2017-07-28T01:26:54Z

tools/git_sha_rewriter.py

    print 'Usage: %s <Envoy binary path> ' % sys.argv[0]
    sys.exit(1)
+  if platform.system() == 'Darwin':
+    print 'Stamping not supported on OSX'


What's the limitation here?

At a minimum, OS X binaries are not ELF format, so this particular tool will not work. I don't know enough about Mach-O to say whether it's possible to do something similar.

htuch · 2017-07-28T17:25:36Z

Right, this is Mach-O. I think we don't need this, it's only something we added for Lyft. I'd recommend just changing the message to "not supported with Mach-O" to make clear this is a file format issue, not an OS one.

…

On Fri, Jul 28, 2017 at 1:16 PM Stephan Zuercher ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tools/git_sha_rewriter.py <#1346 (comment)>: > @@ -107,6 +108,9 @@ def RewriteBinary(path, offset, git5_sha1): if len(sys.argv) != 2: print 'Usage: %s <Envoy binary path> ' % sys.argv[0] sys.exit(1) + if platform.system() == 'Darwin': + print 'Stamping not supported on OSX' At a minimum, OS X binaries are not ELF format, so this particular tool will not work. I don't know enough about Mach-O to say whether it's possible to do something similar. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1346 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKaLv_wqiQU__y6RVpoMsNt-ScgZ_m34ks5sShdfgaJpZM4Ol-EM> .

htuch

LGTM modulo small comment request.

htuch · 2017-07-28T22:09:56Z

bazel/envoy_build_system.bzl

+    return select({
+        # TODO(zuercher): build id could be supported via "-sectcreate __TEXT __build_id <file>"
+        # The file could should contain the current git SHA (or enough placeholder data to allow
+        # it to be rewritten by tools/git_sha_rewriter.py).


Can you also include a comment on how static/system libs work on OS X?

htuch · 2017-07-28T22:11:38Z

@zuercher Can you file issues with https://github.com/bazelbuild/bazel/issues for the static link issue if you haven't already and add references to that in this commit? Ideally these get fixes one day upstream and we can unwind our hacks. Here's the issue I filed to track the Linux stuff bazelbuild/bazel#2840.

zuercher · 2017-07-30T04:53:22Z

@htuch I apologize. The ar bug is fixed in upstream bazel. I thought it hadn't because the incorrect flags I was modifing are still there. In 0.5.2, they just don't get used for some reason.

The strip bug has already been reported by someone else as bazelbuild/bazel#209. If not for that bug, we could use bazel 0.5.2's configure_osx_toolchain without modification.

I modified the bazel/osx/crosstool bits to use the 0.5.2 CROSSTOOL.tpl with just my fix for stripping and added diff headers referring to the strip bug and the git tag the files came from.

htuch · 2017-07-30T18:11:32Z

bazel/osx/crosstool/CROSSTOOL.tpl

    tool {
-      tool_path: "wrapped_clang"
-      execution_requirement: "requires-darwin"
+      tool_path: "DUMMY_TOOL"


I'm a bit confused by the introduction of DUMMY_TOOL here - it doesn't appear to be in crosstool.tpl.diff or in the upstream https://github.com/bazelbuild/bazel/blob/master/tools/osx/crosstool/CROSSTOOL.tpl.

I wanted to submit a simple project as a test case for the ar bug, but after I was unable to reproduce it, I realized that the 0.5.2 tag is not bazelbuild/bazel@7b85122, the latter being the SHA from which other stuff was copied.

I think what's happened is that a 0.5.2 release branch was started, and the ar bug was fixed there. Meanwhile in master the DUMMY_TOOL entries were removed (presumably because they weren't being used) and the ar bug was also fixed there, but after 7b85122c. I'm at a loss to explain what actually fixes the bug or why the DUMMY_TOOL entries don't seem to bother anything.

And I should add, I copied this CROSSTOOL.tpl from the 0.5.2 tag, hence the older DUMMY_TOOL entries.

@zuercher I don't know any of the details here, but FYI 0.5.3 is out of that changes anything.

htuch · 2017-07-30T18:14:11Z

@zuercher Sounds good on the upstream story. Could you also leave a comment on bazelbuild/bazel#209 to remind folks this is also affecting Envoy? Thanks.

zuercher · 2017-07-31T19:17:28Z

So, the CROSSTOOL.tpl from 0.5.2 had didn't work with the just-released 0.5.3. However, the 0.5.3 version (plus the strip fix) works in 0.5.2.

I think all the comments are addressed at this point.

Description: #1338 changed argv, but kept argc the same. This PR updates command line args to no longer depend on a hardcoded value. Risk Level: low Testing: local build to make sure all command line args were being respected. Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

) **Description** This completes metrics and tracing and updates docs. So, now /v1/completions is complete! **Related Issues/PRs (if applicable)** Fixes #1231 --------- Signed-off-by: Adrian Cole <adrian@tetrate.io>

mattklein123 previously approved these changes Jul 27, 2017

View reviewed changes

zuercher mentioned this pull request Jul 28, 2017

OSX support #128

Closed

bazel: remove -luuid for now

dae17d3

zuercher dismissed mattklein123’s stale review via dae17d3 July 28, 2017 00:26

fix format

ec046ad

lizan reviewed Jul 28, 2017

View reviewed changes

more fixing

d42a7e4

htuch reviewed Jul 28, 2017

View reviewed changes

add note about realpath

fcceaaa

zuercher added 4 commits July 28, 2017 10:48

tools: git_sha_rewriter.py complains about Mach-O, not OSX

ba3ec74

re-enable tcmalloc for osx

a7e1c18

bazel: remove extraneous osx config

9843560

bazel: add todo re: OS X build id

4b0e6c7

htuch reviewed Jul 28, 2017

View reviewed changes

zuercher added 2 commits July 29, 2017 19:51

not about OSX dynamic libraries

f4d7a29

further osx crosstool cleanup

3d46d4c

htuch reviewed Jul 30, 2017

View reviewed changes

bazel/osx: switch to 0.5.3 version of CROSSTOOL.tpl

4fa4771

htuch approved these changes Jul 31, 2017

View reviewed changes

mattklein123 approved these changes Jul 31, 2017

View reviewed changes

mattklein123 merged commit ef08a87 into envoyproxy:master Jul 31, 2017

zuercher deleted the osx-bazel-support branch August 10, 2017 16:07

Conversation

zuercher commented Jul 27, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

lizan 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!

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!

htuch commented Jul 28, 2017 via email

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Jul 28, 2017

Uh oh!

zuercher commented Jul 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 commented Jul 30, 2017

Uh oh!

zuercher commented Jul 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

zuercher commented Jul 30, 2017 •

edited

Loading