Consolidate actions needed to build a package

[jayconrod]

Currently, we define separate actions for:

• Compiling Go code
• Running cgo (actually a few different actions)
• Compiling C/C++ code (via cc_library, cc_binary).
• Assembling Go assembly
• Packing .o files produced by the assembler into .a files
• Coverage

We should consolidate all this functionality into a single action. This should reduce set up / tear down overhead in both local configurations (sandboxing) and remote configurations (file transfer).

Perhaps more importantly, cgo will be much easier to maintain if we move it out of loading / analysis phases into execution. Previously, we needed cgo logic to be in loading / analysis because we needed to compile generated C code with cc_library / cc_binary rules. Bazel now gives us enough information to run the C compiler on our own.

[jayconrod]

@steeve #2027 is finally, finally merged. Would you mind trying it out some time in the next couple weeks? cgo is pretty much rewritten, and you're making the most extensive use of cgo that I know of.

We're still using the old cgo code when objc = True. I'd love to remove this path, since it's a lot of very complicated and now redundant code. I think you're right that we can compile .m files using the toolchain from cc_common on Darwin, but I don't think we can support the attributes that currently get passed to objc_library. That includes stuff like non_arc_srcs, sdk_frameworks, etc. Are you currently using these on go_library rules? If so, would it be viable to move the .m files into separate objc_library rules, then depend on those via cdeps?

[steeve]

Good job!
We are indeed using sdk_frameworks.
I just did a quick checkout and for now and for now it's not building on both iOS and Android.
I'll sure update with some infos when I get the chance to look into it.

I'm on the fence with regards to having everything into one action. It sure does help for remote builds, but I'm not sure how it stands with regards to caching and incremental builds ?

[jayconrod]

I don't think this should affect caching or incremental builds too much. If a source file changes for a package, all the actions for that package (previously many, now one) need to be re-run.

One case that might be slower is if a package has several C/C++/ObjC sources that take a long time to compile. If a .go file changes, or if an imported package changes, those will get recompiled. I think I'd recommend pulling those into an explicit separate cc_library / objc_library, but I'd like to know if there are cases where that's very difficult.

[steeve]

That's a fair point indeed.
So the build failure was first due to an empty output group, which wasn't needed anyway in our internal rules_gomobile repo (which I'm working on open sourcing when I get some time).

So I've managed to make our Android library build with the following changes, which I'm not sure is the correct one, but gets the job done:

diff --git a/go/tools/builders/filter.go b/go/tools/builders/filter.go
index 5771b580..7c1f3bd7 100644
--- a/go/tools/builders/filter.go
+++ b/go/tools/builders/filter.go
@@ -100,7 +100,7 @@ func readFileInfo(bctx build.Context, input string, needPackage bool) (fileInfo,
                        fi.ext = mExt
                case ".s":
                        fi.ext = sExt
-               case ".h":
+               case ".h", ".hh", ".hpp", ".hxx":
                        fi.ext = hExt
                default:
                        return fileInfo{}, fmt.Errorf("unrecognized file extension: %s", ext)

All in all, the change in build time is definitely noticeable on Android builds! Sandboxing the Android SDK and NDK isn't free.

Good job jay !

Now let's see why the iOS isn't building.

[steeve]

Ok so the build failures on iOS are happening due to objc bridge code being in comments in a .go file:

/*
#cgo CFLAGS: -x objective-c -fmodules -fobjc-arc
@import Foundation;
*/

[steeve]

The correct copts are appended via:

        kwargs["copts"] = copts + select({
            "@co_znly_rules_gomobile//platform:ios": [
                "-x objective-c",
                "-fmodules",
                "-fobjc-arc",
            ],
            "//conditions:default": [],
        })

Which is weird that they are not picked up ? Here are the flags for the action:

-gcflags '-N -l -shared' -asmflags '-N -l -shared' -cppflags '-I application' -cflags '-D_FORTIFY_SOURCE=1 -fstack-protector -Wthread-safety -Wself-assign -fno-omit-frame-pointer -g DEBUG_PREFIX_MAP_PWD=. -isysroot __BAZEL_XCODE_SDKROOT__ -miphoneos-version-min=10.0 -no-canonical-prefixes -Wno-builtin-macro-redefined -D__DATE__="redacted" -D__TIMESTAMP__="redacted" -D__TIME__="redacted" -target arm64-apple-ios -miphoneos-version-min=7.0 -x objective-c -fmodules -fobjc-arc -fPIC' -cxxflags '-D_FORTIFY_SOURCE=1 -fstack-protector -Wthread-safety -Wself-assign -fno-omit-frame-pointer -g -std=c++11 DEBUG_PREFIX_MAP_PWD=. -isysroot __BAZEL_XCODE_SDKROOT__ -miphoneos-version-min=10.0 -no-canonical-prefixes -Wno-builtin-macro-redefined -D__DATE__="redacted" -D__TIMESTAMP__="redacted" -D__TIME__="redacted" -target arm64-apple-ios -miphoneos-version-min=7.0 -fPIC' -ldflags '-lc++ -fobjc-link-runtime -framework UIKit -headerpad_max_install_names -no-canonical-prefixes -target arm64-apple-ios -miphoneos-version-min=10.0 -miphoneos-version-min=7.0'

As you can see, -x objective-c -fmodules -fobjc-arc are definitely there.

[steeve]

Also, the problem I see with emulating objc with cc_common is that the objc provider wouldn't be passed on, so some things will be dropped when using a go_library with objc as a dep to a objc_library.
For instance:

objc with sdk_frameworks -> cgo -> go (shared) -> objc (lost sdk_frameworks)

Note that this is already the case since today the objc provider is lost since cgo.o is a cc_binary.

[steeve]

Actually @jayconrod, I think we can simply use the objc actions directly https://github.com/bazelbuild/bazel/blob/master/tools/build_defs/cc/action_names.bzl#L115-L120 and get the command lines.

They were added by bazelbuild/bazel@f451b79

[steeve]

Also, I think you might want to use the ACTION_NAMES struct rather than the names directly.

EDIT: actually, they were added later by bazelbuild/bazel@f147608

[steeve]

So I went ahead and tried to display the command line for the objc-compile action, and alas there is no -x objective-c attribute.

I'm seeing several possibilities:

• manually add -x objective-c -fmodules -fobjc-arc to a normal C compilation step
  • this can cause problem when using sdk_frameworks or link problems
• let the user add those arguments themselves (which is what Go does)
• somehow use apple_common.new_objc_provider under the hood
  • this might be the cleanest approach, but doesn't play well with how GoCompilePkg works
  • the problematic code is mostly CGo-ObjC bridge code

[steeve]

Also, master doesn't build correctly for iOS. Not sure why right now, but I've already seen that error.

[steeve]

Ok so the iOS problem error was due to a missing --apple_platform_type=ios in the bazelrc.

That said, the objc problem I had in #1957 (comment) is still there.

[jayconrod]

(Sorry I haven't responded to this yet. I'm trying to get some changes into cmd/go before the 1.13 freeze next week, and we're pretty swamped at the moment.)

[steeve]

No worries! I'll send a PR if I get to working on that.

[steeve]

PR opened at #2045

[jayconrod]

Hey @steeve, I'd like to understand what Objective C support is needed on the new GoCompilePkg path before I can remove the old actions and most stuff in cgo.bzl, then close this issue.

My vision for this is that go_library would accept .m and .mm files, and would support the basic stuff that go build supports. Attributes that the go_library macro currently passes through to objc_library when objc = True would no longer be supported. If you need these, you'd need to extract the .m files into a separate objc_library, then depend on it via cdeps.

If this is too inconvenient, we could continue having a go_library macro does the above, but I'd still like to get rid of most of the cgo.bzl stuff.

Here's what I have so far:

• go_library and other rules (the rules, not the macros) must accept .m and .mm files and pass them to GoCompilePkg.
• GoCompilePkg should build .m files with CC and the same options that would be used for a .c file. .mm files would be built with C++ options. Some language adjustment may be needed with -x.
• We may need an objcopts attribute on go_library to specify additional options for compiling .m and .mm files.
  • Please confirm. We'll definitely have pure some C files, at least generated by cgo from .go sources. I'm not sure how picky clang is about these.
• GoCompilePkg: use objects from cdeps when linking _cgo_.o #2067 - When we link _cgo_.o before generating _cgo_imports.go, we must include objects from cdeps. (This isn't an Objective C issue, but it does block this project).

Please let me know if I'm missing anything you know of. After the stuff above is done, I'll ask you to check that this works in your build before removing anything.

[steeve]

That looks like a good plan.

-x objective-c can go a long way imho. Plus the objc support in cgo.bzl isn't as clean anyway: https://github.com/bazelbuild/rules_go/blob/master/go/private/rules/cgo.bzl#L604-L609

The main problem though, used to be that the generated _cgo.o was exposed as a cc_library (objc_library -> cc_library -> cc_binary), this means that some properties are lost (sdk_frameworks comes to mind). This means that some options are not passed on down the graph.
Actually, I described it in #1957 (comment)

This may not look like a big deal, but it's would actually be pretty useful when calling custom .framework, as they need to be linked in the final binary.

[steeve]

Also, I see objc_library got simplified massively: bazelbuild/bazel#7594

[jayconrod]

Do you have anything that's using the generated _cgo_.o file? It's supposed to just be used to generate _cgo_imports.go. From your comment in #1957, it doesn't sound like this is working now.

I don't think I understood why you needed Go rules to emit the objc provider before. Instead of go_library providing objc, it might be better to gather information from cdeps with objc providers into a field in GoArchive, then if go_binary is linked in mode c-archive or c-shared, it could provide objc if there was any Objective C stuff in linked GoArchives.

That's not quite a clean interface and probably needs more thought, but for C++ at least, go_binary should produce a .a, .so, or .dylib, which can be wrapped with cc_import. I think we should follow the same general approach with Objective C, but we should preserve objc and keep the interface simple (perhaps skipping the cc_import step) if we can.

[jayconrod]

Closing this at last.

• go_library, go_binary, and go_test now accept .m and .mm files. They'll be compiled with the Objective-C and Objective-C++ flags from the toolchain, plus copts or cxxopts. I haven't added separate objcopts and objcxxopts attributes that set these per library. #cgo directives don't provide anything like this, so I expect they shouldn't be widely needed.
• _cgo_.o picks up all link flags, including those linking static libraries from cdeps. Not sure when that was fixed, but I've added a test to verify it.
• _cgo_.o still won't be visible in the action graph. It's an implementation detail of cgo, so I'd recommend against relying on it. Instead, use go_binary in the c-archive or c-shared link mode, as described above.