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

Building 2.1 from source on Mac (10.15.5) #361

Closed
scbash opened this issue Aug 14, 2020 · 2 comments · Fixed by #479
Closed

Building 2.1 from source on Mac (10.15.5) #361

scbash opened this issue Aug 14, 2020 · 2 comments · Fixed by #479

Comments

@scbash
Copy link

scbash commented Aug 14, 2020

While tracking down a bug in Tensorflow, yesterday I had to build Tensorflow Text from source (2.1 branch, happened to be at the 2.1.1 tag) and wanted to report a few surprises. These could be split into separate bugs, but since I think they're relatively minor I'm including them all in one report. My environment is MacOS 10.15.5, Homebrew Python 3.7.8 (in venv), and Bazelisk (bazel 3.4.1).

  1. I had to add patch_tool="patch" (switching to system patch rather than Bazel's built-in algorithm) in the ICU section of WORKSPACE to successfully build ICU. Bazel's built-in function complained "the chunk content doesn't match the target". AFAICT they did match, including whitespace and line endings, so I'm not sure what went wrong. Full error message:
ERROR: An error occurred during the fetch of repository 'icu':
   Traceback (most recent call last):
	File "/private/var/tmp/_bazel_bashs011/0e925819d1e8f6af831f3495cf59cf53/external/bazel_tools/tools/build_defs/repo/http.bzl", line 121
		patch(ctx)
	File "/private/var/tmp/_bazel_bashs011/0e925819d1e8f6af831f3495cf59cf53/external/bazel_tools/tools/build_defs/repo/utils.bzl", line 135, in patch
		ctx.patch(patchfile, strip)
com.google.devtools.build.lib.syntax.EvalException: Error applying patch /Users/bashs011/Development/cca_parser/tf-text/third_party/icu/udata.patch: Incorrect Chunk: the chunk content doesn't match the target
**Original Position**: 55

**Original Content**:
#include "uconfig_local.h"
#endif

/**
 * \def U_DEBUG
 * Determines whether to include debugging code.
-- a/icu4c/source/common/udata.cpp

**Revised Content**:
#include "uconfig_local.h"
#endif

// Tensorflow is statically linked on all platforms.
#ifndef U_STATIC_IMPLEMENTATION
#define U_STATIC_IMPLEMENTATION
#endif

/**
 * \def U_DEBUG
 * Determines whether to include debugging code.
  1. I had to install homebrew coreutils for greadlink. I'm actually a little surprised I didn't already have coreutils on the machine, but it's probably worth mentioning in the build instructions. From run_build.sh:
Target //oss_scripts/pip_package:build_pip_package up-to-date:
  bazel-bin/oss_scripts/pip_package/build_pip_package
INFO: Elapsed time: 260.035s, Critical Path: 152.70s
INFO: 696 processes: 696 local.
INFO: Build completed successfully, 699 total actions
+ ./bazel-bin/oss_scripts/pip_package/build_pip_package .
Darwin
./bazel-bin/oss_scripts/pip_package/build_pip_package: line 32: greadlink: command not found
  1. My Mac is still running a case insensitive file system, so docs/api_docs/python/text/WordShape.md conflicts with wordshape.md. I was able to ignore the problem for my needs, but it can be annoying when switching branches because Git thinks the contents of one of the two files has changed (after they bash each other on checkout).
@broken
Copy link
Member

broken commented Aug 21, 2020

Thanks; I'll respond individually.

  1. We noticed this as well. Bazel had changed to default to their own patch tool rather than using the system one. Passing the "-s" patch argument is supposed to have the side-effect of forcing Bazel to using the system one though. I'm surprised this did not work for you. This was actually what that team had recommended us to do when we originally reached out about the issue. However, using the patch_tool argument does seem a lot less hacky. (I wonder if this was available at the time..)

  2. I'll add the coreutils dependency to the build instructions.

  3. This is a known problem, and as one of the first ops added, I think there is some regret having the class and op named so similarly. While we want to fix this, it's considered low priority since it's not causing development problems, and since we haven't written the tool that generates the docs, nobody has had the time to look into fixing it.

tf-text-github-robot pushed a commit that referenced this issue Dec 21, 2020
…em is on MacOS (and maybe Windows) this filename collides with wordshape.md, because the filesystem does not differentiate cases for the files. This is purely a QOL change for anybody checking out the library on a non-Linux platform. Fix #361.

PiperOrigin-RevId: 348496513
tf-text-github-robot pushed a commit that referenced this issue Dec 21, 2020
…em is on MacOS (and maybe Windows) this filename collides with wordshape.md, because the filesystem does not differentiate cases for the files. This is purely a QOL change for anybody checking out the library on a non-Linux platform. Fix #361.

PiperOrigin-RevId: 348508297
@broken
Copy link
Member

broken commented Dec 21, 2020

  1. With the Windows release in v2.4, we've updated the patch files to work with Bazel's internal patcher and removed the "-s" patch arguments.

  2. This was added to the build instructions soon after this was originally reported.

  3. With pr Rename documentation file WordShape.md to WordShape_cls.md. The problem is on MacOS (and maybe Windows) this filename collides with wordshape.md, because the filesystem does not differentiate cases for the files. This is purely a QOL change for anybody checking out the library on a non-Linux platform. Fix #361. #479, we've renamed one of these files, so there should no longer be any collisions.

@broken broken closed this as completed Dec 21, 2020
broken added a commit that referenced this issue Dec 22, 2020
…em is on MacOS (and maybe Windows) this filename collides with wordshape.md, because the filesystem does not differentiate cases for the files. This is purely a QOL change for anybody checking out the library on a non-Linux platform. Fix #361.

PiperOrigin-RevId: 348508297
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment