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

Add build_flags per go_binary #5658

Merged
merged 7 commits into from Apr 9, 2018

Conversation

Projects
None yet
2 participants
@traviscrawford
Copy link
Member

traviscrawford commented Apr 4, 2018

Problem

Go support currently has a global build flags option that is added to
every go command invocation.

$ ./pants compile --help | grep build-flags -A1
--compile-go-build-flags=<str> (default: '')
    Build flags to pass to Go compiler.

Talking with users there is need for user-defined flags at the go_binary
target level (e.g.: -tags netgo).

Solution

Here we add support for per-target build_flags which are merged with compile-go-build-flags, handling the case where they were defined in the config file, or on the command-line.

Result

Users can now check build flags into their go_binary target definitions so there is no need for an external script to pass these args in at build time.

@@ -60,9 +60,25 @@ def execute(self):
lib_binary_map[vt.target] = os.path.join(gopath, 'pkg', self.goos_goarch,
vt.target.import_path + '.a')

@classmethod
def _split_build_flags(cls, build_flags):

This comment has been minimized.

@traviscrawford

traviscrawford Apr 4, 2018

Member

I'm happy to do this in-line too, though while figuring things out I found the tests helpful, so decided to leave it. I don't feel strongly about it though.

This comment has been minimized.

@jsirois

jsirois Apr 5, 2018

Member

I'm fine with it since it is in-fact tested and that serves as useful enough doc to hopefully save others time grokking this. That said, # Visible for testing is probably better than """Visible for testing""" since the triple quoted string implies API docs, and this is just a note to devs about a method already _private anyhow.

@@ -85,3 +85,31 @@ def test_sync_binary_dep_links_refreshes_links(self):
mtime = lambda t: os.lstat(os.path.join(os.path.join(a_gopath, 'pkg', t.address.spec))).st_mtime
# Make sure c's link was untouched, while b's link was refreshed.
self.assertLessEqual(mtime(c), mtime(b) - 1)

def test_split_build_flags_simple(self):

This comment has been minimized.

@traviscrawford

traviscrawford Apr 4, 2018

Member

Note that test_go_compile_fully_static already covers the command-line case, so no integration tests are added as part of this PR. That test was actually helpful though, since it exercises the case where we need that regex, which was initially unintuitive for me.

https://github.com/pantsbuild/pants/blob/master/contrib/go/tests/python/pants_test/contrib/go/tasks/test_go_compile_integration.py

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented Apr 4, 2018

Here's an example use-case. Let's say we want to always build a particular binary with some build flags, we can now define them in the target itself. Then, let's say we want to build in verbose mode for one invocation, we see both those sets of args being merged together:

Let's add some build_flags:

[root@949c89f79bef pants]# git diff
diff --git a/contrib/go/examples/src/go/server/BUILD b/contrib/go/examples/src/go/server/BUILD
index 0b9bf56..e9e292a 100644
--- a/contrib/go/examples/src/go/server/BUILD
+++ b/contrib/go/examples/src/go/server/BUILD
@@ -7,5 +7,6 @@ go_binary(
     'contrib/go/examples/3rdparty/go/golang.org/x/net:http2',
     'contrib/go/examples/3rdparty/go/google.golang.org/grpc',
     'contrib/go/examples/3rdparty/go/gopkg.in/yaml.v2',
-  ]
+  ],
+  build_flags='-tags netgo',
 )

And build:

./pants binary contrib/go/examples/src/go/server/ --compile-go-build-flags='-v'

And we can see the command-line was correctly merged:

[root@949c89f79bef pants]# grep tags /workspace/pants/.pants.d/reports/pants_run_2018_04_04_23_34_18_860_1c5fe13e26fb40a88212e34c814ac32f/html/build.html
pants.appendString('GOROOT=/root/.cache/pants/bin/go/linux/x86_64/1.8.3/go.tar/go GOPATH=/workspace/pants/.pants.d/compile/go/contrib.go.examples.src.go.server.server /root/.cache/pants/bin/go/linux/x86_64/1.8.3/go.tar/go/bin/go install -tags netgo -v server', '#3bfee0ba-b568-45c1-8234-cacd8e7be493-cmd-content');
[root@949c89f79bef pants]#

@jsirois
Copy link
Member

jsirois left a comment

Thanks @traviscrawford! This looks good to me mod the invalidation bug pointed out. That should be sorted before this lands.

@@ -65,3 +65,10 @@ def import_path(self):
:returns: An import path that can be used to import this package in a `.go` file.
:rtype: string
"""

def __init__(self, build_flags=None, **kwargs):

This comment has been minimized.

@jsirois

jsirois Apr 5, 2018

Member

It would be nice to keep __init__ above other instance methods in the file since we do this ~everywhere else I know of.

:param string build_flags: Build flags to pass to Go compiler.
"""
super(GoTarget, self).__init__(**kwargs)
self.build_flags = build_flags

This comment has been minimized.

@jsirois

jsirois Apr 5, 2018

Member

IIUC these should only be accepted for GoBinary targets and they should invalidate compiles (installs). There are 2 paths to invalidation:

  1. Include field into the default fingerprint of the target. An example is in GoLocalSource, although you just need add_field singular + a PrimitiveField.
  2. Use a custom FingerprintStrategy in the task(s) that cares about this field. A FingerprintEStrategy example is here and and you'd pass it in as the custom fingerprint_strategy in the GoCompile invalidated block.

I think you want 2 here since build flags are only used by GoCompile now and conceivably forever. In addition, the fingerprinting probably ought to be over the final build_flags you compose in GoCompile since order matter in the case of dups. This probably also means taking the fingerprint=True off the option (and maybe adding a comment that fp is handled by the custom FPStrategy) and bumping the task implementation_version.

This comment has been minimized.

@traviscrawford

traviscrawford Apr 6, 2018

Member

Sounds good - thanks for the examples, those were really helpful. 🙇

@@ -60,9 +60,25 @@ def execute(self):
lib_binary_map[vt.target] = os.path.join(gopath, 'pkg', self.goos_goarch,
vt.target.import_path + '.a')

@classmethod
def _split_build_flags(cls, build_flags):

This comment has been minimized.

@jsirois

jsirois Apr 5, 2018

Member

I'm fine with it since it is in-fact tested and that serves as useful enough doc to hopefully save others time grokking this. That said, # Visible for testing is probably better than """Visible for testing""" since the triple quoted string implies API docs, and this is just a note to devs about a method already _private anyhow.

args = safe_shlex_split(build_flags) + [target.import_path]
"""Create and execute a `go install` command.
Build flags can be defined as globals (in `pants.ini`), as arguments to a Target, and

This comment has been minimized.

@jsirois

jsirois Apr 5, 2018

Member

I think it's worth extracting _calculate_build_flags or somesuch as its own method. It's meaty logic and arguably obscures self.go_dist... which is the proper highlight of the method.

non-existent, or refreshed if the link is older than the underlying binary. Any pre-existing
links within gopath's "pkg/" dir that do not correspond to a transitive dependency of target
are deleted.
non-existent, refreshed if the link is older than the underlying binary, or replaced with a

This comment has been minimized.

@jsirois

jsirois Apr 5, 2018

Member

This smells like the invalidation bug mentioned above and likely can be reverted when that is fixed.

This comment has been minimized.

@traviscrawford

traviscrawford Apr 6, 2018

Member

Confirmed this is fixed after updating the fingerprint strategy, so backed this out.

@traviscrawford traviscrawford force-pushed the traviscrawford:travis/go_binary_build_flags branch from b2c84ca to b326b30 Apr 6, 2018

Travis Crawford and others added some commits Apr 4, 2018

Add build_flags per go_binary
Go support currently has a global build flags option that is added to
every `go` command invocation.

```
$ ./pants compile --help | grep build-flags -A1
--compile-go-build-flags=<str> (default: '')
    Build flags to pass to Go compiler.
```

Talking with users there is need for
user-defined flags at the `go_binary` target level
(e.g.: `-tags netgo`).

@traviscrawford traviscrawford force-pushed the traviscrawford:travis/go_binary_build_flags branch from 73f42a1 to a2f782a Apr 6, 2018

@jsirois

jsirois approved these changes Apr 6, 2018

Copy link
Member

jsirois left a comment

This LGTM. Comments are just nits, so let me know what you want to do and I'm ready to merge at any time.

"""
self._build_flags_from_option = build_flags_from_option
self._is_flagged = is_flagged
self._get_build_flags = get_build_flags

This comment has been minimized.

@jsirois

jsirois Apr 6, 2018

Member

This is all you care about; you're just a middleman for the 2 parameters above. How about currying the GoCompile._get_build_flags at the top of execute and using that for this strategy and in the main GoCompile.execute invalid target loop?

This comment has been minimized.

@traviscrawford

traviscrawford Apr 6, 2018

Member

I had a partial (ha!) implementation of that approach earlier, and thought this was more straightforward. But now that you bring it up too, yeah, that's probably the better approach. I'll update with this suggestion.

build_flags = re.sub(r'^"|"$', '', self.get_options().build_flags)
args = safe_shlex_split(build_flags) + [target.import_path]
@classmethod
def _get_build_flags(cls, target, build_flags_from_option, is_flagged):

This comment has been minimized.

@jsirois

jsirois Apr 6, 2018

Member

You might:

@classmethod
@memoized_method
def _get_build_flags(cls, target, build_flags_from_option, is_flagged):

This will get called twice for each invalid target, once by the fp strategy, once by the work loop in execute.

"""
# If self.get_options().build_flags returns a quoted string, remove the outer quotes,
# which happens for flags passed from the command-line.
if (build_flags_from_option.startswith('\'') and build_flags_from_option.endswith('\'')) or \

This comment has been minimized.

@traviscrawford

traviscrawford Apr 6, 2018

Member

I ended up changing this to the verbose but explicit case of removing wrapped single and double quotes. Here's the command-line that failed before:

./pants binary contrib/go/examples/src/go/server/ \
    --compile-go-build-flags='-v -tags "netgo"'

In this case, the quoted arg "netgo" had it's right hand double quote removed.


def __eq__(self, other):
return type(self) == type(other) and \
self._get_build_flags_func.args == other._get_build_flags_func.args

This comment has been minimized.

@traviscrawford

traviscrawford Apr 6, 2018

Member

By using args in __eq__ we require get_build_flags_func be a partial function, which I think is ok.

This comment has been minimized.

@jsirois

jsirois Apr 9, 2018

Member

Agreed, since this FP strategy is only for use by GoCompile, but really the FingerprintStrategy docs could use some more clarity. For a single-use startegy like this, simple default object equality is enough. I'll follow up with that since this all looks great.

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented Apr 6, 2018

@jsirois Thanks for the suggestions, I believe this is complete now.

@jsirois

jsirois approved these changes Apr 7, 2018

Copy link
Member

jsirois left a comment

Thanks Travis!

@jsirois jsirois merged commit a9d3e19 into pantsbuild:master Apr 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment