Add compiler-specific warnings in Bazel.

[david-german-tri]

This change is 

[david-german-tri]

I'm not exactly in love with this approach, but it seems to work on both OS X and Ubuntu. @jwnimmer-tri for initial feature review. I've also opened bazelbuild/bazel#2358

[jwnimmer-tri]

First pass done.

I will probably wait for one round of any edits you elect to do, and after that work on stress testing it locally.

---

Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

---

a discussion (no related file):
Please run tools/buildifier.sh to format this PR.

---

a discussion (no related file):
FYI Probably not worth attempting in this same PR, but I wonder if this toolchain shim would let us do better on #4464. I guess if local_config_cc has already finished it may be too late. Maybe we can still fail-fast and report a good error, though.

---

tools/amend_crosstool.sh, line 1 at r1 (raw file):

#!/bin/bash

Meta This file mixed tabs and spaces for indent.

Also lines are >80 and while that's not a rule I guess, it's better for reviewable to keep it all <80.

---

tools/amend_crosstool.sh, line 10 at r1 (raw file):

# It also may not be the right thing to do, since it forces our flag choices onto all
# the externals.
# TODO(david-german-tri): Improve this if possible.

Add citation to bazelbuild/build#2358 discussion?

---

tools/amend_crosstool.sh, line 20 at r1 (raw file):

    | sed 's/cc_wrapper\.sh/.\/cc_wrapper.sh/' \
    | grep -v '/bin/false' \
    > $compiler_version_commands

BTW If you put the | on the prior line, you don't need the \s; it will continue the line for you.

---

tools/amend_crosstool.sh, line 24 at r1 (raw file):

has_gcc=$?
source $compiler_version_commands | grep clang > /dev/null
has_clang=$?

BTW Consider confirming that the exitcode of the source ... portion was success (zero)? Also that would let you only run it once (to a tmpfile) and then grep twice.

---

tools/amend_crosstool.sh, line 36 at r1 (raw file):

	# Assemble additional warnings for gcc.
	echo '  # Additional GCC warnings from amend_crosstool.sh' >> $extra_warnings
	echo '  cxx_flag: "-Werror=extra"' >> $extra_warnings

BTW Should we read these from a one-per-line text file like drake-distro/tools/gcc-cxxopts.txt that the repository rule could reference? That might be easier for developers to maintain over time. Actually, I guess all of the echo'd content could just be in a file, cxx_flag goop and all. Then this script just has to chose which txt file to use and then wedge it in place.

---

tools/amend_crosstool.sh, line 43 at r1 (raw file):

    # Inject the warnings into the "local" configuration.
    sed -i -e "/^toolchain {/r ${extra_warnings}" CROSSTOOL
	exit 0

BTW De-duplicate the above three lines between the two cases that share them?

---

tools/bazel.rc, line 9 at r1 (raw file):

build --crosstool_top=//external:drake_toolchain

# Compilation flags that apply on all supported compilers.

BTW Consider a comment citation for where to place new warning flags that are only for some compilers?

---

tools/toolchain.bzl, line 17 at r1 (raw file):

        print("toolchain.bzl: Failed to detect exactly one supported compiler.")

drake_toolchain_autoconf = repository_rule(

BTW I think the word autoconf here could be easily confused for GNU autoconf.

---

Comments from Reviewable

[jwnimmer-tri]

Review status: all files reviewed at latest revision, 11 unresolved discussions.

---

tools/toolchain.bzl, line 8 at r1 (raw file):

def _drake_toolchain_impl(repository_ctx):
    repository_ctx.file("scratch", content="foo")

Unclear what purpose this serves.

---

Comments from Reviewable

[david-german-tri]

Review status: 3 of 10 files reviewed at latest revision, 11 unresolved discussions.

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Please run tools/buildifier.sh to format this PR.

Done.

---

a discussion (no related file):

Maybe we can still fail-fast and report a good error, though.

Yes, we definitely could.

---

tools/amend_crosstool.sh, line 1 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Meta This file mixed tabs and spaces for indent.

Also lines are >80 and while that's not a rule I guess, it's better for reviewable to keep it all <80.

Done.

---

tools/amend_crosstool.sh, line 10 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Add citation to bazelbuild/build#2358 discussion?

Done.

---

tools/amend_crosstool.sh, line 20 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW If you put the | on the prior line, you don't need the \s; it will continue the line for you.

Done.

---

tools/amend_crosstool.sh, line 24 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Consider confirming that the exitcode of the source ... portion was success (zero)? Also that would let you only run it once (to a tmpfile) and then grep twice.

Done.

---

tools/amend_crosstool.sh, line 36 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Should we read these from a one-per-line text file like drake-distro/tools/gcc-cxxopts.txt that the repository rule could reference? That might be easier for developers to maintain over time. Actually, I guess all of the echo'd content could just be in a file, cxx_flag goop and all. Then this script just has to chose which txt file to use and then wedge it in place.

Done.

---

tools/amend_crosstool.sh, line 43 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW De-duplicate the above three lines between the two cases that share them?

Done.

---

tools/bazel.rc, line 9 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Consider a comment citation for where to place new warning flags that are only for some compilers?

Done.

---

tools/toolchain.bzl, line 8 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Unclear what purpose this serves.

Done.

---

tools/toolchain.bzl, line 17 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I think the word autoconf here could be easily confused for GNU autoconf.

Done.

---

Comments from Reviewable

[jwnimmer-tri]

Alright, just a few more notes. Next I will stress test it locally. After these discussions have a response, probably a fine time to tag a second reviewer.

---

Reviewed 7 of 7 files at r2.
Review status: 9 of 10 files reviewed at latest revision, 3 unresolved discussions.

---

tools/amend_crosstool.sh, line 1 at r1 (raw file):

Previously, david-german-tri (David German) wrote…

Done.

These two intro paragraphs are still showing up >>80 in reviewable for me?

---

tools/amend_crosstool.sh, line 26 at r2 (raw file):

compiler_versions=$(mktemp)
source $compiler_version_commands > $compiler_versions

BTW Meta its always a bit safer to quote all variable-provided filenames, e.g. "$compiler_version_commands" and "$compiler_versions" and etc. below, in case the user is insane and their tmpdir has spaces in its path.

---

tools/amend_crosstool.sh, line 42 at r2 (raw file):

# cxx_flag field, and appends the result onto the second file.
read_options_into() {
    sed -e 's/^/  cxx_flag: "/' "$1" | sed -e 's/$/"/' >> "$2"

Does this script fail-fast when gcc_options.txt disappears or isn't sandboxed correctly somehow? Otherwise it seems like we could silently lose warnings accidentally.

---

tools/BUILD, line 40 at r2 (raw file):

        "gcc_options.txt",
    ],
)

BTW Both of these labels are public visibility. I guess that's okay?

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

tools/BUILD, line 30 at r2 (raw file):

    name = "amend_crosstool",
    srcs = ["amend_crosstool.sh"],
    deps = [":compiler_options"],

Huh, I would have thought this was at least data? Or is it really just all data anyway, and amend_crosstool.sh should also just be in a filegroup and not a sh_binary? I guess I don't know exactly how sandboxing works (or doesn't) for repository rules.

---

Comments from Reviewable

[jwnimmer-tri]

tools/BUILD, line 30 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Huh, I would have thought this was at least data? Or is it really just all data anyway, and amend_crosstool.sh should also just be in a filegroup and not a sh_binary? I guess I don't know exactly how sandboxing works (or doesn't) for repository rules.

Indeed, changing gcc_options.txt only has an effect the second time I run bazel. I think the repository_rule has to declare its dependencies on all of the labels is uses as inputs.

---

Comments from Reviewable

[jwnimmer-tri]

I take that back. I'm going to stop testing, and we should defer additional review, until the PR correctly and immediately rebuilds everything when the files in this PR are edited.

---

Comments from Reviewable

[david-german-tri]

I agree. Looking into it now.

---

Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

Comments from Reviewable

[david-german-tri]

It turns out there is no way to invalidate a repository_rule based on changes to a file that it references. (Because we referenced a Label, we got invalidation of that Label on the second call to bazel after changes to a file included in the Label.)

I'm abandoning this approach in favor of (3) from bazelbuild/bazel#2358.