Skip to content
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.

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

build: fix gperftools build on OS X #1504

Merged
merged 4 commits into from
Aug 21, 2017

Conversation

zuercher
Copy link
Member

gperftools-2.6.1 doesn't build on OS X without adding -D_XOPEN_SOURCE=1 to enable some deprecated libc code.

if [[ `uname` == "Darwin" ]];
then
# enable ucontext(3)
export CPPFLAGS="-D_XOPEN_SOURCE=1"
Copy link
Contributor

Choose a reason for hiding this comment

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

1 is not valid _XOPEN_SOURCE value, please use 700.

Copy link
Member

Choose a reason for hiding this comment

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

Also I think you want CPPFLAGS="$CPPFLAGS -D_XOPEN_SOURCE=700"

Copy link
Member Author

Choose a reason for hiding this comment

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

For _XOPEN_SOURCE=700 or _XOPEN_SOURCE=600, gperftools fails to compile because getpagesize is not defined in the version of _POSIX_C_SOURCE that those definitions imply.

getpagesize is defined when _XOPEN_SOURCE=500, but syscall isn't. However, defining _DARWIN_C_SOURCE brings syscall back.

@PiotrSikora
Copy link
Contributor

Actually, looking into this a bit closer, <ucontext.h> is only included in the benchmark tool (file: benchmark/getcontext_light.cc), which we don't use.

The library correctly includes <sys/ucontext.h>, so it doesn't require extra magic on macOS:

#if defined(HAVE_SYS_UCONTEXT_H)
#include <sys/ucontext.h>
#elif defined(HAVE_UCONTEXT_H)
#include <ucontext.h>
#elif defined(HAVE_CYGWIN_SIGNAL_H)
#include <cygwin/signal.h>
typedef ucontext ucontext_t;
#else
typedef int ucontext_t;   // just to quiet the compiler, mostly
#endif

Perhaps we could just build the library itself? It's a bit hacky, but:

make V=1 tcmalloc_and_profiler_unittest

seems to work just fine.

@zuercher
Copy link
Member Author

We use make install to get the headers into $THIRDPARTY_BUILD/include and that target depends on building everything (even though the test that uses the benchmark library is not installed).

The other alternative is probably to remove support for it in the Mac build altogether. I think that's basically disabling tcmalloc and skipping this build recipe.

@mattklein123
Copy link
Member

I think this is fine. @PiotrSikora any particular reason to not just do this if it works?

@zuercher
Copy link
Member Author

FWIW: gperftools/gperftools#910

@PiotrSikora
Copy link
Contributor

@mattklein123 I'm just worried that Envoy and gperftools might pull different struct definitions, because of different _XOPEN_SOURCE and _DARWIN_C_SOURCE defines, but I'm not sure if that's a valid concern. I think it's fine to do it for now.

@mattklein123
Copy link
Member

I really know nothing about this. It seems reasonable to me for now. @zuercher perhaps can you put in a comment in that script with a TODO which links to the gperftools bug so maybe we can clean this up at some point or let people know why this is being set and what it is working around?

@mattklein123 mattklein123 merged commit bf9345d into envoyproxy:master Aug 21, 2017
@zuercher zuercher deleted the fix-macos-gperf-build branch August 21, 2017 22:55
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Description: This PR modifies the build recipe for `envoy_engine.so` to hide all symbols except for `_PyInit_envoy_engine` by default on maOS, so that envoy_engine can be loaded alongside a [protobuf](http://pypi.org/project/protobuf) wheel without breaking.

Loading in protobuf alongside `envoy_engine.so` would register two sets of the same symbols and, on macOS, the runtime linker chooses the first symbol it finds causing problems. See the [similar protobuf change](protocolbuffers/protobuf#8346) and its [sister change in grpc](grpc/grpc#24992) for more information on what's going on.

Risk Level: Low
Testing: See the now-closed #1504 for how I've been testing this.
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Cerek Hillen <[email protected]>
Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Description: This PR modifies the build recipe for `envoy_engine.so` to hide all symbols except for `_PyInit_envoy_engine` by default on maOS, so that envoy_engine can be loaded alongside a [protobuf](http://pypi.org/project/protobuf) wheel without breaking.

Loading in protobuf alongside `envoy_engine.so` would register two sets of the same symbols and, on macOS, the runtime linker chooses the first symbol it finds causing problems. See the [similar protobuf change](protocolbuffers/protobuf#8346) and its [sister change in grpc](grpc/grpc#24992) for more information on what's going on.

Risk Level: Low
Testing: See the now-closed #1504 for how I've been testing this.
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Cerek Hillen <[email protected]>
Signed-off-by: JP Simard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants