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 go_protobuf_library #5838

Merged
merged 4 commits into from May 25, 2018

Conversation

Projects
None yet
2 participants
@traviscrawford
Copy link
Member

traviscrawford commented May 18, 2018

Following the per-language *_protobuf_library pattern, here we add go_protobuf_library support and as simple example showing how to use it.

# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

go_remote_library(rev='v1.1.0')

This comment has been minimized.

@traviscrawford

traviscrawford May 18, 2018

Member

Note - this library will need to be published to https://github.com/pantsbuild/binaries

This comment has been minimized.

@stuhood

stuhood May 21, 2018

Member

I'm probably ok with hosting this on pantsbuild/binaries, but are there other options here? A consumer of this library will have a go environment bootstrapped by pants, so it seems like go get into a directory under ~/.cache/pants (ie get_options().pants_bootstrapdir) would work ok too.

This comment has been minimized.

@traviscrawford

traviscrawford May 23, 2018

Member

Thanks for the suggestion about making this a Subsystem - I didn't realize that's the recommended approach these days. Switched this over. Overall it's pretty nice, the one thing that stood out was needing to self-manage the working dir, which itself is not a big deal, but I'm probably not being consistent with how other folks use it.

This comment has been minimized.

@traviscrawford

traviscrawford May 23, 2018

Member

Now that we switch to a Subsystem, I'll close the PR against binaries as that is no longer needed.

@@ -20,7 +20,7 @@ class GoReleaseUrlGenerator(BinaryToolUrlGenerator):
_DIST_URL_FMT = 'https://storage.googleapis.com/golang/go{version}.{system_id}.tar.gz'

_SYSTEM_ID = {
'darwin': 'darwin-amd64',
'mac': 'darwin-amd64',

This comment has been minimized.

@traviscrawford

traviscrawford May 18, 2018

Member

With a clean build, this change is needed to fetch a Go distribution. I believe this is related to the recent Go distribution fetching changes, which are awesome BTW! This is great so every single Go distribution is available as they are released by the Go team.

This comment has been minimized.

@stuhood

stuhood May 21, 2018

Member

cc @cosmicexplorer : is this expected?

This comment has been minimized.

@traviscrawford

traviscrawford May 23, 2018

Member

Bump @cosmicexplorer - AFAICT this is needed to build.

# Licensed under the Apache License, Version 2.0 (see LICENSE).

# This target must be build with protoc that recognizes `option go_package`
# ./pants run contrib/go/examples/src/go/distance/ --protoc-version=3.5.1

This comment has been minimized.

@traviscrawford

traviscrawford May 18, 2018

Member

option go_package does not appear to be available in protobuf 2.x, which is what the Pants build currently defaults to. I'm not sure how to deal with this, other than change the protoc version at runtime.

One idea that jumps to mind is tagging this target to indicate that it requires proto3, and we could have a separate CI job that runs with proto3. I'm totally open to suggestions for how to deal with this.

This comment has been minimized.

@stuhood

stuhood May 21, 2018

Member

Generally targets that do not build without extra flags end up needing to be defined under <root>/testprojects.

But because golang doesn't support multiple sourceroots, I think what you might need to do here is define this target in a file like TEST_BUILD, and then rename it for the purpose of an integration test. See https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/backend/python/test_pants_requirement_integration.py#L36-L46 for an example of doing this.

But please do add an integration test that covers building the binary.

This comment has been minimized.

@stuhood

stuhood May 24, 2018

Member

Could you update the comment for the more specific protoc arg?

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented May 18, 2018

Example output:

SFO-M-TCRAWFORD03:pants travis$ ./pants clean-all run contrib/go/examples/src/go/distance/ --protoc-version=3.5.1

22:31:55 00:00 [main]
               See a report at: http://localhost:64824/run/pants_run_2018_05_17_22_31_55_439_c955c133ee994f48ba5879cdaa8b60c6
22:31:55 00:00   [setup]
22:31:55 00:00     [parse]
               Executing tasks in goals: clean-all -> jvm-platform-validate -> bootstrap -> imports -> unpack-jars -> deferred-sources -> gen -> resolve -> resources -> compile -> pyprep -> binary -> run
22:31:56 00:01   [clean-all]
22:31:56 00:01     [ng-killall]
22:31:56 00:01     [kill-pantsd]
22:31:56 00:01     [clean-all]INFO] For async removal, run `./pants clean-all --async`

22:31:56 00:01   [jvm-platform-validate]
22:31:56 00:01     [jvm-platform-validate]
22:31:56 00:01   [bootstrap]
22:31:56 00:01     [substitute-aliased-targets]
22:31:56 00:01     [jar-dependency-management]
22:31:56 00:01     [bootstrap-jvm-tools]
22:31:57 00:02     [provide-tools-jar]
22:31:57 00:02   [imports]
22:31:57 00:02     [ivy-imports]
22:31:57 00:02   [unpack-jars]
22:31:57 00:02     [unpack-jars]
22:31:57 00:02   [deferred-sources]
22:31:57 00:02     [deferred-sources]
22:31:57 00:02   [gen]
22:31:57 00:02     [antlr-java]
22:31:57 00:02     [antlr-py]
22:31:57 00:02     [jaxb]
22:31:57 00:02     [protoc]
22:31:57 00:02     [ragel]
22:31:57 00:02     [thrift-java]
22:31:57 00:02     [thrift-py]
22:31:57 00:02     [wire]
22:31:57 00:02     [avro-java]
22:31:57 00:02     [go-thrift]
22:31:57 00:02     [go-protobuf]
                   Invalidated 2 targets..
                       /Users/travis/.cache/pants/bin/protoc/mac/10.13/3.5.1/protoc -I=/Users/travis/src/pants/contrib/go/examples/src/protobuf --go_out=/Users/travis/src/pants/.pants.d/gen/go-protobuf/252d64521cf9/contrib.go.examples.src.protobuf.org.pantsbuild.example.distance.distance-go/current/src/go /Users/travis/src/pants/contrib/go/examples/src/protobuf/org/pantsbuild/example/distance/distances.proto.
                       /Users/travis/.cache/pants/bin/protoc/mac/10.13/3.5.1/protoc -I=/Users/travis/src/pants/contrib/go/examples/src/protobuf --go_out=/Users/travis/src/pants/.pants.d/gen/go-protobuf/252d64521cf9/contrib.go.examples.src.protobuf.org.pantsbuild.example.route.route-go/current/src/go /Users/travis/src/pants/contrib/go/examples/src/protobuf/org/pantsbuild/example/route/route.proto
22:31:57 00:02     [jax-ws]
22:31:57 00:02     [scrooge]
22:31:57 00:02     [thrifty]
22:31:57 00:02   [resolve]
22:31:57 00:02     [ivy]
22:31:57 00:02     [coursier]
22:31:57 00:02     [go]
                   Invalidated 1 target.
22:31:57 00:02       [fetch github.com/golang/protobuf/proto]INFO] Downloading https://github.com/golang/protobuf/archive/v1.1.0.tar.gz...

22:31:58 00:03       [list github.com/golang/protobuf/proto]
22:31:59 00:04     [scala-js-compile]
22:31:59 00:04     [scala-js-link]
22:31:59 00:04     [node]
22:31:59 00:04   [resources]
22:31:59 00:04     [prepare]
22:31:59 00:04     [services]
22:31:59 00:04   [compile]
22:31:59 00:04     [node]
22:31:59 00:04     [compile-jvm-prep-command]
22:31:59 00:04       [jvm_prep_command]
22:31:59 00:04     [compile-prep-command]
22:31:59 00:04     [compile]
22:31:59 00:04     [zinc]
22:31:59 00:04     [javac]
22:31:59 00:04     [cpp]
22:31:59 00:04     [errorprone]
22:31:59 00:04     [findbugs]
22:31:59 00:04     [go]
                   Invalidated 4 targets.
22:31:59 00:04       [install contrib/go/3rdparty/go/github.com/golang/protobuf/proto:proto]
                     
22:32:00 00:05       [install .pants.d/gen/go-protobuf/252d64521cf9/contrib.go.examples.src.protobuf.org.pantsbuild.example.distance.distance-go/current/src/go/pantsbuild/example/distance:distance-go]
                     
22:32:01 00:06       [install .pants.d/gen/go-protobuf/252d64521cf9/contrib.go.examples.src.protobuf.org.pantsbuild.example.route.route-go/current/src/go/pantsbuild/example/route:route-go]
                     
22:32:01 00:06       [install contrib/go/examples/src/go/distance:distance]
                     
22:32:02 00:07   [pyprep]
22:32:02 00:07     [interpreter]
22:32:03 00:08     [build-local-dists]
22:32:03 00:08     [requirements]
22:32:04 00:09     [sources]
22:32:04 00:09   [binary]
22:32:04 00:09     [binary-jvm-prep-command]
22:32:04 00:09       [jvm_prep_command]
22:32:04 00:09     [binary-prep-command]
22:32:04 00:09     [py]
22:32:04 00:09     [py-wheels]
22:32:04 00:09     [jvm]
22:32:04 00:09     [dup]
22:32:04 00:09     [cpplib]
22:32:04 00:09       [cpp-library]
22:32:04 00:09     [cpp]
22:32:04 00:09       [cpp-binary]
22:32:04 00:09     [go]
                   creating dist/go/bin/distance
22:32:04 00:09   [run]
22:32:04 00:09     [py]
22:32:04 00:09     [jvm]
22:32:04 00:09     [cpp]
22:32:04 00:09     [go]name:"example_route" distances:<unit:"parsecs" number:27 > distances:<unit:"mm" number:2 > 

22:32:04 00:09     [node]
               Waiting for background workers to finish.
22:32:04 00:09   [complete]
               SUCCESS
SFO-M-TCRAWFORD03:pants travis$ 

@stuhood stuhood requested a review from jsirois May 18, 2018

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented May 21, 2018

Just sent pantsbuild/binaries#67 for review which adds protoc-gen-go.

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

go_remote_library(rev='v1.1.0')

This comment has been minimized.

@stuhood

stuhood May 21, 2018

Member

I'm probably ok with hosting this on pantsbuild/binaries, but are there other options here? A consumer of this library will have a go environment bootstrapped by pants, so it seems like go get into a directory under ~/.cache/pants (ie get_options().pants_bootstrapdir) would work ok too.

# Licensed under the Apache License, Version 2.0 (see LICENSE).

# This target must be build with protoc that recognizes `option go_package`
# ./pants run contrib/go/examples/src/go/distance/ --protoc-version=3.5.1

This comment has been minimized.

@stuhood

stuhood May 21, 2018

Member

Generally targets that do not build without extra flags end up needing to be defined under <root>/testprojects.

But because golang doesn't support multiple sourceroots, I think what you might need to do here is define this target in a file like TEST_BUILD, and then rename it for the purpose of an integration test. See https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/backend/python/test_pants_requirement_integration.py#L36-L46 for an example of doing this.

But please do add an integration test that covers building the binary.

@@ -20,7 +20,7 @@ class GoReleaseUrlGenerator(BinaryToolUrlGenerator):
_DIST_URL_FMT = 'https://storage.googleapis.com/golang/go{version}.{system_id}.tar.gz'

_SYSTEM_ID = {
'darwin': 'darwin-amd64',
'mac': 'darwin-amd64',

This comment has been minimized.

@stuhood

stuhood May 21, 2018

Member

cc @cosmicexplorer : is this expected?


@classmethod
def subsystem_dependencies(cls):
return super(GoProtobufGen, cls).subsystem_dependencies() + (Protoc.scoped(cls), ProtocGenGo.scoped(cls),)

This comment has been minimized.

@stuhood

stuhood May 21, 2018

Member

Since the Protoc subsystem is scoped to this class (yay!) I believe you can resolve the version problem by overriding the version of protoc only within this scope... ie: --protoc-gen-go-proto-version=.

Likewise, I don't think scoping ProtocGenGo makes sense (unless you're expecting reuse in other tasks).

This comment has been minimized.

@traviscrawford

traviscrawford May 23, 2018

Member

Great suggestion about scoped options - I put these in pants.ini and we no longer need to pass the protoc version args on the command-line. I bet this will pass CI too.

return self._protoc.select(context=self.context)

@property
def _version(self):

This comment has been minimized.

@stuhood

stuhood May 21, 2018

Member

I think this is unused.


@memoized_property
def _protoc(self):
return Protoc.scoped_instance(self)

This comment has been minimized.

@stuhood

stuhood May 21, 2018

Member

Looks like this is only used in one place, so could probably inline and memoize that instead.


env = os.environ.copy()
env['PATH'] = ':'.join([env['PATH'],
os.path.dirname(self._protoc_gen_go.select(context=self.context))])

This comment has been minimized.

@stuhood

stuhood May 21, 2018

Member

Consider prefixing this onto the path rather than suffixing?


bases = OrderedSet(tgt.target_base for tgt in target.closure() if self.is_gentarget(tgt))
for base in bases:
target_cmd.append('-I={}'.format(os.path.join(get_buildroot(), base)))

This comment has been minimized.

@stuhood

stuhood May 21, 2018

Member

So, the java task does this as well (giving the entire target base to protoc), but if there is any way to hand it explicit files for the dependencies, that would be ideal (it might not be). Giving the compiler the entire base can lead to undeclared dependencies.

raise TaskError('File {} must contain "option go_package"'.format(source))
return namespace.group(1)

# Override because file names were too long.

This comment has been minimized.

@stuhood

stuhood May 21, 2018

Member

Could you open a ticket about this one? Or fix it in the base class maybe?

This comment has been minimized.

@traviscrawford

traviscrawford May 24, 2018

Member

Here's an example of the error:

Exception caught: (<type 'exceptions.IOError'>)
  File "/Users/travis/src/pants/src/python/pants/bin/pants_loader.py", line 73, in <module>
    main()
  File "/Users/travis/src/pants/src/python/pants/bin/pants_loader.py", line 69, in main
    PantsLoader.run()
  File "/Users/travis/src/pants/src/python/pants/bin/pants_loader.py", line 65, in run
    cls.load_and_execute(entrypoint)
  File "/Users/travis/src/pants/src/python/pants/bin/pants_loader.py", line 58, in load_and_execute
    entrypoint_main()
  File "/Users/travis/src/pants/src/python/pants/bin/pants_exe.py", line 39, in main
    PantsRunner(exiter, start_time=start_time).run()
  File "/Users/travis/src/pants/src/python/pants/bin/pants_runner.py", line 53, in run
    return runner.run()
  File "/Users/travis/src/pants/src/python/pants/bin/local_pants_runner.py", line 45, in run
    self._run()
  File "/Users/travis/src/pants/src/python/pants/bin/local_pants_runner.py", line 90, in _run
    goal_runner_result = goal_runner.run()
  File "/Users/travis/src/pants/src/python/pants/bin/goal_runner.py", line 272, in run
    result = self._execute_engine()
  File "/Users/travis/src/pants/src/python/pants/bin/goal_runner.py", line 260, in _execute_engine
    result = engine.execute(self._context, self._goals)
  File "/Users/travis/src/pants/src/python/pants/engine/legacy_engine.py", line 26, in execute
    self.attempt(context, goals)
  File "/Users/travis/src/pants/src/python/pants/engine/round_engine.py", line 233, in attempt
    goal_executor.attempt(explain)
  File "/Users/travis/src/pants/src/python/pants/engine/round_engine.py", line 49, in attempt
    task.execute()
  File "/Users/travis/src/pants/contrib/go/src/python/pants/contrib/go/tasks/go_compile.py", line 66, in execute
    self._go_install(vt.target, gopath, build_flags)
  File "/Users/travis/src/pants/contrib/go/src/python/pants/contrib/go/tasks/go_compile.py", line 107, in _go_install
    workunit_labels=[WorkUnitLabel.COMPILER])
  File "/Users/travis/src/pants/contrib/go/src/python/pants/contrib/go/subsystems/go_distribution.py", line 139, in execute_go_cmd
    stdout=workunit.output('stdout'),
  File "/Users/travis/src/pants/src/python/pants/base/workunit.py", line 193, in output
    self._outputs[name] = FileBackedRWBuf(path)
  File "/Users/travis/src/pants/src/python/pants/util/rwbuf.py", line 76, in __init__
    _RWBuf.__init__(self, open(backing_file, 'a+'))

Exception message: [Errno 63] File name too long: u'/Users/travis/src/pants/.pants.d/run-tracker/pants_run_2018_05_23_17_04_36_657_cf1fd75f8bd547b1bd06cce8dcf5ad19/tool_outputs/install__pants_d_gen_go_protobuf_252d64521cf9_contrib_go_examples_src_protobuf_org_pantsbuild_example_distance_distance_go_current_src_go_pantsbuild_example_distance_contrib_go_examples_src_protobuf_org_pantsbuild_example_distance_distance_go-d58513c8-2091-49eb-8339-b9ff1d96a514.stdout'

I made the target.id -> target.name change in the base class, though that affects other code generators too.

This comment has been minimized.

@stuhood

stuhood May 24, 2018

Member

Oooh.

So, in that case, I think that's because the name of the workunit is too long in compile.go, here: https://github.com/pantsbuild/pants/blob/master/contrib/go/src/python/pants/contrib/go/tasks/go_compile.py#L106 ... moving that to something shorter (or summarizing into a package name) might help.

@traviscrawford traviscrawford force-pushed the traviscrawford:travis/go_protobuf_library branch 2 times, most recently from bf904d0 to ce19e86 May 23, 2018

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented May 24, 2018

Thanks for the great feedback, especially around using a Subsystem. The tool can now self bootstrap!

Here's some example output:

SFO-M-TCRAWFORD03:pants travis$ ./pants run contrib/go/examples/src/go/distance/ 

20:31:55 00:00 [main]
               See a report at: http://localhost:64824/run/pants_run_2018_05_23_20_31_56_399_535e91d5d02c45388a1c21dd9a7109f2
20:31:56 00:01   [setup]
20:31:56 00:01     [parse]
               Executing tasks in goals: bootstrap -> imports -> unpack-jars -> deferred-sources -> gen -> jvm-platform-validate -> resolve -> resources -> compile -> pyprep -> binary -> run
20:31:57 00:02   [bootstrap]
20:31:57 00:02     [substitute-aliased-targets]
20:31:57 00:02     [jar-dependency-management]
20:31:57 00:02     [bootstrap-jvm-tools]
20:31:57 00:02     [provide-tools-jar]
20:31:57 00:02   [imports]
20:31:57 00:02     [ivy-imports]
20:31:57 00:02   [unpack-jars]
20:31:57 00:02     [unpack-jars]
20:31:57 00:02   [deferred-sources]
20:31:57 00:02     [deferred-sources]
20:31:57 00:02   [gen]
20:31:57 00:02     [antlr-java]
20:31:57 00:02     [antlr-py]
20:31:57 00:02     [jaxb]
20:31:57 00:02     [protoc]
20:31:57 00:02     [ragel]
20:31:57 00:02     [thrift-java]
20:31:57 00:02     [thrift-py]
20:31:57 00:02     [wire]
20:31:57 00:02     [avro-java]
20:31:57 00:02     [go-thrift]
20:31:57 00:02     [go-protobuf]
                   Invalidated 2 targets.Cloning into '/Users/travis/src/pants/.pants.d/protoc-gen-go/versions/v1.1.0/src/github.com/golang/protobuf'...
remote: Counting objects: 4680, done.
remote: Compressing objects: 100% (27/27), done.
remote: Total 4680 (delta 13), reused 29 (delta 11), pack-reused 4640
Receiving objects: 100% (4680/4680), 3.45 MiB | 4.78 MiB/s, done.
Resolving deltas: 100% (3178/3178), done.
Note: checking out 'v1.1.0'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

HEAD is now at b4deda0 Merge pull request #591 from golang/master-merge
INFO] Selected protoc-gen-go binary bootstrapped to: /Users/travis/src/pants/.pants.d/protoc-gen-go/versions/v1.1.0/bin/protoc-gen-go
.
                       /Users/travis/.cache/pants/bin/protoc/mac/10.13/3.4.1/protoc -I=/Users/travis/src/pants/contrib/go/examples/src/protobuf --go_out=/Users/travis/src/pants/.pants.d/gen/go-protobuf/252d64521cf9/contrib.go.examples.src.protobuf.org.pantsbuild.example.distance.distance-go/current/src/go /Users/travis/src/pants/contrib/go/examples/src/protobuf/org/pantsbuild/example/distance/distances.proto.
                       /Users/travis/.cache/pants/bin/protoc/mac/10.13/3.4.1/protoc -I=/Users/travis/src/pants/contrib/go/examples/src/protobuf --go_out=/Users/travis/src/pants/.pants.d/gen/go-protobuf/252d64521cf9/contrib.go.examples.src.protobuf.org.pantsbuild.example.route.route-go/current/src/go /Users/travis/src/pants/contrib/go/examples/src/protobuf/org/pantsbuild/example/route/route.proto
20:32:01 00:06     [jax-ws]
20:32:01 00:06     [scrooge]
20:32:01 00:06     [thrifty]
20:32:01 00:06   [jvm-platform-validate]
20:32:01 00:06     [jvm-platform-validate]
20:32:01 00:06   [resolve]
20:32:01 00:06     [ivy]
20:32:01 00:06     [coursier]
20:32:01 00:06     [go]
                   Invalidated 1 target.
20:32:01 00:06       [fetch github.com/golang/protobuf/proto]INFO] Downloading https://github.com/golang/protobuf/archive/v1.1.0.tar.gz...

20:32:02 00:07       [list github.com/golang/protobuf/proto]
20:32:03 00:08     [scala-js-compile]
20:32:03 00:08     [scala-js-link]
20:32:03 00:08     [node]
20:32:03 00:08   [resources]
20:32:03 00:08     [prepare]
20:32:03 00:08     [services]
20:32:03 00:08   [compile]
20:32:03 00:08     [node]
20:32:03 00:08     [compile-jvm-prep-command]
20:32:03 00:08       [jvm_prep_command]
20:32:03 00:08     [compile-prep-command]
20:32:03 00:08     [compile]
20:32:03 00:08     [zinc]
20:32:03 00:08     [javac]
20:32:03 00:08     [cpp]
20:32:03 00:08     [errorprone]
20:32:03 00:08     [findbugs]
20:32:03 00:08     [go]
                   Invalidated 4 targets.
20:32:03 00:08       [install contrib/go/3rdparty/go/github.com/golang/protobuf/proto:proto]
                     
20:32:04 00:09       [install .pants.d/gen/go-protobuf/252d64521cf9/contrib.go.examples.src.protobuf.org.pantsbuild.example.distance.distance-go/current/src/go/pantsbuild/example/distance:distance-go]
                     
20:32:05 00:10       [install .pants.d/gen/go-protobuf/252d64521cf9/contrib.go.examples.src.protobuf.org.pantsbuild.example.route.route-go/current/src/go/pantsbuild/example/route:route-go]
                     
20:32:05 00:10       [install contrib/go/examples/src/go/distance:distance]
                     
20:32:06 00:11   [pyprep]
20:32:06 00:11     [interpreter]
20:32:07 00:12     [build-local-dists]
20:32:07 00:12     [requirements]
20:32:08 00:13     [sources]
20:32:08 00:13   [binary]
20:32:08 00:13     [binary-jvm-prep-command]
20:32:08 00:13       [jvm_prep_command]
20:32:08 00:13     [binary-prep-command]
20:32:08 00:13     [py]
20:32:08 00:13     [py-wheels]
20:32:08 00:13     [jvm]
20:32:08 00:13     [dup]
20:32:08 00:13     [cpplib]
20:32:08 00:13       [cpp-library]
20:32:08 00:13     [cpp]
20:32:08 00:13       [cpp-binary]
20:32:08 00:13     [go]
                   creating dist/go/bin/distance
20:32:08 00:13   [run]
20:32:08 00:13     [py]
20:32:08 00:13     [jvm]
20:32:08 00:13     [cpp]
20:32:08 00:13     [go]name:"example_route" distances:<unit:"parsecs" number:27 > distances:<unit:"mm" number:2 > 

20:32:08 00:13     [node]
               Waiting for background workers to finish.
20:32:08 00:13   [complete]
               SUCCESS
SFO-M-TCRAWFORD03:pants travis$ 
@stuhood
Copy link
Member

stuhood left a comment

Thanks Travis! This looks great. A few small things to fix.

# Licensed under the Apache License, Version 2.0 (see LICENSE).

# This target must be build with protoc that recognizes `option go_package`
# ./pants run contrib/go/examples/src/go/distance/ --protoc-version=3.5.1

This comment has been minimized.

@stuhood

stuhood May 24, 2018

Member

Could you update the comment for the more specific protoc arg?

'versions', self.get_options().version)
tool_path = os.path.join(workdir, 'bin/protoc-gen-go')

if not os.path.exists(tool_path):

This comment has been minimized.

@stuhood

stuhood May 24, 2018

Member

There is a potential failure mode here where if the checkout or install fail, we're left with a corrupted checkout.

To avoid that, you can use safe_concurrent_creation to create in a temp directory, which can be renamed into the final location only if the install completes successfully:

@contextmanager
def safe_concurrent_creation(target_path):
"""A contextmanager that yields a temporary path and renames it to a final target path when the
contextmanager exits.
Useful when concurrent processes may attempt to create a file, and it doesn't matter who wins.
:param target_path: The final target path to rename the temporary path to.
:yields: A temporary path containing the original path with a unique (uuid4) suffix.
"""

The entire body of this if statement would be inside the contextmanager.

This comment has been minimized.

@traviscrawford

traviscrawford May 24, 2018

Member

I think this will recover from a failed checkout, etc. because if the binary doesn't exist, we delete the workdir and try again:

safe_mkdir(workdir, clean=True)

That said, this context manager is handy! I'll switch things over.

@@ -197,7 +197,7 @@ def _do_validate_sources_present(self, target):
return True

def _get_synthetic_address(self, target, target_workdir):
synthetic_name = target.id
synthetic_name = target.name

This comment has been minimized.

@stuhood

stuhood May 24, 2018

Member

I replied to your comment about the error: I was wrong about where to fix this.

So, in that case, I think that's because the name of the workunit is too long in compile.go, here: https://github.com/pantsbuild/pants/blob/master/contrib/go/src/python/pants/contrib/go/tasks/go_compile.py#L106 ... moving that to something shorter (or summarizing into a package name) might help.

And looking around a bit, it looks like GoTarget.import_path might be a good choice for a shorter name to use in compile.go?

@@ -263,6 +267,12 @@ skip: True
[fmt.scalafix]
skip: True

[protoc.gen.go-protobuf]
version=3.4.1

This comment has been minimized.

@stuhood

stuhood May 24, 2018

Member

Which of these tool definitions is actually being used? I think maybe the gen.go-protobuf one? You should be able to remove/replace the subsystem dependency on Protoc from either your subsystem or the task, depending on which is actually using it.

This comment has been minimized.

@traviscrawford

traviscrawford May 25, 2018

Member

I spent a bit looking through the various flags, and found something interesting. Imagine you're a user and is trying to figure out which flag to set. Here's an example.

[root@ea70b2190df0 pants]# ./pants --help-all | grep -- --protoc | sort -u
--protoc-gen-go-protobuf-version=<str> (default: '3.4.1')
--protoc-gen-go-version=<str> (default: 'v1.1.0')
--protoc-gen-protoc-version=<str> (default: '2.4.1')
--protoc-gen-version=<str> (default: '2.4.1')
--protoc-protoc-gen-go-version=<str> (default: '2.4.1')
--protoc-version=<str> (default: '2.4.1')
[root@ea70b2190df0 pants]# 

Notice how we concatenate protoc-gen-go - protobuf - version together, for example. We use - as the delimiter within scopes, and between scopes, which makes this a bit hard to read.

This is just an observation at this time. Some things we could do about this is switch to another delimiter within scopes (e.g.: _) or perhaps join scopes with a double dash (e.g.: --) but not proposing this at this time.

This comment has been minimized.

@traviscrawford

traviscrawford May 25, 2018

Member

If we use options_scope = 'protoc_gen_go' here's an example:

[root@ea70b2190df0 pants]# ./pants --help-all | grep -- --protoc | sort -u
--protoc-gen-go-protobuf-version=<str> (default: '3.4.1')
--protoc_gen_go-version=<str> (default: 'v1.1.0')

Notice how the prefix "protoc gen go" prefix is the same in both cases, with just the delimiter different due to the options_scope.

No proposed change at this time, just noticing something through the lens of a user. Really I noticed this while trying to update some docs and thinking, wow this is confusing :)

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented May 24, 2018

Also, it looks like there is one substantive CI failure.

Travis Crawford and others added some commits May 17, 2018

Add go_protobuf_library
Following the per-language `*_protobuf_library` pattern, here we add `go_protobuf_library` support and as simple example showing how to use it.
Updates per review feedback
Here we change how run tracker workunits are named, using a shorter name inside `GoCompile` so it does not create file names that are too long.

@traviscrawford traviscrawford force-pushed the traviscrawford:travis/go_protobuf_library branch from ce19e86 to e259deb May 25, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@stuhood stuhood merged commit 4ae9f6d into pantsbuild:master May 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@traviscrawford traviscrawford deleted the traviscrawford:travis/go_protobuf_library branch May 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment