Fix bug that recognized "C" as a remote package.

The `import "C"` statement has special semantics and imports of this
form are satisfied by the Go distribution with links to local libraries
using the C calling convention.  Pants did not recognize this and would
attempt to fetch "C" and fail since "C" is not part of the stdlib and is
also not a remote package.

This change fixes import scanning code to know about "C" and also
centralizes the import logic in `ImportOracle`.

Testing Done:
Added a new failing `test_issues_2616` to `GoFetchTest` that this change
fixes as well `compile` and `run` integration tests for Cgo that aren't
directly related to this change but do exercise the full pipeline of
interest for Cgo code.

Also did not check in the motivating example since it requires ImageMagick
libs, but adding
`contrib/go/examples/3rdparty/go/github.com/gographics/imagick/BUILD`:
```python
# Auto-generated by pants!
# To re-generate run: `pants buildgen.go --materialize --remote`

go_remote_libraries(
  rev='02d20b370cf9bac9d1d8297397c7e4b20a92add4',
  packages=[
    'imagick',
  ]
)
```

Demonstrates a successful resolve and compile for remote Cgo code (the
scary output below is just 3 compile warnings in ImageMagick):
```
./pants compile contrib/go/examples/3rdparty/go/github.com/gographics/imagick
...
09:43:02 00:00   [resolve]
...
09:43:02 00:00     [go]
                   Invalidated 1 target.INFO] Downloading https://github.com/gographics/imagick/archive/02d20b370cf9bac9d1d8297397c7e4b20a92add4.tar.gz...

09:43:04 00:02       [github.com/gographics/imagick/imagick]
...
09:43:04 00:02   [compile]
...
09:43:04 00:02     [go]
                   Invalidated 1 target.
09:43:04 00:02       [install]
                     # github.com/gographics/imagick/imagick
                     .pants.d/compile/go/contrib.go.examples.3rdparty.go.github.com.gographics.imagick.imagick/src/github.com/gographics/imagick/imagick/magick_core_env.go: In function ‘_cgo_85b92643e208_Cfunc_IsMagickInstantiated’:
                     .pants.d/compile/go/contrib.go.examples.3rdparty.go.github.com.gographics.imagick.imagick/src/github.com/gographics/imagick/imagick/magick_core_env.go:54:2: warning: ‘IsMagickInstantiated’ is deprecated [-Wdeprecated-declarations]
                     In file included from /usr/include/ImageMagick-6/magick/MagickCore.h:95:0,
                                      from /usr/include/ImageMagick-6/wand/MagickWand.h:72,
                                      from .pants.d/compile/go/contrib.go.examples.3rdparty.go.github.com.gographics.imagick.imagick/src/github.com/gographics/imagick/imagick/magick_core_env.go:8:
                     /usr/include/ImageMagick-6/magick/deprecate.h:205:3: note: declared here
                        IsMagickInstantiated(void) magick_attribute((deprecated)),
                        ^
                     # github.com/gographics/imagick/imagick
                     .pants.d/compile/go/contrib.go.examples.3rdparty.go.github.com.gographics.imagick.imagick/src/github.com/gographics/imagick/imagick/magick_wand_image.go: In function ‘_cgo_85b92643e208_Cfunc_MagickRadialBlurImage’:
                     .pants.d/compile/go/contrib.go.examples.3rdparty.go.github.com.gographics.imagick.imagick/src/github.com/gographics/imagick/imagick/magick_wand_image.go:3032:2: warning: ‘MagickRadialBlurImage’ is deprecated [-Wdeprecated-declarations]
                     In file included from /usr/include/ImageMagick-6/wand/MagickWand.h:78:0,
                                      from .pants.d/compile/go/contrib.go.examples.3rdparty.go.github.com.gographics.imagick.imagick/src/github.com/gographics/imagick/imagick/magick_wand_image.go:9:
                     /usr/include/ImageMagick-6/wand/deprecate.h:109:3: note: declared here
                        MagickRadialBlurImage(MagickWand *,const double)
                        ^
                     .pants.d/compile/go/contrib.go.examples.3rdparty.go.github.com.gographics.imagick.imagick/src/github.com/gographics/imagick/imagick/magick_wand_image.go: In function ‘_cgo_85b92643e208_Cfunc_MagickRadialBlurImageChannel’:
                     .pants.d/compile/go/contrib.go.examples.3rdparty.go.github.com.gographics.imagick.imagick/src/github.com/gographics/imagick/imagick/magick_wand_image.go:3049:2: warning: ‘MagickRadialBlurImageChannel’ is deprecated [-Wdeprecated-declarations]
                      // image that is added back into the original.
                       ^
                     In file included from /usr/include/ImageMagick-6/wand/MagickWand.h:78:0,
                                      from .pants.d/compile/go/contrib.go.examples.3rdparty.go.github.com.gographics.imagick.imagick/src/github.com/gographics/imagick/imagick/magick_wand_image.go:9:
                     /usr/include/ImageMagick-6/wand/deprecate.h:111:3: note: declared here
                        MagickRadialBlurImageChannel(MagickWand *,const ChannelType,const double)
                        ^

...
09:43:14 00:12   [complete]
               SUCCESS
```

CI went green here:
  https://travis-ci.org/pantsbuild/pants/builds/93049583

Bugs closed: 2616, 2619

Reviewed at https://rbcommons.com/s/twitter/r/3170/

contrib/go/examples/src/go/cgo/BUILD

@@ -0,0 +1,4 @@
+# Auto-generated by pants!
+# To re-generate run: `pants buildgen.go --materialize --remote`
+
+go_binary()

contrib/go/examples/src/go/cgo/main.go

@@ -0,0 +1,12 @@
+package main
+
+/*
+#include <stdlib.h>
+*/
+import "C"
+
+import "fmt"
+
+func main() {
+	fmt.Printf("Random from C: %d", int(C.random()))
+}

contrib/go/src/python/pants/contrib/go/tasks/go_buildgen.py

@@ -5,21 +5,18 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
-import json
 import os
-import subprocess
 from collections import defaultdict, namedtuple
 from textwrap import dedent
 
 from pants.base.build_environment import get_buildroot
 from pants.base.exceptions import TaskError
 from pants.base.generator import Generator, TemplateData
-from pants.base.workunit import WorkUnit, WorkUnitLabel
+from pants.base.workunit import WorkUnitLabel
 from pants.build_graph.address import Address
 from pants.build_graph.address_lookup_error import AddressLookupError
 from pants.util.contextutil import temporary_dir
 from pants.util.dirutil import safe_mkdir, safe_open
-from pants.util.memo import memoized_property
 
 from pants.contrib.go.subsystems.fetchers import Fetchers
 from pants.contrib.go.targets.go_binary import GoBinary
@@ -44,10 +41,9 @@ class WrongLocalSourceTargetTypeError(GenerationError):
   class NewRemoteEncounteredButRemotesNotAllowedError(GenerationError):
     """Indicates a new remote library dependency was found but --remote was not enabled."""
 
-  def __init__(self, workunit_factory, go_distribution, build_graph, local_root, fetchers,
+  def __init__(self, import_oracle, build_graph, local_root, fetchers,
                generate_remotes=False, remote_root=None):
-    self._workunit_factory = workunit_factory
-    self._go_distribution = go_distribution
+    self._import_oracle = import_oracle
     self._build_graph = build_graph
     self._local_source_root = local_root
     self._fetchers = fetchers
@@ -63,21 +59,21 @@ def generate(self, local_go_targets):
     visited = {l.import_path: l.address for l in local_go_targets}
     with temporary_dir() as gopath:
       for local_go_target in local_go_targets:
-        name, import_paths = self._list_deps(gopath, local_go_target.address)
-        self._generate_missing(gopath, local_go_target.address, name, import_paths, visited)
+        deps = self._list_deps(gopath, local_go_target.address)
+        self._generate_missing(gopath, local_go_target.address, deps, visited)
     return visited.items()
 
-  def _generate_missing(self, gopath, local_address, name, import_paths, visited):
-    target_type = GoBinary if name == 'main' else GoLibrary
+  def _generate_missing(self, gopath, local_address, import_listing, visited):
+    target_type = GoBinary if import_listing.pkg_name == 'main' else GoLibrary
     existing = self._build_graph.get_target(local_address)
     if not existing:
       self._build_graph.inject_synthetic_target(address=local_address, target_type=target_type)
     elif existing and not isinstance(existing, target_type):
       raise self.WrongLocalSourceTargetTypeError('{} should be a {}'
                                                  .format(existing, target_type.__name__))
 
-    for import_path in import_paths:
-      if import_path not in self._go_stdlib:
+    for import_path in import_listing.all_imports:
+      if not self._import_oracle.is_go_internal_import(import_path):
         if import_path not in visited:
           fetcher = self._fetchers.maybe_get_fetcher(import_path)
           if fetcher:
@@ -98,17 +94,12 @@ def _generate_missing(self, gopath, local_address, name, import_paths, visited):
             # Recurse on local targets.
             address = Address(os.path.join(self._local_source_root, import_path),
                               os.path.basename(import_path))
-            name, import_paths = self._list_deps(gopath, address)
-            self._generate_missing(gopath, address, name, import_paths, visited)
+            deps = self._list_deps(gopath, address)
+            self._generate_missing(gopath, address, deps, visited)
           visited[import_path] = address
         dependency_address = visited[import_path]
         self._build_graph.inject_dependency(local_address, dependency_address)
 
-  @memoized_property
-  def _go_stdlib(self):
-    out = self._go_distribution.create_go_cmd('list', args=['std']).check_output()
-    return frozenset(out.strip().split())
-
   def _list_deps(self, gopath, local_address):
     # TODO(John Sirois): Lift out a local go sources target chroot util - GoWorkspaceTask and
     # GoTargetGenerator both create these chroot symlink trees now.
@@ -122,23 +113,7 @@ def _list_deps(self, gopath, local_address):
         dest_path = os.path.join(src_path, source_file)
         os.symlink(source_path, dest_path)
 
-    # TODO(John Sirois): Lift up a small `go list utility` - GoFetch and GoTargetGenerator both use
-    # this go command now as well as a version of the stdlib gathering done above in _go_stdlib.
-    go_cmd = self._go_distribution.create_go_cmd('list', args=['-json', import_path], gopath=gopath)
-    with self._workunit_factory(local_address.reference(),
-                                cmd=str(go_cmd),
-                                labels=[WorkUnitLabel.TOOL]) as workunit:
-      # TODO(John Sirois): It would be nice to be able to tee the stdout to the workunit to we have
-      # a capture of the json available for inspection in the server console.
-      process = go_cmd.spawn(stdout=subprocess.PIPE, stderr=workunit.output('stderr'))
-      out, _ = process.communicate()
-      returncode = process.returncode
-      workunit.set_outcome(WorkUnit.SUCCESS if returncode == 0 else WorkUnit.FAILURE)
-      if returncode != 0:
-        raise self.GenerationError('Problem listing imports for {}: {} failed with exit code {}'
-                                   .format(local_address, go_cmd, returncode))
-      data = json.loads(out)
-      return data.get('Name'), data.get('Imports', []) + data.get('TestImports', [])
+    return self._import_oracle.list_imports(import_path, gopath=gopath)
 
 
 class GoBuildgen(GoTask):
@@ -392,8 +367,7 @@ def generate_targets(self, local_go_targets=None):
                                          .format('\n\t'.join(sorted(remote_roots))))
     remote_root = remote_roots.pop() if remote_roots else None
 
-    generator = GoTargetGenerator(self.context.new_workunit,
-                                  self.go_dist,
+    generator = GoTargetGenerator(self.import_oracle,
                                   self.context.build_graph,
                                   local_root,
                                   Fetchers.global_instance(),

contrib/go/src/python/pants/contrib/go/tasks/go_fetch.py

@@ -5,7 +5,6 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
-import json
 import os
 import shutil
 from collections import defaultdict
@@ -14,8 +13,7 @@
 from pants.build_graph.address import Address
 from pants.build_graph.address_lookup_error import AddressLookupError
 from pants.util.contextutil import temporary_dir
-from pants.util.dirutil import safe_mkdir, safe_open
-from pants.util.memo import memoized_property
+from pants.util.dirutil import safe_mkdir
 
 from pants.contrib.go.subsystems.fetchers import Fetchers
 from pants.contrib.go.targets.go_remote_library import GoRemoteLibrary
@@ -192,32 +190,15 @@ def _resolve(self, dependent_remote_lib, address, pkg, implict_ok):
         raise self.UndeclaredRemoteLibError(address)
     return self.context.build_graph.get_target(address)
 
-  @memoized_property
-  def go_stdlib(self):
-    out = self.go_dist.create_go_cmd('list', args=['std']).check_output()
-    return frozenset(out.strip().split())
-
   @staticmethod
   def _is_relative(import_path):
     return import_path.startswith('.')
 
   def _get_remote_import_paths(self, pkg, gopath=None):
     """Returns the remote import paths declared by the given Go `pkg`."""
-    out = self.go_dist.create_go_cmd('list', args=['-json', pkg], gopath=gopath).check_output()
-    try:
-      data = json.loads(out)
-      imports = data.get('Imports', [])
-      imports.extend(data.get('TestImports', []))
-
-      return [imp for imp in imports
-              if (imp not in self.go_stdlib and
-                  # We assume relative imports are local to the package and skip attempts to
-                  # recursively resolve them.
-                  not self._is_relative(imp))]
-    except ValueError as e:
-      save_file = os.path.join(gopath, '.errors', pkg, 'list.json')
-      with safe_open(save_file, 'w') as fp:
-        fp.write(out)
-      self.context.log.error('Problem determining imports for {}, saved json response to {}'
-                             .format(pkg, save_file))
-      raise TaskError(e)
+    import_listing = self.import_oracle.list_imports(pkg, gopath=gopath)
+    return [imp for imp in import_listing.all_imports
+            if (not self.import_oracle.is_go_internal_import(imp) and
+                # We assume relative imports are local to the package and skip attempts to
+                # recursively resolve them.
+                not self._is_relative(imp))]

contrib/go/src/python/pants/contrib/go/tasks/go_task.py

@@ -5,8 +5,13 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
+import json
+import subprocess
+from collections import namedtuple
+
+from pants.base.workunit import WorkUnit, WorkUnitLabel
 from pants.task.task import Task
-from pants.util.memo import memoized_property
+from pants.util.memo import memoized_method, memoized_property
 
 from pants.contrib.go.subsystems.go_distribution import GoDistribution
 from pants.contrib.go.targets.go_binary import GoBinary
@@ -45,14 +50,95 @@ def is_go(target):
   def go_dist(self):
     return GoDistribution.Factory.global_instance().create()
 
+  @memoized_property
+  def import_oracle(self):
+    """Return an import oracle that can help look up and categorize imports.
+
+    :rtype: :class:`ImportOracle`
+    """
+    return ImportOracle(go_dist=self.go_dist, workunit_factory=self.context.new_workunit)
+
   @memoized_property
   def goos_goarch(self):
-    """Returns concatenated $GOOS and $GOARCH environment variables, separated by an underscore.
+    """Return concatenated $GOOS and $GOARCH environment variables, separated by an underscore.
 
     Useful for locating where the Go compiler is placing binaries ("$GOPATH/pkg/$GOOS_$GOARCH").
+
+    :rtype: string
     """
     return '{goos}_{goarch}'.format(goos=self._lookup_go_env_var('GOOS'),
                                     goarch=self._lookup_go_env_var('GOARCH'))
 
   def _lookup_go_env_var(self, var):
     return self.go_dist.create_go_cmd('env', args=[var]).check_output().strip()
+
+
+class ImportOracle(object):
+  """Answers questions about Go imports."""
+
+  class ListDepsError(Exception):
+    """Indicates a problem listing import paths for one or more packages."""
+
+  def __init__(self, go_dist, workunit_factory):
+    self._go_dist = go_dist
+    self._workunit_factory = workunit_factory
+
+  @memoized_property
+  def go_stdlib(self):
+    """Return the set of all Go standard library import paths.
+
+    :rtype: frozenset of string
+    """
+    out = self._go_dist.create_go_cmd('list', args=['std']).check_output()
+    return frozenset(out.strip().split())
+
+  def is_go_internal_import(self, import_path):
+    """Return `True` if the given import path will be satisfied directly by the Go distribution.
+
+    For example, both the go standard library ("archive/tar", "bufio", "fmt", etc.) and "C" imports
+    are satisfiable by a Go distribution via linking of internal Go code and external c standard
+    library code respectively.
+
+    :rtype: bool
+    """
+    # The "C" package is a psuedo-package that links through to the c stdlib, see:
+    #   http://blog.golang.org/c-go-cgo
+    return import_path == 'C' or import_path in self.go_stdlib
+
+  class ImportListing(namedtuple('ImportListing', ['pkg_name', 'imports', 'test_imports'])):
+    """Represents all the imports of a given package."""
+
+    @property
+    def all_imports(self):
+      """Return all imports for this package, including any test imports.
+
+      :rtype: list of string
+      """
+      return self.imports + self.test_imports
+
+  @memoized_method
+  def list_imports(self, pkg, gopath=None):
+    """Return a listing of the dependencies of the given package.
+
+    :param string pkg: The package whose files to list all dependencies of.
+    :param string gopath: An optional $GOPATH which points to a Go workspace containing `pkg`.
+    :returns: The import listing for `pkg` that represents all its dependencies.
+    :rtype: :class:`ImportOracle.ImportListing`
+    :raises: :class:`ImportOracle.ListDepsError` if there was a problem listing the dependencies
+             of `pkg`.
+    """
+    go_cmd = self._go_dist.create_go_cmd('list', args=['-json', pkg], gopath=gopath)
+    with self._workunit_factory(pkg, cmd=str(go_cmd), labels=[WorkUnitLabel.TOOL]) as workunit:
+      # TODO(John Sirois): It would be nice to be able to tee the stdout to the workunit to we have
+      # a capture of the json available for inspection in the server console.
+      process = go_cmd.spawn(stdout=subprocess.PIPE, stderr=workunit.output('stderr'))
+      out, _ = process.communicate()
+      returncode = process.returncode
+      workunit.set_outcome(WorkUnit.SUCCESS if returncode == 0 else WorkUnit.FAILURE)
+      if returncode != 0:
+        raise self.ListDepsError('Problem listing imports for {}: {} failed with exit code {}'
+                                 .format(pkg, go_cmd, returncode))
+      data = json.loads(out)
+      return self.ImportListing(pkg_name=data.get('Name'),
+                                imports=data.get('Imports', []),
+                                test_imports=data.get('TestImports', []))

contrib/go/tests/python/pants_test/contrib/go/tasks/BUILD

@@ -139,3 +139,12 @@ python_tests(
     'tests/python/pants_test:int-test',
   ]
 )
+
+python_tests(
+  name='go_task',
+  sources=['test_go_task.py'],
+  dependencies=[
+    'contrib/go/src/python/pants/contrib/go/tasks:go_task',
+    'tests/python/pants_test/tasks:task_test_base',
+  ]
+)

contrib/go/tests/python/pants_test/contrib/go/tasks/test_go_buildgen.py

@@ -333,3 +333,39 @@ def test_issues_2395(self):
     self.build_graph.reset()  # Force targets to be loaded off disk
     self.assertEqual('v4.5.6', self.target('3rdparty/go/pantsbuild.org/fake').rev)
     self.assertEqual(pre_execute_files, self.buildroot_files())
+
+  def test_issues_2616(self):
+    self.set_options(remote=False)
+
+    self.create_file(relpath='src/go/jane/bar.go', contents=dedent("""
+        package jane
+
+        var PublicConstant = 42
+      """))
+    self.create_file(relpath='src/go/fred/foo.go', contents=dedent("""
+        package main
+
+        /*
+        #include <stdlib.h>
+        */
+        import "C" // C was erroneously categorized as a remote lib in issue 2616.
+
+        import (
+          "fmt"
+          "jane"
+        )
+
+        func main() {
+          fmt.Printf("Hello %s!", jane.PublicConstant)
+          fmt.Printf("Random from C: %d", int(C.random()))
+        }
+      """))
+    fred = self.make_target('src/go/fred', GoBinary)
+    context = self.context(target_roots=[fred])
+    task = self.create_task(context)
+    task.execute()
+
+    jane = self.target('src/go/jane')
+    self.assertIsNotNone(jane)
+    self.assertEqual([jane], fred.dependencies)
+    self.assertEqual({jane, fred}, set(self.build_graph.targets()))

contrib/go/tests/python/pants_test/contrib/go/tasks/test_go_compile_integration.py

@@ -31,3 +31,8 @@ def test_go_compile_simple(self):
                              for libname in ('libA', 'libB', 'libC', 'libD', 'libE'))
         self.assert_contains_exact_files(os.path.join(workdir, 'compile', 'go'),
                                          expected_files)
+
+  def test_go_compile_cgo(self):
+    args = ['compile', 'contrib/go/examples/src/go/cgo']
+    pants_run = self.run_pants(args)
+    self.assert_success(pants_run)