FreeBSD compatibility #7073

Merged
merged 11 commits into from Feb 15, 2017

Projects

None yet

9 participants

@blodan
Contributor
blodan commented Jan 25, 2017

Two commits

One with non breaking changes which should be able to be applied straight away
One with breaking changes as FreeBSD has libdl integrated into libc (so the -ldl linker option should probably be handled in another way but I have no idea how)

@tensorflow-jenkins
Collaborator

Can one of the admins verify this patch?

@googlebot

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

๐Ÿ“ Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.
@googlebot googlebot added the cla: no label Jan 25, 2017
@blodan
Contributor
blodan commented Jan 25, 2017

I've signed the CLA.

@googlebot

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@blodan
Contributor
blodan commented Jan 25, 2017

I've added the e-mail I commited with as alternate e-mail and verified it for my account as described in the contact info section so the CLA should be good now.

@googlebot

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Jan 25, 2017
@blodan blodan Adding FreeBSD compatibility (BREAKING changes)
 - FreeBSD has libdl integrated into libc so -ldl is not available
8102388
@hholst80
Contributor

@blodan also GPU will work with this patch?

@gunan
Member
gunan commented Jan 27, 2017

By breaking change, do you mean breaking for existing configurations?
Instead of those, we can conditionally set flags with the help of such macros:
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/tensorflow.bzl#L79

@blodan
Contributor
blodan commented Jan 27, 2017

@hholst80 i have not tried GPU mode, i know there is a binary nvidia driver for freebsd, but i don't know if the cudatoolkit is available on bsd

@blodan
Contributor
blodan commented Jan 27, 2017

@gunan yes, as the "breaking change" commit removes the "-ldl" link option everywhere compilation would probably break for linux

How would i go about writing such a macro for this line for example?
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/BUILD#L1171

If you could give me an example for that one I can fix it for the rest of the -ldl places.

@gunan
Member
gunan commented Jan 27, 2017

https://github.com/tensorflow/tensorflow/blob/master/tensorflow/tensorflow.bzl#L94
Here is how most of our copts should be set. As you see, based on different OS and toolchains, we select different options.
The config settings that pick what to build are defined here:
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/BUILD#L47

We probably need a new config setting for freebsd.
@damienmg @meteorcloudy How can we setup a new config setting for freeBSD in tensorflow BUILD files?

@gunan
Member
gunan commented Jan 28, 2017

OK, after reading a bit more:
once we add a new config value there, then we will need to set it from the command line when compiling for freeBSD.
So let's do this:

in tensorflow/BUILD, we need a new config setting.

config_setting(
  name = "freeBSD",
  values = {
    "define": "os=freeBSD"
  },
)

Then in this file https://github.com/tensorflow/tensorflow/blob/master/tensorflow/tensorflow.bzl#L94
We can conditionally unset the linker flags.

Then, when running bazel, you will need to add the flag --define os=freeBSD.

@blodan Could you try the above out?

@@ -1150,7 +1150,7 @@ cc_library(
],
copts = tf_copts(),
defines = tf_additional_lib_defines(),
- linkopts = ["-ldl"],
@jart
jart Jan 29, 2017 Member

Would it be possible to have a config_setting() rule and a select() function to selectively apply this change to only freebsd? This might work, but I'm not sure.

config_setting(
    name = "freebsd",
    values = {"cpu": "freebsd"},
)

See also: https://github.com/tensorflow/tensorflow/blob/master/third_party/jpeg/jpeg.BUILD#L463

@rmlarsen rmlarsen assigned rmlarsen and jart and unassigned rmlarsen Jan 30, 2017
third_party/gpus/cuda_configure.bzl
@@ -374,6 +374,13 @@ def _lib_name(lib, cpu_value, version="", static=False):
if version:
version = ".%s" % version
return "lib%s.so%s" % (lib, version)
+ elif cpu_value == "FreeBSD":
@jart
jart Jan 31, 2017 Member

This appears to be the same as the previous clause. Couldn't you say cpu_value in ("Linux", "FreeBSD")?

tensorflow/stream_executor/BUILD
@@ -25,7 +25,6 @@ cc_library(
"platform/**/*.h",
]),
linkopts = [
- "-ldl",
@jart
jart Jan 31, 2017 Member

Have you investigated why this was put here in the first place? We want to preserve support for other platforms.

@blodan
Contributor
blodan commented Jan 31, 2017 edited

@gunan @jart I'm trying to get this to work with your pointers but it doesn't pick up my settings I'm trying to implement.

First off I've defined

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

in tensorflow/BULD

Then I tried this on line 1153 in tensorflow/core/BUILD:

linkopts = select({
"//tensorflow:FreeBSD": [],
"//conditions:default": ["-ldl"],
})

However, when trying to build it still adds -ldl to the compile options, shouldn't the above "clear" the settings for FreeBSD?

@blodan
Contributor
blodan commented Jan 31, 2017

Figured out that

values = {"cpu": "freebsd"},

had to be in lowercase, just going to build this on a fresh freebsd system to see that it actually works before commiting the changes.

blodan added some commits Jan 31, 2017
@blodan blodan - Define FreeBSD config
- Clear -ldl linkopts for FreeBSD only
0944645
@blodan blodan Clear -ldl linkopts for FreeBSD only
1978209
@blodan
Contributor
blodan commented Jan 31, 2017

Okay so I've commited the changes now and this should be good to go if you don't want me to do any other changes @gunan @jart

@rmlarsen
Member

@tensorflow-jenkins test this please

@damienmg
Member
damienmg commented Feb 1, 2017

Sorry I was OOO. /cc @aehlig who made Bazel FreeBSD port.

I had only one comment but I see that @jart already gave the same :)

@blodan blodan Merge branch 'master' into master
65b8732
@rmlarsen
Member
rmlarsen commented Feb 1, 2017

@tensorflow-jenkins test this please

@rmlarsen
Member
rmlarsen commented Feb 2, 2017

@blodan it looks like the build files are still not quite there. Can you try to fix the problem?

@blodan
Contributor
blodan commented Feb 2, 2017

@rmlarsen from reading the output of jenkins it seems there was a failure during download/fetching some kind of repository? I don't think thats related to what I've changed.

https://ci.tensorflow.org/job/tensorflow-pull-requests-sanity/2936/console

Or am I looking at the wrong output?

@gunan
Member
gunan commented Feb 2, 2017

Jenkins, test this please.

@gunan
Member
gunan commented Feb 2, 2017

https://ci.tensorflow.org/job/tensorflow-pull-requests-sanity/2967/console

It failed again.
Bazel is not great at rreporting errors, but this usually means there is an error in BUILD of .bzl files.
Could you test it out locally (or on docker)? Simply running configure should trigger the issue.

@damienmg
Member
damienmg commented Feb 3, 2017

Hi, I am interested to know what generate that stacktrace, Bazel should definitely never print a stack trace (means a bug to us)

@damienmg
Member
damienmg commented Feb 3, 2017

Which version of Bazel TensorFlow CI is using? 0.4.3? I cannot repro the stack trace. However the error message I get is not really helpful either, it tries to load @local_config_cuda//cuda:cudart_static but cannot find @local_config_cuda//cuda build file. Looking at $(bazel help output_base)/external/local_config_cuda/cuda I can find a build file...

@damienmg

This is the only issue on my side.

third_party/gpus/cuda/BUILD.tpl
- linkopts = [
- "-ldl",
+ linkopts = select({
+ "//tensorflow:FreeBSD": [],
@damienmg
damienmg Feb 3, 2017 Member

This won't find //tensorflow:FreeBSD...

Either the TF team decide to switch to bazel 0.4.4 and you can just add @org_tensorflow in front of the label or you have to work around either by injecting the workspace name from the template generation.

@damienmg
Member
damienmg commented Feb 3, 2017

Ok the problem is that it is looking for @local_config_cuda//tensorflow:FreeBSD which does not exists. The error message I get with Bazel 0.4.4 is definitely misleading:

.........
ERROR: /usr/local/google/home/dmarting/.cache/bazel/_bazel_dmarting/e5cce820cc082410b4fcc604db349066/external/local_config_cuda/cuda/BUILD:48:1: no such package '@local_config_cuda//tensorflow': BUILD file not found on package path and referenced by '@local_config_cuda//cuda:cudart_static'
ERROR: /usr/local/google/home/dmarting/.cache/bazel/_bazel_dmarting/e5cce820cc082410b4fcc604db349066/external/local_config_cuda/cuda/BUILD:48:1: no such package '@local_config_cuda//tensorflow': BUILD file not found on package path and referenced by '@local_config_cuda//cuda:cudart_static'
ERROR: Evaluation of query "deps((//tensorflow/... - //tensorflow/examples/android/...))" failed: errors were encountered while computing transitive closure

The stacktrace is probably coming from one of those case we fixed in recent release.

@blodan blodan Changing to local condition just as previously done with darwin
a93cee6
@blodan
Contributor
blodan commented Feb 3, 2017

Thanks for the more descriptive error @damienmg, that saved me a lot of time.

I've now changed to a local config for the cuda BUILD.tpl as you seem to have done with darwin previously, it should build fine in jenkins now.

@rmlarsen
Member
rmlarsen commented Feb 3, 2017

@tensorflow-jenkins test this please

@rmlarsen
Member
rmlarsen commented Feb 3, 2017

@blodan it appears there are still formatting errors in the build files :

FAIL: buildifier found errors and/or warnings in above BUILD files.
buildifier suggested the following changes:
1310,1313c1310,1313
< "//tensorflow:FreeBSD": [],
< "//conditions:default": ["-ldl"],
< }) + [
< "-lm"

    "//tensorflow:FreeBSD": [],
    "//conditions:default": ["-ldl"],
}) + [
    "-lm",

Please fix manually or run buildifier to auto-fix.

@blodan
Contributor
blodan commented Feb 6, 2017

I've now removed one indentation as specified, hopefully it goes through now! :)

@blodan blodan Removing one indentation here too
f2fe461
@gunan
Member
gunan commented Feb 6, 2017

Jenkins, test this please.

tensorflow/BUILD
@@ -86,6 +86,12 @@ config_setting(
visibility = ["//visibility:public"],
)
+config_setting(
+ name = "FreeBSD",
@jart
jart Feb 6, 2017 Member

Nit: Can we make this lowercase like the others?

@blodan blodan - Changing new config name to lowercase
- Fixing buildifier warning
4dc83ca
@blodan
Contributor
blodan commented Feb 6, 2017

I've now changed the config option to lowercase and fixed the last buildifer warning

@gunan
Member
gunan commented Feb 7, 2017

Jenkins, test this please.

@gunan
Member
gunan commented Feb 13, 2017

Looks like all tests are passing.
@jart @damienmg could you take another look?

@vrv
Contributor
vrv commented Feb 13, 2017

@jart does this all look good to you?

@tensorflow-jenkins test this please

@jart
jart approved these changes Feb 13, 2017 View changes
@vrv vrv merged commit e75c0fc into tensorflow:master Feb 15, 2017

11 checks passed

Android Demo App SUCCESS
Details
Linux CPU Tests SUCCESS
Details
Linux CPU Tests (Python 3) SUCCESS
Details
Linux CPU Tests CMAKE SUCCESS
Details
Linux CPU Tests Makefile SUCCESS
Details
Linux GPU SUCCESS
Details
MacOS CPU Tests SUCCESS
Details
Sanity Checks SUCCESS
Details
Windows Cmake Tests SUCCESS
Details
ci.tensorflow.org SUCCESS
Details
cla/google All necessary CLAs are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment