Various fixes and changes #1340

steeve · 2018-02-21T14:23:36Z

This PR is a mix of various changes and fixes we've made on https://github.com/znly/rules_go that I think are upstream-able first. Of course we can split and shrink if you guys want.

Here goes:
36bc7cc: Add runfiles to GoArchiveData and use them in the GoPath
Some binaries (such as https://github.com/golang/mobile/tree/master/cmd/gobind) rely on runtime files to be in the GOPATH

060c171: Expose GoPath provider
Title says it all. Useful for custom rules.

f59a93d: Add tags and environment to go_context
Title says it all.

e4a157e: Expose GoSource provider
Title says it all. Useful for custom rules.

7d1d956: Make the Go environment an update of the OS environment
This ensures that the Go environment also has the OS environment keys.

f789bcd: Propagate build tags when go installing stdlib
std has a few tags here and there (for instance crypto/x509 needs the ios tag to build on darwin_arm64). This makes sure they are propagated.

93ce009: Make stdlib building depend on CROSSTOOL (for runtime/cgo)
runtime/cgo needs the C compiler, and it's not declared as a dependency. As a result, when using a different CROSSTOOL (such as Android), it will fail to find the compiler (clang).

aad1ce3: extras/bindata: make it executable and run on host
Needed to use cross compilation (via --platforms) and bindata.

bazel-io · 2018-02-21T14:23:38Z

Can one of the admins verify this patch?

googlebot · 2018-02-21T14:23:39Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

zllak · 2018-02-21T14:28:12Z

I am the author of one of the commits, it's part of a join effort with @steeve 👍

steeve · 2018-02-21T14:54:20Z

go/tools/builders/env.go

+		k, v := pair[0], pair[1]
+		retEnvMap[k] = v
+	}
+	for _, e := range new {


i should not use new as a var name

jayconrod

Sorry for the delay in review. Mostly looks good, but I have a few questions and suggested changes.

There are also a few test failures. You can run our CI tests with bazel test //....

@ianthehat may also have some comments.

jayconrod · 2018-02-22T19:42:39Z

go/private/actions/archive.bzl

@@ -86,6 +86,7 @@ def emit_archive(go, source=None):
      file = out_lib,
      srcs = as_tuple(source.srcs),
      searchpath = searchpath,
+      runfiles = runfiles,


Instead of putting runfiles in GoArchiveData, I think it would be better to use the runfiles already in GoArchive or in DefaultInfo.

The reason for the split between GoArchive and GoArchiveData is avoiding O(n^2) blowup when accumulating information from a large graph of dependencies. GoArchiveData contains information about a single archive. GoArchive contains information about an archive and all of its transitive dependencies. runfiles is a better fit in GoArchive for this reason.

sounds good

jayconrod · 2018-02-22T19:46:50Z

go/private/tools/path.bzl

@@ -75,7 +75,7 @@ Found {} in
    seen_libs[golib.importpath] = golib
    package_files = []
    prefix = "src/" + golib.importpath + "/"
-    for src in golib.srcs:
+    for src in as_iterable(golib.srcs) + tuple(as_iterable(golib.runfiles.files)):


Rather than modifying this loop, create a runfiles object above (where golibs is accumulated) and merge in the runfiles from each archive. Then iterate over that in a separate loop.

the thing is that i need the importpath to put them in the fake gopath properly, so keeping the archives is rather useful

Hmm, makes sense. There needs to be some association between the import path and the data files, but we need to avoid placing runfiles in GoArchiveData to avoid a blowup in data structure size and time to iterate.

Instead of adding runfiles to GoArchiveData, I'd suggest adding a data field, which will be a tuple of data files just for that archive. This will be essentially the same format as srcs. You can get this tuple in emit_archive by iterating source.runfiles before it's merged.

jayconrod · 2018-02-22T20:05:45Z

go/private/context.bzl

@@ -74,6 +74,7 @@ def _new_args(go):
      "-goarch", go.mode.goarch,
      "-cgo=" + ("0" if go.mode.pure else "1"),
  ])
+  args.add(go.tags, before_each = "-tags")


env.go expects -tags to be a comma separated list. It can't be repeated. I think args.add(go.tags, join_with=",") would be correct.

good call! thanks

jayconrod · 2018-02-22T20:07:10Z

go/private/context.bzl

@@ -267,13 +270,17 @@ def _go_context_data(ctx):
  linker_options = [o for o in raw_linker_options if not o in [
    "-Wl,--gc-sections",
  ]]
+  env = {}


What would go here in the future? We generally want to avoid pulling anything out of the user's environment, since that makes builds less reproducible.

this is not about pulling the user's environment, but rather merging the rule environment with the builder's or go install based rules such as stdlib

Ok, that makes sense.

jayconrod · 2018-02-22T20:09:26Z

go/private/context.bzl

@@ -267,13 +270,17 @@ def _go_context_data(ctx):
  linker_options = [o for o in raw_linker_options if not o in [
    "-Wl,--gc-sections",
  ]]
+  env = {}
+  tags = []


I think the best way to populate this is by reading ctx.var["gotags"] or something similar. Users could set tags with --define=gotags=foo,bar,baz on the command line or in an rc file.

this could be added later, but the main case for me to use it since the builders can too.
also, when building for ios, both tags and env need to be properly set up, and in that case I'm doing it from inside the rules themselves

actually ctx.var["gotags"] or [] is rather easy to do, i'm sold

jayconrod · 2018-02-22T20:14:10Z

go/def.bzl

@@ -23,6 +23,8 @@ load(
 load(
    "@io_bazel_rules_go//go/private:providers.bzl",
    _GoLibrary = "GoLibrary",
+    _GoSource = "GoSource",
+    _GoPath = "GoPath",


Feel free to expose GoArchive and GoArchiveData, too. They're documented and fairly stable at this point.

jayconrod · 2018-02-22T20:24:13Z

go/tools/builders/env.go

+		pair := strings.SplitN(e, "=", 2)
+		k, v := pair[0], pair[1]
+		if preV := retEnvMap[k]; preV != "" {
+			retEnvMap[k] = preV + " " + v


This scares me. Seems like we could end up with GOOS=linux linux or something. Why is this needed?

this is true. however, both ends (old and new) are controlled by rules_go. But in the case of CGO_CFLAGS for instance, we could get to the case when it would get overridden instead of merged

by both end I mean the skylark and go builder side

Ok, I think I understand this better. I'm still concerned that it's prone to duplication. I don't think there's a situation where we actually need to merge flags in the same environment variable. Here's what I think we should do:

Any value that any builder needs to handle explicitly should be passed with a command line argument. For example, the builders perform filter on build tags, so they need to know GOARCH and GOOS. The builder will add these to the environment before executing an SDK tool.

Any value that all builders should simply pass through to executed programs should be an environment variable.

There shouldn't be any overlap between these two categories, so env.go can continue doing what it did before.

jayconrod · 2018-02-22T20:29:07Z

go/tools/builders/stdlib.go

@@ -24,13 +24,14 @@ import (
 )

 func install_stdlib(goenv *GoEnv, target string, args []string) error {
+	if goenv.tags != "" {
+		args = append(args, "-tags", goenv.tags)


You mentioned ios in the description. I wonder if there's a way we can set that one automatically?

At the moment the only way I found to reliably do it is by using ctx.fragments.cpp.target_gnu_system_name.

Here is the gist of the code, more or less (I plan to submit this later on):

PLATFORMS = { 'armv7-apple-ios': apple_common.platform.ios_device, 'arm64-apple-ios': apple_common.platform.ios_device, 'i386-apple-ios': apple_common.platform.ios_simulator, 'x86_64-apple-ios': apple_common.platform.ios_simulator, } def apple_ensure_options(ctx, env, tags, compiler_options, linker_options): system_name = ctx.fragments.cpp.target_gnu_system_name platform = PLATFORMS.get(system_name) if platform == None: return if system_name.endswith("-ios"): tags.append("ios") # needed for stdlib building if platform in [apple_common.platform.ios_device, apple_common.platform.ios_simulator]: min_version = _apple_version_min(platform, "6.1") compiler_options.append(min_version) linker_options.append(min_version) xcode_config = ctx.attr._xcode_config[apple_common.XcodeVersionConfig] env.update(apple_common.target_apple_env(xcode_config, platform))

As you can see also, this method modifies the environment, and in this case to tell the compiler which SDK to use (iPhoneOS, iPhoneSimulator).

For instance:

APPLE_SDK_PLATFORM=iPhoneOS APPLE_SDK_VERSION_OVERRIDE=11.2 XCODE_VERSION_OVERRIDE=9.2.0

These also need to be set at stdlib bootstrapping.

Thanks for explaining. That makes more sense now.

steeve · 2018-02-22T21:04:15Z

thanks for the comments! i'll be updating it when I get the chance

steeve · 2018-02-24T18:30:01Z

I've updated the PR with the gotags suggestions 99f11a3.

I have all the tests passing. I fixed them with e42f021

steeve · 2018-02-24T20:53:08Z

Changed the go-bindata commit message to be closer to the ones here.

steeve · 2018-02-24T23:00:06Z

Added 940d41b which splits sources via a dict instead of using any + lists.
It improves splitting performance by about 6-7x. As this is kind of a hot path, hopefully it should yield noticeable improvements.

I benched it with this code: https://gist.github.com/steeve/56d3f107f24d8e67833f66ed3e06acdd

old: [8.00760793685913, 8.148405075073242, 8.099946975708008]
new: [1.3760578632354736, 1.4115018844604492, 1.4008851051330566]

steeve · 2018-02-24T23:12:29Z

Added two more commits that ensure the proper dependency chain for CGo and makes sure that all compiler options are used (some were missed when cross compiling)

steeve · 2018-02-27T15:22:08Z

I'm going to leave out the gopath commit for now so as to not block the PR with this. I'll open it on another PR.

jayconrod · 2018-02-27T18:30:14Z

+1 to moving gopath to a separate PR. It will be easier to review and merge in pieces.

My only remaining comment is this one from d73858f. GitHub's code review system confuses me, and I think it got buried in a rebase somewhere. I'll copy it here:

Ok, I think I understand this better. I'm still concerned that it's prone to duplication. I don't think there's a situation where we actually need to merge flags in the same environment variable. Here's what I think we should do:

Any value that any builder needs to handle explicitly should be passed with a command line argument. For example, the builders perform filter on build tags, so they need to know GOARCH and GOOS. The builder will add these to the environment before executing an SDK tool.

Any value that all builders should simply pass through to executed programs should be an environment variable.

There shouldn't be any overlap between these two categories, so env.go can continue doing what it did before.

steeve · 2018-02-27T18:32:18Z

To be clear, what you're advising is that cgo_ldflags should be set specifically in the case of the stdlib bootstrap (it is set via a env now). SGTM

jayconrod · 2018-02-27T19:16:44Z

My concern was about that we shouldn't do environment merging in env.go. It would be better to either set an environment variable to be passed through without modification to the toolchain, or to provide a command line argument to the builders (that maybe gets turned into an environment variable) but not both.

I guess CGO_LDFLAGS / -ld_flag is actually both right now. That's something I'm not happy with, but there's no need to fix that in this PR.

steeve · 2018-02-27T19:34:13Z

I'm going to go with the -ld_flags from env.go (at least for stdlib.bzl) because, well, it's there and setting CGO_LDFLAGS from the rule doesn't really make sense actually, imho

steeve · 2018-02-28T12:36:00Z

Ok so what I'm going to do is if there is a conflict between the old (os.Environ() and the new env.env()), then the new will take precedence.
Effectively working like dict.update()

steeve · 2018-02-28T13:26:19Z

I've updated the branch with your suggestions in 840fda3 and 5cda48b, so far so good

steeve · 2018-02-28T13:26:40Z

I've added 112739d for a very quick fix by disabling linking with pthreads on all darwins, not only amd64

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

steeve · 2018-02-28T13:47:43Z

Also added aa59cc9 which ensures go-bindata doesn't use the file's modtime unless otherwise specified. This would make our builds no hermetic when using it.

steeve · 2018-02-28T13:48:26Z

And rebased on master

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

steeve · 2018-02-28T14:23:32Z

Added dc60119 to add some parameters and extra_args to the go-bindata rule

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

steeve · 2018-02-28T14:58:32Z

Added 31222ea which add the field_mask well known type to gogo remaps

Sorry I keep adding commits but I figure given their scopes that now is a good time to do it

jayconrod · 2018-02-28T15:58:29Z

Ok, I think everything looks good at this point. Thanks!

steeve requested review from ianthehat and jayconrod as code owners February 21, 2018 14:23

googlebot added the cla: no label Feb 21, 2018

steeve commented Feb 21, 2018

View reviewed changes

jayconrod self-assigned this Feb 21, 2018

jayconrod reviewed Feb 22, 2018

View reviewed changes

jayconrod mentioned this pull request Feb 26, 2018

go_binary: support build constraints via build_setting #1351

Closed

zllak and others added 6 commits February 28, 2018 14:37

Switch go-bindata to host configuration and executable

ef06bc1

Make stdlib building depend on crosstool (for runtime/cgo)

4b26b72

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

Propagate build tags when go installing stdlib

0394596

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

Make the Go environment an update of the OS environment

9f25dd0

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

Pass CGoCodeGen CGO_LDFLAGS via the builder's ld_flag

2f33644

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

Expose GoSource, GoPath, GoArchive and GoArchiveData providers

b8dca56

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

steeve added 8 commits February 28, 2018 15:17

Add tags and environment to go_context

863d053

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

Enforce debug symbols generation in CGo

29f56bb

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

Splits sources with a dict

bfdeb73

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

Use all compiler options and improve blacklist performance

1d11bde

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

Make CGo Codegen depend on CROSSTOOL and use all compiler options

8948583

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

Do not link with pthreads on all darwin

e13288b

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

Ensure go-bindata doesn't use modtime unless specified

2d0c60d

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

Add memcopy and extra_args parameters to go-bindata

dc60119

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

Add field_mask to proto gogo remaps

31222ea

Signed-off-by: Steeve Morin <steeve.morin@gmail.com>

jayconrod approved these changes Feb 28, 2018

View reviewed changes

jayconrod merged commit 9527696 into bazel-contrib:master Feb 28, 2018

zllak deleted the fix/various branch February 28, 2018 16:09

Various fixes and changes #1340

Various fixes and changes #1340

Conversation

steeve commented Feb 21, 2018 • edited Loading

bazel-io commented Feb 21, 2018

googlebot commented Feb 21, 2018

zllak commented Feb 21, 2018

Choose a reason for hiding this comment

jayconrod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steeve Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steeve commented Feb 22, 2018

steeve commented Feb 24, 2018 • edited Loading

steeve commented Feb 24, 2018

steeve commented Feb 24, 2018 • edited Loading

steeve commented Feb 24, 2018

steeve commented Feb 27, 2018

jayconrod commented Feb 27, 2018

steeve commented Feb 27, 2018

jayconrod commented Feb 27, 2018

steeve commented Feb 27, 2018 • edited Loading

steeve commented Feb 28, 2018

steeve commented Feb 28, 2018

steeve commented Feb 28, 2018 • edited Loading

steeve commented Feb 28, 2018 • edited Loading

steeve commented Feb 28, 2018

steeve commented Feb 28, 2018

steeve commented Feb 28, 2018

jayconrod commented Feb 28, 2018 • edited Loading

steeve commented Feb 21, 2018 •

edited

Loading

steeve Feb 22, 2018 •

edited

Loading

steeve commented Feb 24, 2018 •

edited

Loading

steeve commented Feb 24, 2018 •

edited

Loading

steeve commented Feb 27, 2018 •

edited

Loading

steeve commented Feb 28, 2018 •

edited

Loading

steeve commented Feb 28, 2018 •

edited

Loading

jayconrod commented Feb 28, 2018 •

edited

Loading