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

[DO NOT MERGE] Attempt and info for merging master into macOS build #39

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft
3 changes: 2 additions & 1 deletion .bazelrc
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,12 @@ build --cxxopt="-std=c++14"
build --cxxopt="-D_GLIBCXX_USE_CXX11_ABI=0"
build --auto_output_filter=subpackages
build --copt="-Wall" --copt="-Wno-sign-compare"
build --linkopt="-lrt -lm"

# TF isn't built in dbg mode, so our dbg builds will segfault due to inconsistency
# of defines when using tf's headers. In particular in refcount.h.
build --cxxopt="-DNDEBUG"

# Options from ./configure
try-import %workspace%/.reverb.bazelrc

build --features=-supports_dynamic_linker
Copy link
Author

Choose a reason for hiding this comment

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

This was necessary due to bazelbuild/bazel#4341.
I also had to use bazel==4.1.0rc04 -- I tried 4.0.0 (the latest homebrew version) and that didn't work for me for some reason.

7 changes: 7 additions & 0 deletions configure.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,13 @@ def main():
reset_configure_bazelrc()
setup_python(environ_cp)

write_to_bazelrc('')
if sys.platform == 'darwin':
write_to_bazelrc('# https://github.com/googleapis/google-cloud-cpp-spanner/issues/1003')
write_to_bazelrc('build --copt=-DGRPC_BAZEL_BUILD')
else:
write_to_bazelrc('build --linkopt="-lrt -lm"')


def get_from_env_or_user_or_default(environ_cp, var_name, ask_for_var,
var_default):
Expand Down
1 change: 1 addition & 0 deletions docker/dev.dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ ARG pip_dependencies=' \
numpy \
oauth2client \
pandas \
platform \
portpicker'


Expand Down
1 change: 1 addition & 0 deletions docker/release.dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ ARG pip_dependencies=' \
numpy \
oauth2client \
pandas \
platform \
portpicker'

# TODO(b/154930404): Update to 2.2.0 once it's out. May need to
Expand Down
27 changes: 19 additions & 8 deletions oss_build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -89,18 +89,29 @@ for python_version in $PYTHON_VERSIONS; do
fi

if [ "$python_version" = "3.6" ]; then
export PYTHON_BIN_PATH=/usr/bin/python3.6 && export PYTHON_LIB_PATH=/usr/local/lib/python3.6/dist-packages
ABI=cp36
elif [ "$python_version" = "3.7" ]; then
export PYTHON_BIN_PATH=/usr/local/bin/python3.7 && export PYTHON_LIB_PATH=/usr/local/lib/python3.7/dist-packages
ABI=cp37
elif [ "$python_version" = "3.8" ]; then
export PYTHON_BIN_PATH=/usr/bin/python3.8 && export PYTHON_LIB_PATH=/usr/local/lib/python3.8/dist-packages
ABI=cp38
else
echo "Error unknown --python. Only [3.6|3.7|3.8]"
exit 1
fi

export PYTHON_BIN_PATH=`which python${python_version}`
export PYTHON_LIB_PATH=`${PYTHON_BIN_PATH} -c 'import site; print(site.getsitepackages()[0])'`

if [ "$(uname)" = "Darwin" ]; then
bazel_config=""
version=`sw_vers -productVersion | sed 's/\./_/g' | cut -d"_" -f1,2`
PLATFORM="macosx_${version}_"`uname -m`
else
bazel_config="--config=manylinux2010"
bazel_config=""
PLATFORM="manylinux2010_x86_64"
fi

# Configures Bazel environment for selected Python version.
$PYTHON_BIN_PATH configure.py

Expand All @@ -111,20 +122,20 @@ for python_version in $PYTHON_VERSIONS; do
# someone's system unexpectedly. We are executing the python tests after
# installing the final package making this approach satisfactory.
# TODO(b/157223742): Execute Python tests as well.
bazel test -c opt --copt=-mavx --config=manylinux2010 --test_output=errors //reverb/cc/...
bazel test -c opt --copt=-mavx ${bazel_config} --test_output=errors //reverb/cc/...

# Builds Reverb and creates the wheel package.
bazel build -c opt --copt=-mavx --config=manylinux2010 reverb/pip_package:build_pip_package
./bazel-bin/reverb/pip_package/build_pip_package --dst $OUTPUT_DIR $PIP_PKG_EXTRA_ARGS
bazel build -c opt --copt=-mavx ${bazel_config} reverb/pip_package:build_pip_package
./bazel-bin/reverb/pip_package/build_pip_package --dst $OUTPUT_DIR $PIP_PKG_EXTRA_ARGS --platform "${PLATFORM}"

# Installs pip package.
$PYTHON_BIN_PATH -mpip install ${OUTPUT_DIR}*${ABI}*.whl
$PYTHON_BIN_PATH -mpip install --force-reinstall ${OUTPUT_DIR}*${ABI}*.whl

if [ "$PYTHON_TESTS" = "true" ]; then
echo "Run Python tests..."
set +e

bash run_python_tests.sh |& tee ./unittest_log.txt
bash run_python_tests.sh 2>&1 | tee ./unittest_log.txt
UNIT_TEST_ERROR_CODE=$?
set -e
if [[ $UNIT_TEST_ERROR_CODE != 0 ]]; then
Expand Down
9 changes: 9 additions & 0 deletions reverb/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,15 @@ licenses(["notice"])

exports_files(["LICENSE"])

config_setting(
name = "macos",
values = {
"apple_platform_type": "macos",
"cpu": "darwin",
},
visibility = ["//visibility:public"],
)

reverb_pytype_strict_library(
name = "reverb",
srcs = ["__init__.py"],
Expand Down
46 changes: 34 additions & 12 deletions reverb/cc/platform/default/build_rules.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -107,20 +107,20 @@ def reverb_cc_proto_library(name, srcs = [], deps = [], **kwargs):
name = "{}_static".format(name),
srcs = gen_srcs,
hdrs = gen_hdrs,
deps = depset(deps + reverb_tf_deps()),
deps = depset([dep.replace(":", ":libxxx") + ".so" for dep in deps] + reverb_tf_deps()),
alwayslink = 1,
**kwargs
)
native.cc_binary(
name = "lib{}.so".format(name),
name = "libxxx{}.so".format(name),
deps = ["{}_static".format(name)],
linkshared = 1,
**kwargs
)
native.cc_library(
name = name,
hdrs = gen_hdrs,
srcs = ["lib{}.so".format(name)],
Copy link
Author

Choose a reason for hiding this comment

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

These lib -> libxxx changes was necessary to let my Bazel not complain about conflicting names.
Inspired by bazelbuild/bazel#4341 (comment).

srcs = ["libxxx{}.so".format(name)],
deps = depset(deps + reverb_tf_deps()),
alwayslink = 1,
**kwargs
Expand Down Expand Up @@ -294,6 +294,16 @@ def reverb_gen_op_wrapper_py(name, out, kernel_lib, linkopts = [], **kwargs):
fail("Argument out must end with '.py', but saw: {}".format(out))

module_name = "lib{}_gen_op".format(name)
exported_symbols_file = "%s-exported-symbols.lds" % module_name
# gen_client_ops -> reverb_client
symbol = "reverb_{}".format(name.split('_')[1])
native.genrule(
name = module_name + "_exported_symbols",
outs = [exported_symbols_file],
cmd = "echo '*%s*' >$@" % symbol,
output_licenses = ["unencumbered"],
visibility = ["//visibility:private"],
)
version_script_file = "%s-version-script.lds" % module_name
native.genrule(
name = module_name + "_version_script",
Expand All @@ -304,16 +314,23 @@ def reverb_gen_op_wrapper_py(name, out, kernel_lib, linkopts = [], **kwargs):
)
native.cc_binary(
name = "{}.so".format(module_name),
deps = [kernel_lib] + reverb_tf_deps() + [version_script_file],
deps = [kernel_lib] + reverb_tf_deps() + [
exported_symbols_file,
version_script_file
],
copts = tf_copts() + [
"-fno-strict-aliasing", # allow a wider range of code [aliasing] to compile.
"-fvisibility=hidden", # avoid symbol clashes between DSOs.
],
linkshared = 1,
linkopts = linkopts + _rpath_linkopts(module_name) + [
"-Wl,--version-script",
"$(location %s)" % version_script_file,
],
linkopts = linkopts + _rpath_linkopts(module_name) + select({
"//reverb:macos": [
"-Wl,-exported_symbols_list,$(location %s)" % exported_symbols_file,
],
"//conditions:default": [
"-Wl,--version-script,$(location %s)" % version_script_file,
],
}),
**kwargs
)
native.genrule(
Expand Down Expand Up @@ -444,10 +461,15 @@ def reverb_pybind_extension(
"-fexceptions", # pybind relies on exceptions, required to compile.
"-fvisibility=hidden", # avoid pybind symbol clashes between DSOs.
],
linkopts = linkopts + _rpath_linkopts(module_name) + [
"-Wl,--version-script",
"$(location %s)" % version_script_file,
],
linkopts = linkopts + _rpath_linkopts(module_name) +
select({"//reverb:macos": [
"-Wl,-exported_symbols_list,$(location %s)" % exported_symbols_file,
],
"//conditions:default": [
"-Wl,--version-script",
"$(location %s)" % version_script_file,
],
}),
deps = depset(deps + [
exported_symbols_file,
version_script_file,
Expand Down
43 changes: 35 additions & 8 deletions reverb/cc/platform/default/repo.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

def is_darwin(ctx):
if ctx.os.name.lower().find("mac") != -1:
return True
return False

# Sanitize a dependency so that it works correctly from code that includes
# reverb as a submodule.
def clean_dep(dep):
Expand Down Expand Up @@ -89,15 +94,21 @@ def _find_python_solib_path(repo_ctx):
fail("Could not locate python shared library path:\n{}"
.format(exec_result.stderr))
version = exec_result.stdout.splitlines()[-1]
basename = "lib{}.so".format(version)
exec_result = repo_ctx.execute(
["{}-config".format(version), "--configdir"],
quiet = True,
)
if exec_result.return_code != 0:
fail("Could not locate python shared library path:\n{}"
.format(exec_result.stderr))
solib_dir = exec_result.stdout.splitlines()[-1]

if is_darwin(repo_ctx):
basename = "lib{}.dylib".format(version)
solib_dir = "/".join(exec_result.stdout.splitlines()[-1].split("/")[:-2])
else:
basename = "lib{}.so".format(version)
solib_dir = exec_result.stdout.splitlines()[-1]

full_path = repo_ctx.path("{}/{}".format(solib_dir, basename))
if not full_path.exists:
fail("Unable to find python shared library file:\n{}/{}"
Expand Down Expand Up @@ -220,17 +231,21 @@ filegroup(
def _tensorflow_solib_repo_impl(repo_ctx):
tf_lib_path = _find_tf_lib_path(repo_ctx)
repo_ctx.symlink(tf_lib_path, "tensorflow_solib")
if is_darwin(repo_ctx):
suffix = "2.dylib"
else:
suffix = "so.2"

repo_ctx.file(
"BUILD",
content = """
cc_library(
name = "framework_lib",
srcs = ["tensorflow_solib/libtensorflow_framework.so.2"],
srcs = ["tensorflow_solib/libtensorflow_framework.{suffix}"],
deps = ["@python_includes", "@python_includes//:numpy_includes"],
visibility = ["//visibility:public"],
)
""",
)
""".format(suffix=suffix))

def _python_includes_repo_impl(repo_ctx):
python_include_path = _find_python_include_path(repo_ctx)
Expand All @@ -243,6 +258,12 @@ def _python_includes_repo_impl(repo_ctx):
python_solib.basename,
)

if is_darwin(repo_ctx):
# Fix Fatal Python error: PyThreadState_Get: no current thread
python_includes_srcs = ""
else:
python_includes_srcs = 'srcs = ["%s"],' % python_solib.basename

# Note, "@python_includes" is a misnomer since we include the
# libpythonX.Y.so in the srcs, so we can get access to python's various
# symbols at link time.
Expand All @@ -252,7 +273,7 @@ def _python_includes_repo_impl(repo_ctx):
cc_library(
name = "python_includes",
hdrs = glob(["python_includes/**/*.h"]),
srcs = ["{}"],
{srcs}
includes = ["python_includes"],
visibility = ["//visibility:public"],
)
Expand All @@ -262,7 +283,7 @@ cc_library(
includes = ["numpy_includes"],
visibility = ["//visibility:public"],
)
""".format(python_solib.basename),
""".format(srcs=python_includes_srcs),
executable = False,
)

Expand Down Expand Up @@ -330,8 +351,14 @@ def _reverb_protoc_archive(ctx):
sha256 = ""
version = override_version

if is_darwin(ctx):
platform = "osx"
sha256 = ""
else:
platform = "linux"

urls = [
"https://github.com/protocolbuffers/protobuf/releases/download/v%s/protoc-%s-linux-x86_64.zip" % (version, version),
"https://github.com/protocolbuffers/protobuf/releases/download/v%s/protoc-%s-%s-x86_64.zip" % (version, version, platform),
]
ctx.download_and_extract(
url = urls,
Expand Down
21 changes: 20 additions & 1 deletion reverb/pip_package/build_pip_package.sh
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ function build_wheel() {
pushd ${TMPDIR} > /dev/null

echo $(date) : "=== Building wheel"
"${PYTHON_BIN_PATH}" setup.py bdist_wheel ${PKG_NAME_FLAG} ${RELEASE_FLAG} ${TF_VERSION_FLAG} --plat manylinux2010_x86_64 > /dev/null
"${PYTHON_BIN_PATH}" setup.py bdist_wheel ${PKG_NAME_FLAG} ${RELEASE_FLAG} ${TF_VERSION_FLAG} --plat ${PLATFORM} > /dev/null
DEST=${TMPDIR}/dist/
if [[ ! "$TMPDIR" -ef "$DESTDIR" ]]; then
mkdir -p ${DESTDIR}
Expand Down Expand Up @@ -71,6 +71,19 @@ function prepare_src() {
# must remain where they are for TF to find them.
find "${TMPDIR}/reverb/cc" -type d -name ops -prune -o -name '*.so' \
-exec mv {} "${TMPDIR}/reverb" \;

# Copy darwin libs over so they can be loaded at runtime
so_lib_dir=$(ls $RUNFILES | grep solib) || true
if [ -n "${so_lib_dir}" ]; then
mkdir -p "${TMPDIR}/${so_lib_dir}"
proto_so_dir=$(ls ${RUNFILES}/${so_lib_dir} | grep proto) || true
for dir in ${proto_so_dir}; do
echo "===== DIR = $dir"
cp -R ${RUNFILES}/${so_lib_dir}/${dir} "${TMPDIR}/${so_lib_dir}"
done

cp -r $TMPDIR/${so_lib_dir} `dirname $PYTHON_LIB_PATH`
fi
}

function usage() {
Expand All @@ -80,6 +93,7 @@ function usage() {
echo " --release build a release version"
echo " --dst path to copy the .whl into."
echo " --tf-version tensorflow version dependency passed to setup.py."
echo " --plat platform."
echo ""
exit 1
}
Expand All @@ -91,6 +105,8 @@ function main() {
# This is where the source code is copied and where the whl will be built.
DST_DIR=""

PLATFORM="manylinux2010_x86_64"

while true; do
if [[ "$1" == "--help" ]]; then
usage
Expand All @@ -103,6 +119,9 @@ function main() {
elif [[ "$1" == "--tf-version" ]]; then
shift
TF_VERSION_FLAG="--tf-version $1"
elif [[ "$1" == "--platform" ]]; then
shift
PLATFORM=$1
fi

if [[ -z "$1" ]]; then
Expand Down
13 changes: 13 additions & 0 deletions reverb/pip_package/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,16 @@ def run_setup(self):
long_description = f.read()

version, project_name = self._get_version()

so_lib_paths = [
i for i in os.listdir('.')
if os.path.isdir(i) and fnmatch.fnmatch(i, '_solib_*')
]

matches = []
for path in so_lib_paths:
matches.extend(['../' + x for x in find_files('*', path) if '.py' not in x])

setup(
name=project_name,
version=version,
Expand All @@ -126,6 +136,9 @@ def run_setup(self):
packages=find_packages(),
headers=list(find_files('*.proto', 'reverb')),
include_package_data=True,
package_data={
'reverb': matches,
},
install_requires=self._get_required_packages(),
extras_require={
'tensorflow': self._get_tensorflow_packages(),
Expand Down