source attribute is automatically promoted to sources (#5908)

This means that either the `source` or `sources` attribute can be used
for any rule which expects sources. Places that `source` was expected
still verify that the correct number of sources are actually present.
Places that `sources` was expected will automatically promote `source`
to `sources`.

This is a step towards all `sources` attributes being
`EagerFilesetWithSpec`s, which will make them cached in the daemon, and
make them easier to work with with both v2 remote execution and in the
v2 engine in general. It also provides common hooks for input file
validation, rather than relying on them being done ad-hoc in each
`Target` constructor.

For backwards compatibility, both attributes will be populated on
`Target`s, but in the future only the sources value will be provided.

`sources` is guaranteed to be an `EagerFilesetWithSpec` whichever of
these mechanisms is used.

A hook is provided for rules to perform validation on `sources` at build
file parsing time. Hooks are put in place for non-contrib rule types
which currently take a `source` attribute to verify that the correct
number of sources are provided. I imagine at some point we may want to
add a "file type" hook too, so that rules can error if files of the
wrong type were added as sources.

This is a breaking change for rules which use both the `source` and `sources` attributes (and where the latter is not equivalent to the former), or where the `source` attribute is used to refer to something other than a file. `source` is now becoming a
reserved attribute name, as `sources` and `dependencies` already are.

This is also a breaking change for rules which use the `source`
attribute, but never set `sources` in a Payload. These will now fail to
parse.

This is also a slightly breaking change for the `page` rule - before,
omitting the `source` attribute would parse, but fail at runtime. Now,
it will fail to parse.

This is also a breaking change in that in means that the source
attribute is now treated like a glob, and so if a file is specified
which isn't present, it will be ignored instead of error. This feels a
little sketchy, but it turns out we did the exact same thing by making
all sources lists be treated like globs...

src/python/pants/backend/docgen/targets/doc.py

@@ -78,15 +78,15 @@ def _compute_fingerprint(self):
       return combine_hashes(artifact.fingerprint() for artifact in self)
 
   def __init__(self,
+               sources,
                address=None,
                payload=None,
-               source=None,
                format=None,
                links=None,
                provides=None,
                **kwargs):
     """
-    :param source: Path to page source file.
+    :param sources: Page source file. Exactly one will be present.
     :param format: Page's format, ``md`` or ``rst``. By default, Pants infers from ``source`` file
        extension: ``.rst`` is ReStructured Text; anything else is Markdown.
     :param links: Other ``page`` targets that this `page` links to.
@@ -97,12 +97,12 @@ def __init__(self,
     """
     payload = payload or Payload()
     if not format:
-      if source and source.lower().endswith('.rst'):
+      if sources.files[0].lower().endswith('.rst'):
         format = 'rst'
       else:
         format = 'md'
     payload.add_fields({
-      'sources': self.create_sources_field(sources=[source],
+      'sources': self.create_sources_field(sources=sources,
                                            sources_rel_path=address.spec_path,
                                            key_arg='sources'),
       'format': PrimitiveField(format),

src/python/pants/backend/jvm/targets/jvm_binary.py

@@ -302,7 +302,7 @@ def __init__(self,
                payload=None,
                main=None,
                basename=None,
-               source=None,
+               sources=None,
                deploy_excludes=None,
                deploy_jar_rules=None,
                manifest_entries=None,
@@ -317,8 +317,8 @@ def __init__(self,
     :param string basename: Base name for the generated ``.jar`` file, e.g.,
       ``'hello'``. (By default, uses ``name`` param)  Note this is unsafe
       because of the possible conflict when multiple binaries are built.
-    :param string source: Name of one ``.java`` or ``.scala`` file (a good
-      place for a ``main``).
+    :param EagerFilesetWithSpec sources: Zero or one source files. If more than one source is
+      required, they should be put in a library target which should be added to dependencies.
     :param dependencies: Targets (probably ``java_library`` and
      ``scala_library`` targets) to "link" in.
     :type dependencies: list of target specs
@@ -340,8 +340,6 @@ def __init__(self,
     self.address = address  # Set in case a TargetDefinitionException is thrown early
     if main and not isinstance(main, string_types):
       raise TargetDefinitionException(self, 'main must be a fully qualified classname')
-    if source and not isinstance(source, string_types):
-      raise TargetDefinitionException(self, 'source must be a single relative file path')
     if deploy_jar_rules and not isinstance(deploy_jar_rules, JarRules):
       raise TargetDefinitionException(self,
                                       'deploy_jar_rules must be a JarRules specification. got {}'
@@ -350,13 +348,6 @@ def __init__(self,
       raise TargetDefinitionException(self,
                                       'manifest_entries must be a dict. got {}'
                                       .format(type(manifest_entries).__name__))
-    sources = [source] if source else None
-    if 'sources' in kwargs:
-      raise self.IllegalArgument(address.spec,
-        'jvm_binary only supports a single "source" argument, typically used to specify a main '
-        'class source file. Other sources should instead be placed in a java_library, which '
-        'should be referenced in the jvm_binary\'s dependencies.'
-      )
     payload = payload or Payload()
     payload.add_fields({
       'basename': PrimitiveField(basename or name),
@@ -372,7 +363,7 @@ def __init__(self,
     super(JvmBinary, self).__init__(name=name,
                                     address=address,
                                     payload=payload,
-                                    sources=self.assert_list(sources, key_arg='sources'),
+                                    sources=sources,
                                     **kwargs)
 
   @property

src/python/pants/backend/python/targets/python_binary.py

@@ -35,7 +35,7 @@ def alias(cls):
   # TODO(wickman) Consider splitting pex options out into a separate PexInfo builder that can be
   # attached to the binary target.  Ideally the PythonBinary target is agnostic about pex mechanics
   def __init__(self,
-               source=None,
+               sources=None,
                entry_point=None,
                inherit_path=False,        # pex option
                zip_safe=True,             # pex option
@@ -47,16 +47,13 @@ def __init__(self,
                platforms=(),
                **kwargs):
     """
-    :param source: relative path to one python source file that becomes this
-      binary's __main__.
-      If None specified, drops into an interpreter by default.
     :param string entry_point: the default entry point for this binary.  if None, drops into the entry
       point that is defined by source. Something like
       "pants.bin.pants_exe:main", where "pants.bin.pants_exe" is the package
       name and "main" is the function name (if ommitted, the module is
       executed directly, presuming it has a ``__main.py__``).
-    :param sources: Overridden by source. To specify more than one source file,
-      use a python_library and have the python_binary depend on that library.
+    :param sources: Zero or one source files. If more than one file is required, it should be put in
+      a python_library which should be added to dependencies.
     :param inherit_path: inherit the sys.path of the environment that this binary runs in
     :param zip_safe: whether or not this binary is safe to run in compacted (zip-file) form
     :param always_write_cache: whether or not the .deps cache of this PEX file should always
@@ -90,17 +87,16 @@ def __init__(self,
       'shebang': PrimitiveField(shebang),
     })
 
-    sources = [] if source is None else [source]
     super(PythonBinary, self).__init__(sources=sources, payload=payload, **kwargs)
 
-    if source is None and entry_point is None:
+    if (not sources or not sources.files) and entry_point is None:
       raise TargetDefinitionException(self,
-          'A python binary target must specify either source or entry_point.')
+          'A python binary target must specify either a single source or entry_point.')
 
     if not isinstance(platforms, (list, tuple)) and not isinstance(platforms, string_types):
       raise TargetDefinitionException(self, 'platforms must be a list, tuple or string.')
 
-    if source and entry_point:
+    if sources and sources.files and entry_point:
       entry_point_module = entry_point.split(':', 1)[0]
       entry_source = list(self.sources_relative_to_source_root())[0]
       source_entry_point = self._translate_to_entry_point(entry_source)

src/python/pants/backend/python/targets/python_target.py

@@ -39,7 +39,7 @@ def __init__(self,
     :type dependencies: list of strings
     :param sources: Files to "include". Paths are relative to the
       BUILD file's directory.
-    :type sources: ``Fileset`` or list of strings
+    :type sources: ``EagerFilesetWithSpec``
     :param provides:
       The `setup_py <#setup_py>`_ to publish that represents this
       target outside the repo.

src/python/pants/build_graph/target.py

@@ -13,6 +13,7 @@
 from twitter.common.collections import OrderedSet, maybe_list
 
 from pants.base.build_environment import get_buildroot
+from pants.base.deprecated import deprecated_conditional
 from pants.base.exceptions import TargetDefinitionException
 from pants.base.fingerprint_strategy import DefaultFingerprintStrategy
 from pants.base.hash_utils import hash_all
@@ -100,18 +101,39 @@ def register_options(cls, register):
                     'in BUILD files that are not yet available in the current version of pants.')
 
     @classmethod
-    def check(cls, target, kwargs):
+    def check(cls, target, kwargs, payload):
       """
       :API: public
       """
-      cls.global_instance().check_unknown(target, kwargs)
+      cls.global_instance().check_unknown(target, kwargs, payload)
 
-    def check_unknown(self, target, kwargs):
+    def check_unknown(self, target, kwargs, payload):
       """
       :API: public
       """
       ignore_params = set((self.get_options().ignored or {}).get(target.type_alias, ()))
       unknown_args = {arg: value for arg, value in kwargs.items() if arg not in ignore_params}
+      if 'source' in unknown_args:
+        if 'sources' in payload.as_dict():
+          deprecated_conditional(
+            lambda: True,
+            '1.10.0.dev0',
+            ('The source argument in targets is deprecated - it gets automatically promoted to '
+            'sources. Target {} should just use a sources argument. No BUILD files need changing. '
+            'The source argument will stop being populated -').format(target.type_alias),
+          )
+          unknown_args.pop('source')
+      if 'sources' in unknown_args:
+        if 'sources' in payload.as_dict():
+          deprecated_conditional(
+            lambda: True,
+            '1.10.0.dev0',
+            ('The source argument is deprecated - it gets automatically promoted to sources.'
+             'Target {} should just use a sources argument. No BUILD files need changing. '
+             'The source argument will stop being populated -').format(target.type_alias),
+          )
+          unknown_args.pop('sources')
+          kwargs.pop('sources')
       ignored_args = {arg: value for arg, value in kwargs.items() if arg in ignore_params}
       if ignored_args:
         logger.debug('{target} ignoring the unimplemented arguments: {args}'
@@ -308,7 +330,7 @@ def __init__(self, name, address, build_graph, type_alias=None, payload=None, ta
     self._cached_exports_addresses = None
     self._no_cache = no_cache
     if kwargs:
-      self.Arguments.check(self, kwargs)
+      self.Arguments.check(self, kwargs, self.payload)
 
   @property
   def scope(self):
@@ -826,6 +848,8 @@ def default_sources(cls, sources_rel_path):
                                             exclude=exclude)
     return None
 
+  # TODO: Inline this as SourcesField(sources=sources) when all callers are guaranteed to pass an
+  # EagerFilesetWithSpec.
   def create_sources_field(self, sources, sources_rel_path, key_arg=None):
     """Factory method to create a SourcesField appropriate for the type of the sources object.
 

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

@@ -392,6 +392,7 @@ def hydrate_sources(sources_field, glob_match_error_behavior):
     sources_field.address.spec_path,
     sources_field.filespecs,
     snapshot)
+  sources_field.validate_fn(fileset_with_spec)
   yield HydratedField(sources_field.arg, fileset_with_spec)
 
 

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

@@ -12,6 +12,7 @@
 from six import string_types
 
 from pants.base.deprecated import deprecated_conditional
+from pants.build_graph.target import Target
 from pants.engine.addressable import addressable_list
 from pants.engine.fs import PathGlobs
 from pants.engine.objects import Locatable
@@ -38,7 +39,24 @@ def get_sources(self):
     refactor how deferred sources are implemented.
       see: https://github.com/pantsbuild/pants/issues/2997
     """
+    source = getattr(self, 'source', None)
     sources = getattr(self, 'sources', None)
+
+    if source is not None and sources is not None:
+      raise Target.IllegalArgument(
+        self.address.spec,
+        'Cannot specify both source and sources attribute.'
+      )
+
+    if source is not None:
+      if not isinstance(source, string_types):
+        raise Target.IllegalArgument(
+          self.address.spec,
+          'source must be a string containing a path relative to the target, but got {} of type {}'
+            .format(source, type(source))
+        )
+      sources = [source]
+
     # N.B. Here we check specifically for `sources is None`, as it's possible for sources
     # to be e.g. an explicit empty list (sources=[]).
     if sources is None and self.default_sources_globs is not None:
@@ -56,7 +74,14 @@ def field_adaptors(self):
         return tuple()
       base_globs = BaseGlobs.from_sources_field(sources, self.address.spec_path)
       path_globs = base_globs.to_path_globs(self.address.spec_path)
-      return (SourcesField(self.address, 'sources', base_globs.filespecs, base_globs, path_globs),)
+      return (SourcesField(
+        self.address,
+        'sources',
+        base_globs.filespecs,
+        base_globs,
+        path_globs,
+        self.validate_sources,
+      ),)
 
   @property
   def default_sources_globs(self):
@@ -66,12 +91,29 @@ def default_sources_globs(self):
   def default_sources_exclude_globs(self):
     return None
 
+  def validate_sources(self, sources):
+    """"
+    Validate that the sources argument is allowed.
+
+    Examples may be to check that the number of sources is correct, that file extensions are as
+    expected, etc.
+
+    TODO: Replace this with some kind of field subclassing, as per
+    https://github.com/pantsbuild/pants/issues/4535
+
+    :param sources EagerFilesetWithSpec resolved sources.
+    """
+    pass
+
 
 class Field(object):
   """A marker for Target(Adaptor) fields for which the engine might perform extra construction."""
 
 
-class SourcesField(datatype(['address', 'arg', 'filespecs', 'base_globs', 'path_globs']), Field):
+class SourcesField(
+  datatype(['address', 'arg', 'filespecs', 'base_globs', 'path_globs', 'validate_fn']),
+  Field
+):
   """Represents the `sources` argument for a particular Target.
 
   Sources are currently eagerly computed in-engine in order to provide the `BuildGraph`
@@ -84,6 +126,8 @@ class SourcesField(datatype(['address', 'arg', 'filespecs', 'base_globs', 'path_
     case of python resource globs.
   :param filespecs: The merged filespecs dict the describes the paths captured by this field.
   :param path_globs: A PathGlobs describing included files.
+  :param validate_fn: A function which takes an EagerFilesetWithSpec and throws if it's not
+    acceptable. This API will almost certainly change in the near future.
   """
 
   def __hash__(self):
@@ -126,6 +170,29 @@ def default_sources_globs(self):
     return self.java_test_globs + self.scala_test_globs
 
 
+class JvmBinaryAdaptor(TargetAdaptor):
+  def validate_sources(self, sources):
+    if len(sources.files) > 1:
+      raise Target.IllegalArgument(self.address.spec,
+                'jvm_binary must have exactly 0 or 1 sources (typically used to specify the class '
+                'containing the main method). '
+                'Other sources should instead be placed in a java_library, which '
+                'should be referenced in the jvm_binary\'s dependencies.'
+              )
+
+
+class PageAdaptor(TargetAdaptor):
+  def validate_sources(self, sources):
+    if len(sources.files) != 1:
+      raise Target.IllegalArgument(
+        self.address.spec,
+        'page targets must have exactly 1 source, but found {} ({})'.format(
+          len(sources.files),
+          ', '.join(sources.files),
+        )
+      )
+
+
 class BundlesField(datatype(['address', 'bundles', 'filespecs_list', 'path_globs_list']), Field):
   """Represents the `bundles` argument, each of which has a PathGlobs to represent its `fileset`."""
 
@@ -211,10 +278,22 @@ def field_adaptors(self):
                                    'resources',
                                    base_globs.filespecs,
                                    base_globs,
-                                   path_globs)
+                                   path_globs,
+                                   lambda _: None)
       return field_adaptors + (sources_field,)
 
 
+class PythonBinaryAdaptor(PythonTargetAdaptor):
+  def validate_sources(self, sources):
+    if len(sources.files) > 1:
+      raise Target.IllegalArgument(self.address.spec,
+        'python_binary must have exactly 0 or 1 sources (typically used to specify the file '
+        'containing the entry point). '
+        'Other sources should instead be placed in a python_library, which '
+        'should be referenced in the python_binary\'s dependencies.'
+      )
+
+
 class PythonLibraryAdaptor(PythonTargetAdaptor):
   @property
   def default_sources_globs(self):

src/python/pants/init/engine_initializer.py

@@ -18,9 +18,10 @@
 from pants.engine.legacy.options_parsing import create_options_parsing_rules
 from pants.engine.legacy.parser import LegacyPythonCallbacksParser
 from pants.engine.legacy.structs import (AppAdaptor, GoTargetAdaptor, JavaLibraryAdaptor,
-                                         JunitTestsAdaptor, PythonLibraryAdaptor,
-                                         PythonTargetAdaptor, PythonTestsAdaptor,
-                                         RemoteSourcesAdaptor, ScalaLibraryAdaptor, TargetAdaptor)
+                                         JunitTestsAdaptor, JvmBinaryAdaptor, PageAdaptor,
+                                         PythonBinaryAdaptor, PythonLibraryAdaptor,
+                                         PythonTestsAdaptor, RemoteSourcesAdaptor,
+                                         ScalaLibraryAdaptor, TargetAdaptor)
 from pants.engine.mapper import AddressMapper
 from pants.engine.native import Native
 from pants.engine.parser import SymbolTable
@@ -62,10 +63,12 @@ def __init__(self, build_file_aliases):
 
     self._table['junit_tests'] = JunitTestsAdaptor
     self._table['jvm_app'] = AppAdaptor
+    self._table['jvm_binary'] = JvmBinaryAdaptor
     self._table['python_app'] = AppAdaptor
     self._table['python_tests'] = PythonTestsAdaptor
-    self._table['python_binary'] = PythonTargetAdaptor
+    self._table['python_binary'] = PythonBinaryAdaptor
     self._table['remote_sources'] = RemoteSourcesAdaptor
+    self._table['page'] = PageAdaptor
 
   def aliases(self):
     return self._build_file_aliases