Skip to content
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 subsystem for appending tags to targets from a single source #7315

Merged
merged 3 commits into from Mar 7, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -4,6 +4,7 @@

from __future__ import absolute_import, division, print_function, unicode_literals

import json
import logging
import os
from builtins import object, str
@@ -28,7 +29,8 @@
from pants.source.payload_fields import SourcesField
from pants.source.wrapped_globs import EagerFilesetWithSpec, FilesetWithSpec
from pants.subsystem.subsystem import Subsystem
from pants.util.memo import memoized_property
from pants.util.dirutil import maybe_read_file
from pants.util.memo import memoized_method, memoized_property


logger = logging.getLogger(__name__)
@@ -139,9 +141,47 @@ def check_unknown(self, target, kwargs, payload):
args=''.join('\n {} = {}'.format(key, value) for key, value in unknown_args.items())
))

class TagAssignments(Subsystem):
"""Tags to be applied to targets defined in a single source file."""

options_scope = 'target-tag-assignments'

@classmethod
def register_options(cls, register):
register('--tag-targets-file', type=str, default=None, fingerprint=True,
help='Source JSON file with tag assignments for targets. Ex: \

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 5, 2019

Member

Rather than JSON (which doesn't support comments), it might be preferable have this be a type=dict option, marked fromfile=True.

That way folks could define the value either directly inline in pants.ini, or in a separate file via @fromfile.

Apparently fromfile=True isn't documented here: https://www.pantsbuild.org/options.html#dict-options ... but dict options are at least.

This comment has been minimized.

Copy link
@codealchemy

codealchemy Mar 6, 2019

Author Contributor

Sounds good - I started down that path and got hung up on getting the fromfile option under tests, will push an update once I get that resolved

This comment has been minimized.

Copy link
@codealchemy

codealchemy Mar 6, 2019

Author Contributor

I added d4e53c7 without testing the fromfile option explicitly as there are other tests to show that fromfile works. I can come back around to adding coverage if needed.

{ "tag_targets_mappings": { "tag1": ["path/to/target:foo"] } }')

@classmethod
def tags_for(cls, target_address):
tag_assignments = cls.global_instance()
if tag_assignments.get_options().tag_targets_file:
return tag_assignments._parsed_tag_mappings().get(target_address, [])
else:
return []

@memoized_method
def _parsed_tag_mappings(self):
tag_targets_file = maybe_read_file(self.get_options().tag_targets_file, binary_mode=False)

if tag_targets_file:
parsed_json = json.loads(tag_targets_file)
mappings = parsed_json.get("tag_targets_mappings", {})
return self._invert_tag_targets_mappings(mappings)
else:
return {}

def _invert_tag_targets_mappings(self, parsed_json):
result = {}
for tag, targets in parsed_json.items():
for target in targets:
target_tags = result.setdefault(Address.parse(target).spec, [])
target_tags.append(tag)
return result

@classmethod
def subsystems(cls):
return super(Target, cls).subsystems() + (cls.Arguments,)
return super(Target, cls).subsystems() + (cls.Arguments, cls.TagAssignments)

@classmethod
def get_addressable_type(target_cls):
@@ -312,7 +352,7 @@ def __init__(self, name, address, build_graph, type_alias=None, payload=None, ta
self.address = address
self._build_graph = build_graph
self._type_alias = type_alias
self._tags = set(tags or [])
self._tags = set(tags or []).union(self.TagAssignments.tags_for(address.spec))
self.description = description

self._cached_fingerprint_map = {}
@@ -41,6 +41,7 @@ python_tests(
sources = ['test_build_file_aliases.py'],
dependencies = [
'src/python/pants/build_graph',
'tests/python/pants_test/subsystem:subsystem_utils',
]
)

@@ -11,6 +11,7 @@
from pants.build_graph.build_file_aliases import BuildFileAliases, TargetMacro
from pants.build_graph.mutable_build_graph import MutableBuildGraph
from pants.build_graph.target import Target
from pants_test.subsystem.subsystem_util import init_subsystem


class BuildFileAliasesTest(unittest.TestCase):
@@ -19,6 +20,7 @@ class BlueTarget(Target):
pass

def setUp(self):
init_subsystem(Target.TagAssignments)
self.target_macro_factory = TargetMacro.Factory.wrap(
lambda ctx: ctx.create_object(self.BlueTarget,
type_alias='jill',
@@ -4,6 +4,7 @@

from __future__ import absolute_import, division, print_function, unicode_literals

import json
import os.path
from builtins import range, str
from hashlib import sha1
@@ -19,6 +20,7 @@
from pants.build_graph.target import Target
from pants.build_graph.target_scopes import Scopes
from pants.source.wrapped_globs import Globs
from pants.util.contextutil import temporary_file
from pants_test.subsystem.subsystem_util import init_subsystem
from pants_test.test_base import TestBase

@@ -110,6 +112,30 @@ def test_unknown_kwargs(self):
target = self.make_target('foo:bar', Target, foobar='barfoo')
self.assertFalse(hasattr(target, 'foobar'))

def test_tags_applied_from_configured_json(self):
with temporary_file(binary_mode=False) as fp:
json.dump({
'tag_targets_mappings': {
'special_tag': ['foo:bar', 'path/to/target:foo', 'path/to/target'],
'special_tag2': ['path/to/target:target', '//base:foo']
}
}, fp)
fp.flush()

options = {Target.TagAssignments.options_scope: {'tag_targets_file': fp.name}}
init_subsystem(Target.TagAssignments, options)
target1 = self.make_target('foo:bar', Target, tags=['tag1', 'tag2'])
target2 = self.make_target('path/to/target:foo', Target, tags=['tag1'])
target3 = self.make_target('path/to/target', Target, tags=['tag2'])
target4 = self.make_target('//base:foo', Target, tags=['tag3'])
target5 = self.make_target('baz:qux', Target, tags=['tag3'])

self.assertEqual({'tag1', 'tag2', 'special_tag'}, target1.tags)
self.assertEqual({'tag1', 'special_tag'}, target2.tags)
self.assertEqual({'tag2', 'special_tag', 'special_tag2'}, target3.tags)
self.assertEqual({'tag3', 'special_tag2'}, target4.tags)
self.assertEqual({'tag3'}, target5.tags)

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Mar 5, 2019

Contributor

Things that might be nice to test

  • what happens when a target has the special tag already (probably nothing)
  • what happens when the tag target mapping points to a non-existent target (probably ignore it--differentiating non-existent vs not in the current closure is probably something the subsystem shouldn't worry about)
  • if an invalidly formatted spec is in the source file, what does the error look like (It'd be nice if the error pointed to the JSON file)

This comment has been minimized.

Copy link
@codealchemy

codealchemy Mar 6, 2019

Author Contributor

what happens when a target has the special tag already (probably nothing)

Nothing, I included target5 to show nothing changes if things don't match, but can add more cases if it's helpful

what happens when the tag target mapping points to a non-existent target (probably ignore it--differentiating non-existent vs not in the current closure is probably something the subsystem shouldn't worry about)

It's ignored, added a case for that in the test - though that illustrates that it's possible to define things in the tag mappings that don't do anything, not sure if it'd be preferable to maybe raise an error if something is defined that doesn't match

if an invalidly formatted spec is in the source file, what does the error look like (It'd be nice if the error pointed to the JSON file)

Just switched it to use a dict and fromfile instead of JSON, tried things out with a malformed JSON file and a ParseError was raised

def test_target_id_long(self):
long_path = 'dummy'
for i in range(1,30):
@@ -320,6 +320,7 @@ def setUp(self):

self._build_configuration = self.build_config()
self._inited_target = False
subsystem_util.init_subsystem(Target.TagAssignments)

def buildroot_files(self, relpath=None):
"""Returns the set of all files under the test build root.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.