Bug 219840 - [NEW PORT] devel/bazel-clang38: using clang38 as default crosstool
Summary: [NEW PORT] devel/bazel-clang38: using clang38 as default crosstool
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Li-Wen Hsu
URL:
Keywords:
Depends on: 219838
Blocks: 219609
  Show dependency treegraph
 
Reported: 2017-06-07 12:09 UTC by Jov
Modified: 2017-07-30 21:47 UTC (History)
2 users (show)

See Also:


Attachments
bazel_clang38 (919 bytes, text/plain)
2017-06-07 12:09 UTC, Jov
no flags Details
bazel_clang38_v2.shar (963 bytes, text/plain)
2017-06-08 04:07 UTC, Jov
amutu: maintainer-approval+
Details
bazel_clang38.shar (998 bytes, text/plain)
2017-06-09 01:12 UTC, Jov
no flags Details
bazel_clang38.shar (5.38 KB, text/plain)
2017-06-25 10:02 UTC, Jov
no flags Details
bazel_clang38.shar (5.56 KB, text/plain)
2017-06-25 11:27 UTC, Jov
no flags Details
bazel-clang38.shar (5.58 KB, text/plain)
2017-07-13 11:41 UTC, Jov
no flags Details
bazel-clang38.shar (5.58 KB, text/plain)
2017-07-13 11:55 UTC, Jov
no flags Details
Adding a clang38 option to devel/bazel (986 bytes, patch)
2017-07-17 08:22 UTC, Klaus Aehlig
no flags Details | Diff
bazel-clang38.shar (747 bytes, text/plain)
2017-07-19 10:32 UTC, Jov
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jov 2017-06-07 12:09:51 UTC
Created attachment 183293 [details]
bazel_clang38

This is a slave port of bazel with clang38 as default crosstool.I make this port for compiling tensorflow on FreeBSD 10.x.Tensorflow do not compile on FreeBSD 10.x with Clang3.4.
see more for tensorflow PR:https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=219609

this port depend on master port PR:https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=219838
Comment 1 Jov 2017-06-07 12:12:54 UTC
And I test pass this port on poudriere with 10.3R-amd64
Comment 2 Jov 2017-06-08 04:07:35 UTC
Created attachment 183321 [details]
bazel_clang38_v2.shar

the parent port using "?=" instead of "+=",I update the slave port.
Comment 3 Jov 2017-06-09 01:12:30 UTC
Created attachment 183345 [details]
bazel_clang38.shar

v3 patch:
add clang38 as run time depends
Comment 4 Jov 2017-06-25 10:02:08 UTC
Created attachment 183781 [details]
bazel_clang38.shar

changes:
1.get rid of /proc mount when build c/c++, this will make science/tensorflow build on poudriere without "USE_PROCFS=yes". 
2.make this port not a slave port of /devel/bazel,so we do not need the devel/bazel do any patch(bug 219838 can be closed).
Comment 5 Jov 2017-06-25 11:27:32 UTC
Created attachment 183783 [details]
bazel_clang38.shar

add missing lines.
Comment 6 Klaus Aehlig 2017-06-26 21:26:52 UTC
Does it really make sense to have this as a self-contained port not building on devel/bazel? Makefile, distinfo, pkg-plist, and pkg-descr are essentially a copy of those of devel/bazel; thus the maintenance effort would be doubled which it wouldn't with your initial proposal of depending on devel/bazel. (As mentioned on bug #219838, I have no objections of making BUILD_DEPENDS overridable.)
Comment 7 Jov 2017-06-27 06:19:11 UTC
(In reply to Klaus Aehlig from comment #6)
There are two differences compare with /devel/bazel:
1.use port clang38 as default c/c++ compiler.This is needed to build tensorflow on FreeBSD 10.x.
2.patch the bazel to get rid of /proc depends.This is needed to build with unprivileged user or poudriere without USE_PROCFS=yes.

For 1 there no one commits the patch so far for the last several weeks.
For 2 I make a pull request on github to bazel,but the review work is too slow(I find you are happened to be the reviewer).
I am pushing the tensorflow to ports and hope it is accepted before Q3 quarterly branch cut, I think making this port standalone make it easier to go forward.

I will maintain this port as long as it is useful. I think after 10.x EOL and the 2 accepted,I can change tensorflow to depend on /devel/bazel,then this port will not be needed.
Comment 8 Klaus Aehlig 2017-06-27 07:14:13 UTC
1. is entirely up to the committer, but keep in mind that committer can commit patches to other ports as a preparation step; as mentioned, I don't have any objections to this change (but I'm not a committer).

2. Why not use the EXTRA_PATCHES += construct?

(Side remark: your pull request was sent after bazel 0.5.2 baseline was cut and for 0.5.3 we're still in time, so the time for review did not at all influence which bazel releases contain that patch.)

In the end, I can live with the fork, but I don't think it's good for the ports tree to duplicate instead of reuse. But that, again, is up to the committers to decide.
Comment 9 Li-Wen Hsu freebsd_committer 2017-07-12 16:22:56 UTC
(In reply to Klaus Aehlig from comment #8)
> 1. is entirely up to the committer, but keep in mind that committer can commit patches  to other ports as a preparation step; as mentioned, I don't have any objections to this change (but I'm not a committer).
> 
> 2. Why not use the EXTRA_PATCHES += construct?

I would like to see this becomes a OPTION and contains in the master port.  We can make devel/bazel_clang38 as a slave port for devel/bazel with that OPTION on.  This reduces the common codes in the ports tree.  A slave port gives us a pre-built package and makes the port depends on it work.

> (Side remark: your pull request was sent after bazel 0.5.2 baseline was cut and for 0.5.3 we're still in time, so the time for review did not at all influence which bazel releases contain that patch.)

Upstreaming as much as we can is always a good idea.

> In the end, I can live with the fork, but I don't think it's good for the ports tree to duplicate instead of reuse. But that, again, is up to the committers to decide.

I think having fork for a short time to make the thing we are interested work while working with other parties is acceptable, or sometimes we just cannot make things happen.

Some other comments:

- We need to set CONFLICTS_INSTALL in both ports because this port and devel/bazel both instal bin/bazel
- I would rename this port to devel/bazel-clang38, and using -clang38 in PKGNAMESUFFIX.
Comment 10 Jov 2017-07-13 11:41:47 UTC
Created attachment 184324 [details]
bazel-clang38.shar

rename to bazel-clang38 and add use PKGNAMESUFFIX
Comment 11 Jov 2017-07-13 11:52:50 UTC
(In reply to Li-Wen Hsu from comment #9)
Hi Li-Wen, thanks for your input!

I have updated the shar file to use '-' instead '_' in the port name, and use PKGNAMESUFFIX. Poudriere testport passed.

For the CONFLICTS_INSTALL, I think it is not necessary to add it to both ports as the conflict will work if either port has this set. This is a test on my host:

root@xx:~ # pkg install bazel
Updating FreeBSD repository catalogue...
FreeBSD repository is up to date.
All repositories are up to date.
The following 1 package(s) will be affected (of 0 checked):

New packages to be INSTALLED:
    bazel: 0.5.0

Number of packages to be installed: 1

The process will require 127 MiB more space.
124 MiB to be downloaded.

Proceed with this action? [y/N]: y
[1/1] Fetching bazel-0.5.0.txz: 100%  124 MiB 314.3kB/s    06:53   
Checking integrity... done (1 conflicting)
  - bazel-0.5.0 conflicts with bazel-clang38-0.5.0 on /usr/local/bin/bazel
Checking integrity... done (0 conflicting)
Conflicts with the existing packages have been found.
One more solver iteration is needed to resolve them.
The following 2 package(s) will be affected (of 0 checked):

Installed packages to be REMOVED:
    bazel-clang38-0.5.0

New packages to be INSTALLED:
    bazel: 0.5.0

Number of packages to be removed: 1
Number of packages to be installed: 1

Proceed with this action? [y/N]:
Comment 12 Jov 2017-07-13 11:55:04 UTC
Created attachment 184325 [details]
bazel-clang38.shar

reorder the PKGNAMESUFFIX to make portlint happy

QA:
portlint -AC
WARN: Makefile: [21]: possible direct use of command "file" found. use ${FILE} instead.
0 fatal errors and 1 warning found.
Comment 13 Li-Wen Hsu freebsd_committer 2017-07-13 12:03:45 UTC
Thanks!  For CONFLICTS_INSTALL, I need check again, I think it is need for using `make install`

And how about merging to bazel as an OPTION thing?
Comment 14 Jov 2017-07-13 12:23:33 UTC
(In reply to Li-Wen Hsu from comment #13)
If the option is set to OFF by default, the tensorflow can not be built.And also it is not reasonable to set to ON by default because of allways llvm38 depends.
Comment 15 Li-Wen Hsu freebsd_committer 2017-07-13 12:55:32 UTC
(In reply to Jov from comment #14)

I understand, what I mean is:

devel/bazel: has an option, ex: CLANG38, default off
devel/bazel-clang38: set CLANG38, dfault on

And we set tensorflow depends on devel/bazel-clang38

Does this make sense to you?
Comment 16 Jov 2017-07-13 13:39:47 UTC
(In reply to Li-Wen Hsu from comment #15)
I think It's OK.

Hi aehlig, is this make sense to you.
Comment 17 Klaus Aehlig 2017-07-17 08:22:43 UTC
Created attachment 184421 [details]
Adding a clang38 option to devel/bazel

As discussed, it seems reasonable to add an option to use clang38 to devel/bazel, so that that option can be used by a slave port.

If desired, we can also use https://github.com/bazelbuild/bazel/commit/fa46172bc3b9bd2398a9d79a05fc5cd55f6059ad as an additional patch, but a version of bazel containing this patch will be released soon anyway.
Comment 18 Klaus Aehlig 2017-07-17 08:24:51 UTC
(In reply to Jov from comment #16)

> Hi aehlig, is this make sense to you.

Yes, I'm always a fan of avoiding code duplication. I've added a diff how adding such an option to devel/bazel might look like and I'm happy with this being committed to devel/bazel as part of adding devel/bazel-clang38 as a slave port.
Comment 19 Jov 2017-07-17 08:50:53 UTC
(In reply to Klaus Aehlig from comment #18)
Thanks, Klaus!

I will reopen 219838 and update the patch to add the clang38 option.
Comment 20 Jov 2017-07-19 10:32:50 UTC
Created attachment 184498 [details]
bazel-clang38.shar

Make this is a slave port of devel/bazel.
Comment 21 commit-hook freebsd_committer 2017-07-30 21:47:01 UTC
A commit references this bug:

Author: lwhsu
Date: Sun Jul 30 21:46:10 UTC 2017
New revision: 446943
URL: https://svnweb.freebsd.org/changeset/ports/446943

Log:
  Add bazel-clang38: using clang38 as crosstool for bazel

  PR:		219840
  Submitted by:	Jov <amutu@amutu.com>

Changes:
  head/devel/Makefile
  head/devel/bazel-clang38/
  head/devel/bazel-clang38/Makefile