Fix cgo build failures with specific GCC flags

[AustinSchuh]

cgo runs the compiler over a test file as part of the build process. It then parses the compiler errors. This requires the compiler to be configured well enough to work for cgo.

-fdiagnostics-color is another way to enable color support on GCC. This adds colors unconditionally to the output (bazel doesn't present a TTY, so to get colors with bazel, you need to force them on).

-fmax-errors=... is a way to prevent GCC from completely flooding your terminal when something goes wrong. A missed semicolon or misplaced {} can cause thousands of lines of errors. This can stop showing errors before cgo sees all the errors it is looking for. We want to strip it out too.

[jayconrod]

Could you add comments (and also edit the PR description) to explain more? I understand why -fdiagnostics-color and -Wno-unused-variable are useful, though it would be good to document. I don't understand -fmax-errors=20.

[jayconrod]

This code is deprecated and is only used to build cgo code for a very narrow legacy case.

_cgo_context_data_impl is probably the right place to add this flag (to c_compile_options).

[AustinSchuh]

I've confirmed by A/B building with and without this line that this is the exact target that is causing the problem.

I'm building github.com/mattn/go-sqlite3

The build rule generated is:

go_library(
    name = "go_default_library",
    srcs = [ 
        "backup.go",
        "callback.go",
        "doc.go",
        "error.go",
        "sqlite3.go",
        "sqlite3-binding.c",
        "sqlite3-binding.h",
        "sqlite3_context.go",
        "sqlite3_go18.go",
        "sqlite3_load_extension.go",
        "sqlite3_other.go",
        "sqlite3_solaris.go",
        "sqlite3_type.go",
        "sqlite3_windows.go",
        "sqlite3ext.h",
        "static_mock.go",
    ],  
    cgo = True,
    clinkopts = select({
        "@io_bazel_rules_go//go/platform:linux": [
            "-ldl",
        ],
        "@io_bazel_rules_go//go/platform:solaris": [
            "-lc",
        ],
        "@io_bazel_rules_go//go/platform:windows": [
            "-lmingwex -lmingw32",
        ],
        "//conditions:default": [], 
    }), 
    copts = [ 
        "-std=gnu99",
        "-DSQLITE_ENABLE_RTREE -DSQLITE_THREADSAFE=1 -DHAVE_USLEEP=1",
        "-DSQLITE_ENABLE_FTS3 -DSQLITE_ENABLE_FTS3_PARENTHESIS -DSQLITE_ENABLE_FTS4_UNICODE61",
        "-DSQLITE_TRACE_SIZE_LIMIT=15",
        "-DSQLITE_DISABLE_INTRINSIC",
        "-Wno-deprecated-declarations",
    ] + select({
        "@io_bazel_rules_go//go/platform:android": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:darwin": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:dragonfly": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:freebsd": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:linux": [
            "-DHAVE_PREAD64=1 -DHAVE_PWRITE64=1",
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:nacl": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:netbsd": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:openbsd": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:plan9": [
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:solaris": [
            "-D__EXTENSIONS__=1",
            "-I.",
        ],
        "@io_bazel_rules_go//go/platform:windows": [
            "-I. -fno-stack-check -fno-stack-protector -mno-stack-arg-probe",
        ],
        "//conditions:default": [], 
    }) + select({
        "@io_bazel_rules_go//go/platform:windows_386": [
            "-D_USE_32BIT_TIME_T",
        ],
        "//conditions:default": [], 
    }), 
    importpath = "github.com/mattn/go-sqlite3",
    visibility = ["//visibility:public"],
)

I put it here since restricting it to the smallest scope seemed like a good idea rather than being more broad. I'm happy to do whatever you want though.

[jayconrod]

Hmm, that's strange. Is that on master or an older version of rules_go? I tried bazel build @com_github_mattn_go_sqlite3//:go_default_library with rules_go and go-sqlite3 at master, but setup_cgo_library_for_mode wasn't called (checked by adding a fail in there).

By any chance, do you have objc = True set in a rule somewhere?

In any case please do add this to _cgo_context_data_impl. The legacy path should disappear at some point soon.

[AustinSchuh]

Just for you, I upgraded to the latest released rules_go (0.18.6), and I'm still seeing it. I'm using Gazelle 0.17.0. The only objc = True in our code base is in a test in rules_go.

I removed the offending rule and gazelle added it back without changes. So I think it's not some legacy flag on the rule. I suspect Bazel is building the binary even if it is unused for completeness.

I used -s with bazel and changed some cgo code to add an unused variable warning and already found -Wno-unused-variable in the command line used to build the cgo generated code, and in the command line used to build standalone c code. I'm reluctant to add it somewhere else since that would duplicate the flag.

Let me know what you want to do.

[jayconrod]

Ah, the release-0.18 branch was cut before the new cgo stuff was merged, so this path will still be active there. It shouldn't be active at master though (unless objc = True).

So I think this option should in both _cgo_context_data_impl and setup_cgo_library_for_mode. I'll backport the latter part to release-0.18 after this is merged (the next minor release will likely be early July).

Some time after release-0.19 is cut, I'll remove the legacy path, which will un-duplicate this.

[AustinSchuh]

Your request got hard... I want to test master with the failing code before sending it out for review, but that's a fair amount of work. I'm tempted to just roll this part back and revisit when you cut a new release.

[jayconrod]

Up to you. I'm fine with this being left here, just be aware that the old path will disappear in a few weeks.

[AustinSchuh]

Let's just do that then. Merge this as is (assuming you have no more feedback), and then when we upgrade again, I'll take stock of how things look and send you any reviews needed to finish this.

Thanks Jay!

[jayconrod]

Test failures are caused by #2100, unrelated this this PR.

[AustinSchuh]

Process question: should I use an amend/rebase workflow, or just add more commits? I normally use gerrit for reviews.

[jayconrod]

I prefer more commits because it preserves comment threads, but either works: the PR will be squashed and merged, so it ends up as a single commit in a linear history.

(I also use Gerrit, and I wish GitHub code review would copy some things from there)

[jayconrod]

Sounds good.

CI failures are due to a regression in Bazel 0.27.0 (hopefully fixed in 0.27.1), unrelated to this change.