Fix Go source excludes; Cleanup old filespec matching (pantsbuild#4350)

### Problem

go sources globs were defined with incorrect excludes.

### Other Change

* Cleanup old filespec matching
* Add actual files during tests due to slight behavior change (from regex match to actual file match)

contrib/go/src/python/pants/contrib/go/targets/go_local_source.py

@@ -67,8 +67,8 @@ def __init__(self, address=None, payload=None, **kwargs):
     #    for example via plain old filesystem access.
     globs = Globs(ParseContext(rel_path=address.spec_path, type_aliases={}))
     sources = globs('*', exclude=[globs('BUILD*'),
-                                  # This skips dirents.
-                                  globs('*/')])
+                                  # This skips subdir content.
+                                  globs('*/**')])
 
     payload = payload or Payload()
     payload.add_fields({

src/python/pants/build_graph/source_mapper.py

@@ -57,8 +57,8 @@ def _find_targets_for_source(self, source, spec_path):
     for address in self._address_mapper.addresses_in_spec_path(spec_path):
       self._build_graph.inject_address_closure(address)
       target = self._build_graph.get_target(address)
-      sources = target.payload.get_field('sources')
-      if self._sources_match(source, sources):
+      sources_field = target.payload.get_field('sources')
+      if sources_field and sources_field.matches(source):
         yield address
       elif self._address_mapper.is_declaring_file(address, source):
         yield address
@@ -71,11 +71,6 @@ def _find_targets_for_source(self, source, spec_path):
             yield address
             break
 
-  def _sources_match(self, source, sources):
-    if not sources:
-      return False
-    return sources.matches(source)
-
 
 class LazySourceMapper(SourceMapper):
   """

src/python/pants/engine/legacy/source_mapper.py

@@ -60,7 +60,7 @@ def _iter_owned_files_from_hydrated_target(self, legacy_target):
     # Handle `sources`-declaring targets.
     target_sources = target_kwargs.get('sources', [])
     if target_sources:
-      for f in target_sources.iter_relative_paths():
+      for f in target_sources.paths_from_buildroot_iter():
         yield f
 
     # Handle `resources`-declaring targets.
@@ -74,7 +74,7 @@ def _iter_owned_files_from_hydrated_target(self, legacy_target):
       #      python_library(..., resources=['file.txt', 'file2.txt'])
       #
       if isinstance(target_resources, EagerFilesetWithSpec):
-        for f in target_resources.iter_relative_paths():
+        for f in target_resources.paths_from_buildroot_iter():
           yield f
       # 2) Strings of addresses, which are represented in kwargs by a list of strings:
       #

src/python/pants/source/payload_fields.py

@@ -8,7 +8,6 @@
 from hashlib import sha1
 
 from pants.base.payload_field import PayloadField
-from pants.source.filespec import matches_filespec
 from pants.source.source_root import SourceRootConfig
 from pants.source.wrapped_globs import FilesetWithSpec
 from pants.util.memo import memoized_property
@@ -33,13 +32,13 @@ def source_root(self):
     # this into the target, rather than have it reach out for global singletons.
     return SourceRootConfig.global_instance().get_source_roots().find_by_path(self.rel_path)
 
+  def matches(self, path):
+    return self.sources.matches(path)
+
   @property
   def filespec(self):
     return self.sources.filespec
 
-  def matches(self, path):
-    return matches_filespec(path, self.filespec)
-
   @property
   def rel_path(self):
     return self.sources.rel_root
@@ -64,7 +63,7 @@ def has_sources(self, extension=None):
 
   def relative_to_buildroot(self):
     """All sources joined with their relative paths."""
-    return list(self.sources.iter_relative_paths())
+    return list(self.sources.paths_from_buildroot_iter())
 
   def _compute_fingerprint(self):
     hasher = sha1()

src/python/pants/source/wrapped_globs.py

@@ -6,7 +6,7 @@
                         unicode_literals, with_statement)
 
 import os
-from abc import abstractproperty
+from abc import abstractmethod, abstractproperty
 from hashlib import sha1
 
 from six import string_types
@@ -30,6 +30,15 @@ def empty(rel_root):
     """Creates an empty FilesetWithSpec object for the given rel_root."""
     return EagerFilesetWithSpec(rel_root, {'globs': []}, tuple(), '<empty>')
 
+  @abstractmethod
+  def matches(self, path_from_buildroot):
+    """
+    Takes in any relative path from build root, and return whether it belongs to this filespec
+
+    :param path_from_buildroot: path relative to build root
+    :return: True if the path matches, else False.
+    """
+
   def __init__(self, rel_root, filespec):
     """
     :param rel_root: The root for the given filespec, relative to the buildroot.
@@ -64,7 +73,7 @@ def __iter__(self):
   def __getitem__(self, index):
     return self.files[index]
 
-  def iter_relative_paths(self):
+  def paths_from_buildroot_iter(self):
     """An alternative `__iter__` that joins files with the relative root."""
     for f in self:
       yield os.path.join(self.rel_root, f)
@@ -75,7 +84,7 @@ def __init__(self, rel_root, filespec, files, files_hash):
     """
     :param rel_root: The root for the given filespec, relative to the buildroot.
     :param filespec: A filespec as generated by `FilesetRelPathWrapper`, which represents
-      what globs or file list it came from.
+      what globs or file list it came from. Must be relative to buildroot.
     :param files: A list of matched files, with declared order and duplicates preserved.
     :param files_hash: A string fingerprint for all files in the fileset.
     """
@@ -94,6 +103,10 @@ def files_hash(self):
   def __repr__(self):
     return 'EagerFilesetWithSpec(rel_root={!r}, files={!r})'.format(self.rel_root, self.files)
 
+  def matches(self, path_from_buildroot):
+    path_relative_to_rel_root = os.path.relpath(path_from_buildroot, self.rel_root)
+    return path_relative_to_rel_root in self._files
+
 
 class LazyFilesetWithSpec(FilesetWithSpec):
   def __init__(self, rel_root, filespec, files_calculator):
@@ -120,6 +133,9 @@ def files_hash(self):
         h.update(f.read())
     return h.digest()
 
+  def matches(self, path_from_buildroot):
+    return any(path_from_buildroot == path_in_spec for path_in_spec in self.paths_from_buildroot_iter())
+
 
 class FilesetRelPathWrapper(AbstractClass):
   KNOWN_PARAMETERS = frozenset(['exclude', 'follow_links'])
@@ -188,6 +204,7 @@ def files_calculator():
       # BUILD file's filesets should contain only files, not folders.
       return [path for path in result
               if not cls.validate_files or os.path.isfile(os.path.join(root, path))]
+
     return files_calculator
 
   @staticmethod

tests/python/pants_test/core_tasks/test_what_changed.py

@@ -78,7 +78,6 @@ def changes_in(_, ds):
 
 
 class WhatChangedTestBasic(BaseWhatChangedTest):
-
   def test_nochanges(self):
     self.assert_console_output(workspace=self.workspace())
 
@@ -300,12 +299,14 @@ def test_diffspec(self):
     )
 
   def test_diffspec_removed_files(self):
+    file_in_target = 'root/src/java/a/b/c/Foo.java'
+    self.create_file(relpath=file_in_target, contents='', mode='a')
     self.assert_console_output(
       'root/src/java/a:a_java',
       options={'diffspec': '42'},
       workspace=self.workspace(
         diffspec='42',
-        diff_files=['root/src/java/a/b/c/Foo.java'],
+        diff_files=[file_in_target],
       ),
     )
 
@@ -375,47 +376,99 @@ def test_deferred_sources_new(self):
     )
 
   def test_rglobs_in_sources(self):
+    file_in_target_a = 'root/src/java/a/foo.java'
+    file_in_target_b = 'root/src/java/a/b/foo.java'
+
+    self.create_file(file_in_target_a, contents='', mode='w')
+    self.create_file(file_in_target_b, contents='', mode='w')
+
     self.assert_console_output(
       'root/src/java/a:a_java',
-      workspace=self.workspace(files=['root/src/java/a/foo.java'])
+      workspace=self.workspace(files=[file_in_target_a])
     )
 
     self.assert_console_output(
       'root/src/java/a:a_java',
-      workspace=self.workspace(files=['root/src/java/a/b/foo.java'])
+      workspace=self.workspace(files=[file_in_target_b])
     )
 
   def test_globs_in_sources(self):
+    file_in_target_a = 'root/src/java/b/foo.java'
+    file_in_target_b = 'root/src/java/b/b/foo.java'
+
+    self.create_file(file_in_target_a, contents='', mode='w')
+    self.create_file(file_in_target_b, contents='', mode='w')
+
     self.assert_console_output(
       'root/src/java/b:b_java',
-      workspace=self.workspace(files=['root/src/java/b/foo.java'])
+      workspace=self.workspace(files=[file_in_target_a])
     )
 
     self.assert_console_output(
       workspace=self.workspace(files=['root/src/java/b/b/foo.java'])
     )
 
-  def test_globs_in_resources(self):
+  def test_globs_in_resources_1(self):
     self.add_to_build_file('root/resources', dedent("""
       resources(
         name='resources',
         sources=globs('*')
       )
     """))
 
+    file_in_target = 'root/resources/foo/bar/baz.yml'
+    self.create_file(file_in_target, contents='', mode='w')
     self.assert_console_output(
-      workspace=self.workspace(files=['root/resources/foo/bar/baz.yml'])
+      workspace=self.workspace(files=[file_in_target])
     )
 
+  def test_globs_in_resources_2(self):
+    self.add_to_build_file('root/resources', dedent("""
+      resources(
+        name='resources',
+        sources=globs('*')
+      )
+    """))
+
+    file_in_target = 'root/resources/baz.yml'
+    self.create_file(file_in_target, contents='', mode='w')
+
     self.assert_console_output(
       'root/resources:resources',
-      workspace=self.workspace(files=['root/resources/baz.yml'])
+      workspace=self.workspace(files=[file_in_target])
     )
 
   def test_root_config(self):
+    file_in_target = 'pants.ini'
+    self.create_file(relpath=file_in_target, contents='', mode='w')
     self.assert_console_output(
       '//:pants-config',
-      workspace=self.workspace(files=['pants.ini'])
+      workspace=self.workspace(files=[file_in_target])
+    )
+
+  def test_exclude_sources(self):
+    self.create_file(relpath='root/resources_exclude/a.png', contents='', mode='w')
+    self.create_file(relpath='root/resources_exclude/dir_a/b.png', contents='', mode='w')
+    self.create_file(relpath='root/resources_exclude/dir_a/dir_b/c.png', contents='', mode='w')
+
+    # Create a resources target that skips subdir contents and BUILD file.
+    self.add_to_build_file('root/resources_exclude/BUILD', dedent("""
+      resources(
+        name='abc',
+        sources=globs('*', exclude=[globs('BUILD*'), globs('*/**')])
+      )
+    """))
+
+    # In target file touched should be reflected in the changed list.
+    self.assert_console_output(
+      'root/resources_exclude:abc',
+      workspace=self.workspace(files=['root/resources_exclude/a.png'])
+    )
+
+    # Changed subdir files should not show up in the changed list.
+    self.assert_console_output(
+      workspace=self.workspace(files=['root/resources_exclude/dir_a/b.png',
+                                      'root/resources_exclude/dir_a/dir_b/c.png'])
     )
 
 

tests/python/pants_test/source/BUILD

@@ -1,14 +1,6 @@
 # Copyright 2015 Pants project contributors (see CONTRIBUTORS.md).
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 
-python_tests(
-  name = 'filespec',
-  sources = ['test_filespec.py'],
-  dependencies = [
-    'src/python/pants/source',
-  ]
-)
-
 python_tests(
   name = 'payload_fields',
   sources = ['test_payload_fields.py'],