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

Conversation

thisiscam
Copy link

Hi,

I would like to share my progress in trying to merge the latest master into the macOS build #24 developed by @lifeiteng.

The merge went smoothly; however, there were a few places that I needed to fix/hack, in order to build successfully.
I will comment in my commit below for the details about the hacks.
Hopefully, these will be somewhat useful info for folks to merge #24 into master.

Currently this fails 10 tests, all of them of the form:

INFO: From Testing //reverb/cc:trajectory_writer_test:
==================== Test output for //reverb/cc:trajectory_writer_test:
dyld: lazy symbol binding failed: Symbol not found: __ZN10tensorflow15TensorShapeBaseINS_18PartialTensorShapeEEC2EN4absl12lts_202103244SpanIKxEE
  Referenced from: /private/var/tmp/_bazel_cyang/b1d79ebef38c9591112b3498fd6e8dab/sandbox/darwin-sandbox/1353/execroot/reverb/bazel-out/darwin-opt/bin/reverb/cc/trajectory_writer_test.runfiles/reverb/reverb/cc/trajectory_writer_test
  Expected in: flat namespace

dyld: Symbol not found: __ZN10tensorflow15TensorShapeBaseINS_18PartialTensorShapeEEC2EN4absl12lts_202103244SpanIKxEE
  Referenced from: /private/var/tmp/_bazel_cyang/b1d79ebef38c9591112b3498fd6e8dab/sandbox/darwin-sandbox/1353/execroot/reverb/bazel-out/darwin-opt/bin/reverb/cc/trajectory_writer_test.runfiles/reverb/reverb/cc/trajectory_writer_test
  Expected in: flat namespace

-- which I assume are caused by incompatible TensorFlow library versions.

@google-cla
Copy link

google-cla bot commented May 5, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@thisiscam thisiscam changed the title [DO NOT MERGE] Attempts/Info for merging master into macOS build [DO NOT MERGE] Attempt and info for merging master into macOS build May 5, 2021
@@ -23,3 +23,5 @@ 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.

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).

@@ -103,7 +103,7 @@ def _find_python_solib_path(repo_ctx):
.format(exec_result.stderr))

if is_darwin(repo_ctx):
basename = "lib{}m.dylib".format(version)
Copy link
Author

Choose a reason for hiding this comment

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

@thisiscam
Copy link
Author

@googlebot I consent

@google-cla
Copy link

google-cla bot commented May 5, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: yes label Nov 8, 2021
@wookayin
Copy link

wookayin commented Dec 24, 2021

The lib -> libxxx changes trick was clever to address the following conflicts (ActionConflictException):

(excerpt of log)

ERROR: com.google.devtools.build.lib.skyframe.ArtifactConflictFinder$ConflictException:
 com.google.devtools.build.lib.actions.MutableActionGraph$ActionConflictException:
 for reverb/cc/checkpointing/libcheckpoint_cc_proto.so,
  previous action: action 'Linking reverb/cc/checkpointing/libcheckpoint_cc_proto.so',
  attempted action: action 'Reporting failed target //reverb/cc/checkpointing:checkpoint_cc_proto located at .../reverb/reverb/cc/checkpointing/BUILD:13:24'

The error you are experiencing (Symbol not found) is probably due to the version mismatch between reverb and tensorflow. You should use tf-nightly or any recent versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants