Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable transitive target CPU constraint #1264

Closed
steeve opened this issue Jan 22, 2018 · 46 comments
Closed

Enable transitive target CPU constraint #1264

steeve opened this issue Jan 22, 2018 · 46 comments
Labels

Comments

@steeve
Copy link
Contributor

steeve commented Jan 22, 2018

Hi,

As part of implementing c-archive and c-shared support, I'm trying to properly propagate CPU selection to the Go toolchain when doing things such as building an iOS app or library.
See https://github.com/znly/rules_go/commit/344d1337b0350ea5e3b9e712437d38f33054d8a7 and https://github.com/znly/rules_go/commit/fce5ac2d1f8a18ae77712cd392c76b54cb03e2d8.

I'm am quite confused as to how to do that properly based on constraints.

I am able to properly select based on a config_setting such as:

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

However, when trying to turn that into a constraint, in doing either:

constraint_setting(name = ":cpu")
constraint_value(
    name = 'ios_arm64',
    constraint_setting = ":cpu",
)

Or even:

constraint_value(
    name = 'ios_arm64',
    constraint_setting = "@bazel_tools//platforms:cpu",
)

And adding that constraints to the arch in list.bzl, it fails to properly select the correct target based on the ios_application I'm building, where config_setting works as expected.
I'm using the following command line

$ bazel build --ios_multi_cpus=arm64 //:sampleapp

This isn important because in the case of iOS one could have multiple versions of the c-archive built on different archs (armv7 and arm64).

Thanks!

@ianthehat
Copy link
Contributor

ios_arm64 is not a cpu...
I am not entirely sure what you want to achieve, but we already have all the constraints for every goos and goarch declared, along with platform declarations for all valid combinations, and then config_setting declarations to match. You should not need to declare any new ones.

I have no idea if what you are trying to do will work though, I think you are after toolchain transitions, which we would love to have but are not exposed to skylark right now.

@steeve
Copy link
Contributor Author

steeve commented Jan 22, 2018

I'm trying to embed a go_binary built as c-archive as a dependency to a cc_library, which is then imported into a ios_application and have goos and goarch transitively selected based on ios_application architecture (or multi architecture, for that matter).

It does work fine when I specifically select the goos/goarch to match the ios_application's arch via platform (in this case darwin_arm64).

Note that selecting (source files, for instance) on config_setting ios_arm64 properly works when using ios_application, even though it's not specified on the command line.

Indeed toolchain transitions sounds quite a bit like what's needed. However, I'm surprised it properly works when using config_setting and not via constraints...

@steeve
Copy link
Contributor Author

steeve commented Jan 22, 2018

For future record, Bazel reports this:

ERROR: No default_toolchain found for cpu 'foo'. Valid cpus are: [
  darwin,
  darwin_x86_64,
  k8,
  ios_x86_64,
  ios_i386,
  ios_armv7,
  ios_arm64,
  watchos_i386,
  watchos_armv7k,
  tvos_x86_64,
  tvos_arm64,
  armeabi-v7a,
]

@ianthehat
Copy link
Contributor

The cpu command line argument is not the same as the cpu constraint, the former is a hodgepodge of crazy values that only make sense to a CROSSTOOL (and the list varies depending on what crosstool is loaded), the latter has cpu and os correctly separated and is designed to work with toolchains and the constraint system (@bazel_tools//platforms:cpu and @bazel_tools//platforms:os)

I suspect the custom magic in the config transition logic for ios_application probably sets the effective value of the command line argument cpu for you, and thus selects based on that work, but they may have not got around to making the constraint system understand that transition, which would also mean that toolchain selection for anything you depend on would be broken. You would need to talk to the bazel team about that.

@steeve
Copy link
Contributor Author

steeve commented Jan 22, 2018

Very clear, thank you.
In this case, perhaps it would be possible to define the platform based on the config_setting?

That would be kind of a hack, but it might work.

@ianthehat
Copy link
Contributor

We used to, but it was seriously problematic...

There was no sane way to map config setting to GOOS and GOARCH values, and many things allowed for go cross compile were not valid as --cpu values.
You could not control the config setting from skylark, so we could not cross compile except by setting --cpu on the command line.
There were at least three separate values for --cpu that meant windows on x86_64, the same was true for some other cases, so it was not possible to write sane select statements.
The set of values is entirely controlled by the CROSSTOOL, so any decision we made about mapping to GOOS and GOARCH was broken for anyone writing their own one.

I think there were some other issues, I don't remember fully any more.

Instead we describe a complete system based on constraints, and trust bazel to map the --cpu value to constraints correctly for now. Hopefully even that part will go away soon when the cc toolchain selection is switched to also obey constraints and the platform stuff stops being experimental.

What we need is for the thing that does the config transition for the ios rules to correctly set the constraints in the same way it would on start up when picking them based on --cpu, and make sure toolchain resolution is run in the dependent rules correctly.

@steeve
Copy link
Contributor Author

steeve commented Jan 22, 2018

Is there any way to define a constraint based on something else than internal Bazel state and platform?
In a nutshell I could detect the ios_arm64/ios_armv7 config_setting and manually set the goos/goarch constraints so that toolchain resolution works?

@steeve
Copy link
Contributor Author

steeve commented Jan 22, 2018

But actually, my only problem is because I'm trying to build a multi-arch artefact (namely an iOS app) and i'm not sure how to do that unless getting it via the cpu config_setting....

Apparently Bazel does propagate things such as target_gnu_system_name in the cpp fragment: https://github.com/znly/rules_go/commit/fce5ac2d1f8a18ae77712cd392c76b54cb03e2d8#diff-b1e7c11591fbac0890652d29a78d6222R55

Command line:

$ bazel build --ios_multi_cpus=armv7,arm64 //:sampleapp

@steeve
Copy link
Contributor Author

steeve commented Jan 22, 2018

Actually, I'm thinking i could hack it by selecting goos/goarch attrs in the go_binary rule maybe?

@ianthehat
Copy link
Contributor

That might work, but it also might cause you issues with caching, depending on how the config switch for ios was written.
Another (ugly) way to do this is to declare multiple go_binary rules that force the goos and goarch attributes, and then a select statement that picks up the right one(s) from the ios rule. Do you use cgo at all (it makes a big difference, because the goos/goarch attributes can't affect the cc toolchain so using them is fraught with peril if you have cgo)
Whatever you do, it's not the correct long term solution, so don't invest too much in it :)

I am really interested to know what you are using go for on ios, I did not realize anyone was using that!

@steeve
Copy link
Contributor Author

steeve commented Jan 22, 2018

I am indeed using cgo!

Our app (Zenly) has a common "on-device app backend" that is built almost entirely using Go and GoMobile, for both Android and iOS. We've been doing this for a year and half now with great success, but are now kind of constrained by the fact that gomobile handles the build part (which was great for a long time) by itself, limiting us in the way we can mix and match languages and libraries (for instance, using a patched/modified Go runtime for golang/go#22716).

That part is responsible for network interface (via gRPC), talking to the device APIs directly (CoreLocation for instance, hence my issue about objc support and https://github.com/znly/rules_go/commit/a9a23fb08c4b35be72c488e3300d18447f173f1f) and pushing this data to the UI via ReactiveX endpoints. We use protobuf to do trans-vm marshalling and ReactiveX for concurrency unification between Java/Swift and Go, which nice features such as a ReactiveX subscription becoming a context.Context on the Go side. We plan to open source this eventually.

Also, finally being able to have one build tool for apps on both platforms, backend, protobuf while also properly expressing the dependency graph cross language is really, really cool (for instance part of the Swift app depending on part of the Go backend etc...).

Hence all the commits we are doing in our fork of rules_go :)

EDIT: and I should also point out that we are re-implementing what gomobile bind does, only with Bazel. We have a working prototype for iOS already (save for that issue :))

@steeve
Copy link
Contributor Author

steeve commented Jan 22, 2018

Bouncing on your comment, I just tested the following, on Bazel master:

load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")

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

go_library(
    name = "go_default_library",
    srcs = [
        "main.go",
    ],
    cgo = True,
    importpath = "github.com/znly/cmd",
    visibility = ["//visibility:private"],
)

go_binary(
    name = "cmd",
    embed = [":go_default_library"],
    goos = "darwin",
    goarch = select({
        ":ios": "arm64",
        "//conditions:default": "amd64",
    }),
    visibility = ["//visibility:public"],
)

And Bazel fails with:

$ bazel build //cmd:cmd --cpu=ios_arm64 -s
DEBUG: /private/var/tmp/_bazel_steeve/e3cf18c019556143010ddd56b908ce94/external/io_bazel_rules_go/go/private/common.bzl:170:5:
Current Bazel is not a release version, cannot check for compatibility.
DEBUG: /private/var/tmp/_bazel_steeve/e3cf18c019556143010ddd56b908ce94/external/io_bazel_rules_go/go/private/common.bzl:172:5: Make sure that you are running at least Bazel 0.8.0.
Unhandled exception thrown during build; message: Unrecoverable error while evaluating node 'PACKAGE:cmd' (requested by nodes '//cmd:cmd')
INFO: Elapsed time: 0.103s
FAILED: Build did NOT complete successfully (0 packages loaded)
    currently loading: cmd
java.lang.RuntimeException: Unrecoverable error while evaluating node 'PACKAGE:cmd' (requested by nodes '//cmd:cmd')
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:414)
	at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:355)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.IllegalArgumentException: wrong type for attribute "goarch" in go_binary rule //cmd:cmd
	at com.google.devtools.build.lib.packages.AbstractAttributeMapper.get(AbstractAttributeMapper.java:70)
	at com.google.devtools.build.lib.packages.SkylarkDefinedAspect.lambda$getDefaultParametersExtractor$0(SkylarkDefinedAspect.java:190)
	at com.google.devtools.build.lib.packages.Attribute$SkylarkRuleAspect.getAspect(Attribute.java:131)
.....

@ianthehat
Copy link
Contributor

I have been doing a lot of work to get to the point where the link modes will work properly, I see you are piggybacking of most of it.
I was focused on plugin (although I have hit a wall with compiling the standard library ready for that mode) but c-archive and c-shared are in the list of things I was planning to add, it may collide with your changes, if you let me know how you use them I can try to make sure we are compatible.

goarch has to be a plain string, because of the way it propagates onto an aspect.
My suggestion of using multiple go_binary rules and doing the select outside will still work (you could either do it on the ios rule or in a filegroup between the two)

@steeve
Copy link
Contributor Author

steeve commented Jan 22, 2018

What do you think I open a PR for link modes and have that conversation there?
There are tons of questions I would have loved to ask but I was just getting a hang of a Bazel itself so I thought I would investigate before.

I still have to rebase on the stdlib builder (not sure if it's really that big of a deal, though).

The more we upstream, the better.

@steeve
Copy link
Contributor Author

steeve commented Jan 22, 2018

The multiple target option seems to be working ok!
However, it somehow tries to build for darwin/amd64 for each binary, resulting in a failure because the cc toolchain is targeted to iOS:

  /bin/bash -c 'export GOROOT="$(pwd)/bazel-out/ios-arm64-min10.0-applebin_ios-ios_arm64-fastbuild/bin/external/go_stdlib_darwin_amd64_cgo" GOROOT_FINAL="GOROOT" GOOS="darwin" GOARCH="amd64" CGO_ENABLED="1" CC="$(pwd)/external/local_config_cc/cc_wrapper.sh" CXX="$(pwd)/external/local_config_cc/cc_wrapper.sh" COMPILER_PATH="/usr/bin" CGO_CPPFLAGS="-no-canonical-prefixes -Wno-builtin-macro-redefined -D__DATE__="redacted" -D__TIMESTAMP__="redacted" -D__TIME__="redacted" -target arm64-apple-ios -miphoneos-version-min=6.1" CGO_LDFLAGS="-headerpad_max_install_names -lc++ -no-canonical-prefixes -target arm64-apple-ios -miphoneos-version-min=6.1" SDKROOT="$(/usr/bin/xcrun -sdk iphoneos --show-sdk-path)" CGO_CFLAGS=" -miphoneos-version-min=6.1" APPLE_SDK_VERSION_OVERRIDE="11.1" APPLE_SDK_PLATFORM="iPhoneOS" && export PATH=$PATH:$(cd "$COMPILER_PATH" && pwd) && mkdir -p bazel-out/ios-arm64-min10.0-applebin_ios-ios_arm64-fastbuild/bin/external/go_stdlib_darwin_amd64_cgo/src && mkdir -p bazel-out/ios-arm64-min10.0-applebin_ios-ios_arm64-fastbuild/bin/external/go_stdlib_darwin_amd64_cgo/pkg && cp -rf external/go_sdk/src/* bazel-out/ios-arm64-min10.0-applebin_ios-ios_arm64-fastbuild/bin/external/go_stdlib_darwin_amd64_cgo/src/ && cp -rf external/go_sdk/pkg/tool bazel-out/ios-arm64-min10.0-applebin_ios-ios_arm64-fastbuild/bin/external/go_stdlib_darwin_amd64_cgo/pkg/ && cp -rf external/go_sdk/pkg/include bazel-out/ios-arm64-min10.0-applebin_ios-ios_arm64-fastbuild/bin/external/go_stdlib_darwin_amd64_cgo/pkg/ && external/go_sdk/bin/go install -asmflags "-trimpath $(pwd)" -tags=ios std && external/go_sdk/bin/go install -asmflags "-trimpath $(pwd)" -tags=ios runtime/cgo')

Perhaps I messed something up in https://github.com/znly/rules_go/commit/fce5ac2d1f8a18ae77712cd392c76b54cb03e2d8 or https://github.com/znly/rules_go/commit/344d1337b0350ea5e3b9e712437d38f33054d8a7 ?

$ bazel build --ios_multi_cpus=armv7,arm64 //:bindlib.framework
........
    GoStdlib external/go_stdlib_darwin_arm_cgo/src; 4s darwin-sandbox
    GoStdlib external/go_stdlib_darwin_amd64_cgo/src; 4s darwin-sandbox
    GoStdlib external/go_stdlib_darwin_arm_cgo/src; 4s darwin-sandbox
    GoStdlib external/go_stdlib_darwin_amd64_cgo/src; 4s darwin-sandbox

@ianthehat
Copy link
Contributor

You might want to update to the latest stdlib builder, it makes sure the stdlib is only ever built by aspect propagation.
Make sure you set tags = ["manual"] on the go_binary rules so they don't appear in globs

@steeve
Copy link
Contributor Author

steeve commented Jan 23, 2018

Thanks Ian i'm making great progress.

I just rebased master with the new stdlib builder (much cleaner, by the way) and most of the meat is at https://github.com/znly/rules_go/commit/c6ba7f617f624c0cc2b7b15031719ea185e56a4b

Now the stdlib bootstraps fine and I can even build a sample darwin_arm64 hello world binary with:

$ bazel build //cmd:cmd -s --cpu=ios_arm64
$ file bazel-bin/cmd/darwin_arm64_stripped/cmd
bazel-bin/cmd/darwin_arm64_stripped/cmd: Mach-O 64-bit executable arm64

With the following BUILD:

go_library(
    name = "go_default_library",
    srcs = [
        "main.go",
    ],
    cgo = False,
    importpath = "github.com/znly/cmd",
    visibility = ["//visibility:private"],
)

go_binary(
    name = "cmd",
    embed = [":go_default_library"],
    goos = "darwin",
    goarch = "arm64",
    visibility = ["//visibility:public"],
)

However, if I set cgo=True, it will try to bootstrap an amd64 toolchain, which will fail because the target is ios_arm64 (with the SDK set to iPhoneOS).

The second weird thing is that with cgo=False it will build the following toolchain:

GoStdlib external/go_stdlib_darwin_arm64_cgo/pkg

Instead of pure. Not that I really care for my use case, but it seems weird don't you think?

Here is the subcommand:

SUBCOMMAND: # @go_stdlib_darwin_amd64_cgo//:go_stdlib_darwin_amd64_cgo [action 'GoStdlib external/go_stdlib_darwin_amd64_cgo/pkg']
(cd /private/var/tmp/_bazel_steeve/e3cf18c019556143010ddd56b908ce94/execroot/__main__ && \
  exec env - \
    APPLE_SDK_PLATFORM=iPhoneOS \
    APPLE_SDK_VERSION_OVERRIDE=11.1 \
  bazel-out/host/bin/external/io_bazel_rules_go/go/tools/builders/darwin_amd64_stripped/stdlib -go external/go_sdk/bin/go -root_file external/go_sdk/packages.txt -goos darwin -goarch amd64 '-cgo=1' -tags ios -compiler_path /usr/bin -cc external/local_config_cc/cc_wrapper.sh -cpp_flag -no-canonical-prefixes -cpp_flag -Wno-builtin-macro-redefined -cpp_flag '-D__DATE__="redacted"' -cpp_flag '-D__TIMESTAMP__="redacted"' -cpp_flag '-D__TIME__="redacted"' -cpp_flag -target -cpp_flag arm64-apple-ios -cpp_flag '-miphoneos-version-min=6.1' -ld_flag -headerpad_max_install_names -ld_flag -lc++ -ld_flag -no-canonical-prefixes -ld_flag -target -ld_flag arm64-apple-ios -ld_flag '-miphoneos-version-min=6.1' -out bazel-out/ios_arm64-fastbuild/bin/external/go_stdlib_darwin_amd64_cgo)
[1 / 12] GoStdlib external/go_stdlib_darwin_amd64_cgo/pkg; 4s darwin-sandbox```
ERROR: /private/var/tmp/_bazel_steeve/e3cf18c019556143010ddd56b908ce94/external/go_stdlib_darwin_amd64_cgo/BUILD.bazel:4:1: GoStdlib external/go_stdlib_darwin_amd64_cgo/pkg failed (Exit 1)
# runtime/cgo
gcc_darwin_amd64.c:43:16: error: unknown token in expression
                asm volatile("movq %%gs:0x8a0, %0" : "=r"(x));

It fails because it is set to use the iPhoneOS sdk.

@steeve
Copy link
Contributor Author

steeve commented Jan 23, 2018

It seems goos/goarch are not properly propagated to the .cgo_* targets generated inside cgo.bzl. So it builds them for the current platform.

@steeve
Copy link
Contributor Author

steeve commented Jan 23, 2018

Indeed, the go_context created in the cgo.bzl rules does not have the pre-set goos/goarch from the go_binary but from the host itself.

I tried giving it manually to setup_cgo_library but no dice so far.

@ianthehat
Copy link
Contributor

The goos and goarch of the go_binary are collected onto the aspect, and then the aspect propagates across all the go rules, all the way down to the standard library.
The cgo stuff is not part of that process at the moment, what is it you are doing that would require it to know the goos and goarch? If it needs to then it has to become a GoLibrary->GoSource transformer so that the aspect causes the work to happen.

The rules also add a default output outside the aspect for the host, so you can actually build a go_library directly (it would be really annoying if you could not build a library from the command line), but if the rule is not built directly and that output is not used, it won't be built.

It does always have to build a copy of the standard library in host mode in order to build the tools it uses, maybe you are seeing that?

@steeve
Copy link
Contributor Author

steeve commented Jan 23, 2018

Indeed I am seeing the host stdlib being built successfully (and bazel marks it as the host, too).

When building as c-archive, cgo needs to be enabled to export functions from the main package. Also, I am calling directly ios apis via the objc support.

Thanks for being so fast!

@steeve
Copy link
Contributor Author

steeve commented Jan 23, 2018

I tried digging into the aspect part to add it to CGo but failed to see how it propagates in the go_context ultimately
Can you elaborate on the GoLibrary->GoSource transformation you're talking about ?

@steeve
Copy link
Contributor Author

steeve commented Jan 24, 2018

Ok so I found the reason: 2 cgo targets do not get goos/goarch properly set:

  • .cgo_codegen
  • .cgo_import

However, .cgo_embed being a regular GoLibrary, its aspect is properly set via embed attribute.

@ianthehat
Copy link
Contributor

What is it that actually goes wrong and needs the goos and goarch on those targets?
I was presuming that the cgo codegen step did not need to know, but that could be wrong.

@steeve
Copy link
Contributor Author

steeve commented Jan 24, 2018

Well apparently since they do reference the stdlib, it triggers a build for the stdlib for the wrong target since it falls back to auto for those two rules.

And because cc is set to the target arch, stdlib building fails when building CGo.
Discarding the fact that it's not needed, since cgo_codegen needs the stdlib and generates code for the target, I would assume it would be best for it to get the proper mode?

@steeve
Copy link
Contributor Author

steeve commented Jan 24, 2018

Instead of trying to bend go_archive_aspect I'm thinking perhaps of creating a new aspect for those two rules ?

@ianthehat
Copy link
Contributor

I am not sure why the cgo_codegen would need the stdlib at all, in theory it should be happy with the bootstrap mode that uses the non target specific form.
I could try switching the rules over to see if fixes it, but I would need a simple way to repro the problem so I could see if it helps.

@steeve
Copy link
Contributor Author

steeve commented Jan 24, 2018

Sure, here is a sample project: https://github.com/znly/rules_go_ios_example

I'm using our fork, specifically this branch: https://github.com/znly/rules_go/tree/misc/newstdlib
Also https://github.com/znly/rules_go/commit/c6ba7f617f624c0cc2b7b15031719ea185e56a4b may be of importance to you to understand what changes (basically detect the ios target and set the proper env vars).

You need a Mac with Xcode and iOS SDKs installed, of course.

Basically, if I add goos/goarch attrs to those two rules (and set default to arm64), the stdlib and target properly builds.

@steeve
Copy link
Contributor Author

steeve commented Jan 24, 2018

Also, I think if this is fixed cross-compilation + cgo would work just fine (assuming a proper crosstool for cc targets).

@steeve
Copy link
Contributor Author

steeve commented Jan 27, 2018

Quick update: I've tried referencing only the bootstrap toolchain (via go_rule bootstrap param), for cgo_import rule for instance, but it does fail also, because the builders (at least cgo tool) is not available in bootstrap mode.

I've been thinking about a way to properly propagate mode to those rules but fail to see how. Would you have any pointers as to how would be the course of action to take?

Many thanks

ianthehat added a commit to ianthehat/rules_go that referenced this issue Jan 31, 2018
This removes the hard coded list of standard libraries, and builds it per aspect
being propagated.
This is both cleaner and a pre-requisite for the additional link modes to work.

Related to bazel-contrib#1264 and bazel-contrib#54 and bazel-contrib#539
ianthehat added a commit that referenced this issue Feb 2, 2018
* Build stdlib from inside the aspect

This removes the hard coded list of standard libraries, and builds it per aspect
being propagated.
This is both cleaner and a pre-requisite for the additional link modes to work.

Related to #1264 and #54 and #539

* review feedback
@steeve
Copy link
Contributor Author

steeve commented Feb 2, 2018

Thanks for #1295, however, it seems to break c-archive link mode (stdlib gets' built twice).
I did some cleaning on our fork, too. Here is the updated commit for c-archive/c-shared: https://github.com/znly/rules_go/commit/c9eae8da9c801b0d9f8e791d6487630916298955

@steeve
Copy link
Contributor Author

steeve commented Feb 2, 2018

@ianthehat
Copy link
Contributor

It will build the stdlib once per build mode now, so it could be built twice, but that should not break anything. When you say breaks, what do you mean?

@steeve
Copy link
Contributor Author

steeve commented Feb 2, 2018

Oh sorry for the wording: I meant break when doing cgo-enabled cross compilation since the mode (goarch/goos) is now wrong compared to what the CROSSTOOL is doing.

However, I do believe that the fact that it does build the stdlib twice (once with the wrong mode) as something to do with it.

@ianthehat
Copy link
Contributor

What is the breakage?
If you look in my plugin branch you can see what I was doing towards making plugin mode work, but I at the moment the error you get is

.init: reloc 26 to non-macho symbol .initdone· type=32
.init: unsupported obj reloc R_GOTPCREL/4 to .initdone·
.init: reloc 26 to non-macho symbol .initdone· type=32
.init: unsupported obj reloc R_GOTPCREL/4 to .initdone·

which I believe is because the standard library is built in the wrong mode, and I cannot find any way to build it in the right mode.

@steeve
Copy link
Contributor Author

steeve commented Feb 2, 2018

I kind of remember seeing that building the stdlib itself as c-shared does not work. Although it's not the same as plugin, could it be related? golang/go#13234

@steeve
Copy link
Contributor Author

steeve commented Feb 2, 2018

The breakage seems to be that somewhere goos/goarch is lost, so go_context falls back to auto (which would be darwin_amd64 in my case instead of darwin_arm64+ios)

In the case of c-archive, since the linkmode is added to the mode string, and I see the stdlib being built with normal, I believe this is related => the mode is lost somewhere:

   if not result or not mode.link == LINKMODE_NORMAL:
     result.append(mode.link)
     result.append(mode.link)

@steeve
Copy link
Contributor Author

steeve commented Feb 2, 2018

In my case I'm building the binary with

go_binary(
    name = "rules_go_example",
    embed = [":go_default_library"],
    linkmode = "c-archive",
    visibility = ["//visibility:public"],
)

@ianthehat
Copy link
Contributor

Yes, exactly, you cannot build the stdlib with the special link modes, because they require you to be building the main package, and there does not seem to be any invocation I can find that would build the stdlib in the same way those modes would do, short of actually compiling a binary that just happened to use the entire standard library....

The mode is lost inside cgo rules, I spent a while looking at it during the week and I don't see how to fix that because it can't get through the cc_library and cc_binary rules that we have to add in. The question is what does it actually break, and is there anything we can do about the breakage (I have not reproduced your problem, I am not set up to build ios apps).

@steeve
Copy link
Contributor Author

steeve commented Feb 8, 2018

Basically, what it does break is cross-compilation using cgo via goos/goarch.

In my case, I have a target:

go_binary(
    name = "rules_go_ios_example",
    embed = [":go_default_library"],
    goos = "darwin",
    goarch = "arm64",
    visibility = ["//visibility:public"],
)

Also, --cpu=ios_arm64 is added to ensure the CROSSTOOL is the iOS one.

Indeed if I build without cgo, the target is properly bootstrapped and built, but with CGo it will somehow try to build the stdlib for amd64, which will fail (because bazel is using the iOS CROSSTOOL).

It is also important to add and I've manually added goos/goarch select clauses to .cgo_codegen and .cgo_import (https://github.com/znly/rules_go/commit/69fc087d0cecd1b779690a3420a8bb7e2966bfb8) so that at least the mode would be correct inside those rules (perhaps it could be done by an aspect but I'm too newbie on that front).

@steeve
Copy link
Contributor Author

steeve commented Feb 8, 2018

Here is the subcommand which fails because goos/goarch (darwin/amd64) is wrong with respect to clang's -target (arm64-apple-ios):

(cd /private/var/tmp/_bazel_steeve/98e33bdfb98ea6347eb98dad1c62e218/execroot/__main__ && \
  exec env - \
    APPLE_SDK_PLATFORM=iPhoneOS \
    APPLE_SDK_VERSION_OVERRIDE=11.2 \
  bazel-out/host/bin/external/io_bazel_rules_go/go/tools/builders/darwin_amd64_stripped/stdlib -go external/go_sdk/bin/go -root_file external/go_sdk/packages.txt -goos darwin -goarch amd64 '-cgo=1' -tags ios -compiler_path /usr/bin -cc external/local_config_cc/cc_wrapper.sh -cpp_flag -no-canonical-prefixes -cpp_flag -Wno-builtin-macro-redefined -cpp_flag '-D__DATE__="redacted"' -cpp_flag '-D__TIMESTAMP__="redacted"' -cpp_flag '-D__TIME__="redacted"' -cpp_flag -target -cpp_flag arm64-apple-ios -cpp_flag '-miphoneos-version-min=6.1' -ld_flag -headerpad_max_install_names -ld_flag -lc++ -ld_flag -no-canonical-prefixes -ld_flag -target -ld_flag arm64-apple-ios -ld_flag '-miphoneos-version-min=6.1' -out 'bazel-out/ios_arm64-fastbuild/bin/external/io_bazel_rules_go/darwin_amd64_stripped/stdlib~')

@steeve
Copy link
Contributor Author

steeve commented Feb 8, 2018

Also, I don't get why the mode would be lost in my case, as I'm actively passing goos and goarch and when I print the mode inside the rule, it seems fine:

  cgo_goos = select(CGO_GOOS)
  cgo_goarch = select(CGO_GOARCH)

  cgo_codegen_name = name + ".cgo_codegen"
  _cgo_codegen(
      name = cgo_codegen_name,
      srcs = srcs,
      deps = cdeps,
      copts = copts,
      linkopts = clinkopts,
      goos = cgo_goos, <-----
      goarch = cgo_goarch, <-----
      visibility = ["//visibility:private"],
  )

So I don't see why select_c_files and select_main_c would somehow lose the mode

@ianthehat
Copy link
Contributor

So the goos and goarch attribute affect the aspect that compiles go code. They cannot change the c compiler, and the aspect does not travel across cc_library and cc_binary rules, so using them to cross compile cgo is never going to work, and there is really nothing I can do about that.

What you are talking about I think is a special case however, where the c compiler has been switched for you by the apple rules (and is thus not the default target platform), and you are just forcing the go aspect to match it, which is not quite the same thing.

The problem in this case is that in order to compile cgo, we have to have a cc_library and cc_binary rule in the chain, which again the aspect cannot travel across. The inputs to those rules come from the cgo_codegen step (because that's how we make the c files being passed in) and thus the cgo_codegen happens without the benefit of the aspect, and thus does not know what the target goos and goarch are, so cgo_codegen always happens for the default platform as specified on the command line. I cannot think of any way to fix this until we get the ability to generate cc actions (rather than rules) so that we have control over them and can transit through them with the aspect.

@steeve
Copy link
Contributor Author

steeve commented Feb 8, 2018

Agreed, however, I am manually setting goos and goarch attrs (which I've added) to cgo_codegen rule and its dependencies to match my target platform (https://github.com/znly/rules_go/commit/69fc087d0cecd1b779690a3420a8bb7e2966bfb8) so I while I do understand that the aspect does not go through them, the other side is already in the proper mode (at least goos/goarch).

@steeve
Copy link
Contributor Author

steeve commented Feb 8, 2018

I did find that removing the select_main_c and cgo_lib_name (which of course fails) does not indeed triggers the wrong stdlib bootstrap, but I since cgo_codegen's mode is manually set by me (and when I print it the goos/goarch are fine), I don't get why it would generate this behaviour

@steeve
Copy link
Contributor Author

steeve commented Feb 10, 2018

Ok I got it to work.

I'm not sure it's upstreamable (although it shouldn't break anything), but I'm now able to fully build for iOS with it: https://github.com/znly/rules_go/commit/6481b91631d2eeeae9930baf3832835658641441

Basically I'm manually setting goos/goarch for the stdlib rule via config_setting/select. Not that great, but it gets the job done and shouldn't break anybody.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants