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

Fix Go source excludes; Cleanup old filespec matching #4350

Merged
merged 30 commits into from
Mar 27, 2017
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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('*/')])
# skip subdir contents
globs('*/**')])
Copy link
Member

@stuhood stuhood Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that this is a legal glob... should it be */**/*? Should check that it still compiles in latest master now that v2 is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test case added to prove the case.


payload = payload or Payload()
payload.add_fields({
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/source/filespec.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def glob_to_regex(pattern):
:returns: A regex string that matches same paths as the input glob does.
"""
out = ['^']
components = pattern.strip('/').replace('.', '[.]').split('/')
components = pattern.strip('/').replace('.', '[.]').replace('$', '[$]').split('/')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might make sense to consider a more general escape mechanism. This covers $, but there are other non-glob regex characters that can appear in paths.

Initially I was thinking we could just drop re.escape in here, but because we're actually trying to parse out the glob info, I don't think that would work.

Maybe on line: 37 https://github.com/pantsbuild/pants/pull/4350/files#diff-2d528bdfb8ee22a56a1b0660753b7a3cR37

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been operating under the assumption that this code doesn't need to live very long with the v2 engine on the way. But if it does, this should probably switch to pathspec, which we already have a dep on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the time when this logic was introduced, pathspec was still using gitignore syntax which is different from glob matching syntax. I am not sure whether pathspec has evolved to support glob matching syntax since.

Copy link
Contributor Author

@wisechengyi wisechengyi Mar 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Tried pathspec, and it doesn't fit our needs.

doublestar = False
for component in components:
if len(out) == 1:
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/source/payload_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,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(path for path in self.sources.iter_relative_paths() if self.matches(path))

def _compute_fingerprint(self):
hasher = sha1()
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/source/wrapped_globs.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,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.
"""
Expand Down
3 changes: 3 additions & 0 deletions tests/python/pants_test/source/test_filespec.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ def assert_rule_match(self, glob, expected_matches, negate=False):
asserter(re.match(regex, expected), 'glob_to_regex(`{}`) -> `{}` {} path `{}`'
.format(glob, regex, match_state, expected))

def test_glob_to_regex_with_dollar_sign(self):
self.assert_rule_match('a/b$abc*.py', ('a/b$abc.py', ))

def test_glob_to_regex_single_star_0(self):
self.assert_rule_match('a/b/*/f.py', ('a/b/c/f.py', 'a/b/q/f.py'))

Expand Down
7 changes: 6 additions & 1 deletion tests/python/pants_test/source/test_payload_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,12 @@ def test_passes_lazy_fileset_with_spec_through(self):
def test_passes_eager_fileset_with_spec_through(self):
self.create_file('foo/a.txt', 'a_contents')

fileset = EagerFilesetWithSpec('foo', {'globs': ['foo/a.txt']}, ['foo/a.txt'], {'foo/a.txt': b'12345'})
fileset = EagerFilesetWithSpec(rel_root='foo',
# Glob spec is relative to build root
filespec={'globs': ['foo/foo/a.txt']},
# files are relative to `rel_root`
files=['foo/a.txt'],
files_hash={'foo/a.txt': b'12345'})
sf = SourcesField(sources=fileset)

self.assertIs(fileset, sf.sources)
Expand Down