Fix Go source excludes; Cleanup old filespec matching #4350

wisechengyi · 2017-03-21T01:10:17Z

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)

stuhood · 2017-03-23T18:04:55Z

@wisechengyi : Sorry for the delayed review. This looks correct, but there is one more improvement while you're in here:

This method is used (recursively) from https://github.com/pantsbuild/pants/blob/master/src/python/pants/build_graph/source_mapper.py#L61 : if you push the match implementation down onto Filespec (https://github.com/pantsbuild/pants/blob/master/src/python/pants/source/wrapped_globs.py#L21), you can have EagerFilesetWithSpec do a lookup in its file list, and LazyFilesetWithSpec use this implementation.

The v2 engine only creates EagerFilesetWithSpec, because it is able to cache them in the daemon. So if you encounter an EagerFilesetWithSpec, it is "free" to use the file list, and definitely cheaper than compiling regexes to match the filespecs.

stuhood · 2017-03-23T18:09:20Z

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

-                                  # This skips dirents.
-                                  globs('*/')])
+                                  # skip subdir contents
+                                  globs('*/**')])


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.

test case added to prove the case.

baroquebobcat · 2017-03-23T18:11:30Z

src/python/pants/source/filespec.py

@@ -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('/')


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

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.

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.

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

wisechengyi · 2017-03-24T06:56:18Z

@wisechengyi : Sorry for the delayed review. This looks correct, but there is one more improvement while you're in here:

This method is used (recursively) from https://github.com/pantsbuild/pants/blob/master/src/python/pants/build_graph/source_mapper.py#L61 : if you push the match implementation down onto Filespec (https://github.com/pantsbuild/pants/blob/master/src/python/pants/source/wrapped_globs.py#L21), you can have EagerFilesetWithSpec do a lookup in its file list, and LazyFilesetWithSpec use this implementation.

The v2 engine only creates EagerFilesetWithSpec, because it is able to cache them in the daemon. So if you encounter an EagerFilesetWithSpec, it is "free" to use the file list, and definitely cheaper than compiling regexes to match the filespecs.

def iter_relative_paths is now an abstract class and LazyFilesetWithSpec has the new implementation.

benjyw · 2017-03-24T15:10:21Z

src/python/pants/source/wrapped_globs.py

@@ -91,6 +91,11 @@ def files(self):
  def files_hash(self):
    return self._files_hash

+  def iter_relative_paths(self):


Better to call this paths_from_buildroot_iter() (and below).

relative is a vague term.

JieGhost

The regex improvement looks good to me. I am fine with leave it as it is, but I will listen to other reviewers' opinion on escape mechanism.

stuhood · 2017-03-24T20:10:59Z

src/python/pants/source/wrapped_globs.py

+    """An alternative `__iter__` that joins files with the relative root."""
+    for f in self:
+      path_from_buildroot = os.path.join(self.rel_root, f)
+      if matches_filespec(path_from_buildroot, self.filespec):


This is redundant. The files in the collection are already guaranteed to match the filespec: the filespec is just metadata about how the fileset was constructed.

The point of the SpecAddressMapper is to not expand the fileset, and to just use the filespec instead. This was based on the assumption that all FilesetWithSpec implementations were lazy.

What I was suggesting was an abstract method on FilesetWithSpec something like:

def matches(self, paths): """Returns the from the given set that match this FilesetWithSpec."""

...and the implementation for Eager would be to just look in the fileset, while the implementation for Lazy would be to compile the regex from the filespec and use that.

Corrected. Pushed matches to FilesetWithSpec, and removed all the regex logic.

stuhood · 2017-03-27T19:24:26Z

As discussed offline, this now removes the optimization represented by https://github.com/pantsbuild/pants/blob/master/src/python/pants/build_graph/source_mapper.py#L21 ... that optimization was only relevant to v1, so I think that I'm ok with this. People should see much more benefit by using the daemon + v2.

But please either clean up or remove SpecSourceMapper, because it no longer uses the spec (and that's ok).

stuhood

Thanks Yi.

### 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)

### Problem #4529 There are 2 kinds of "changed" tasks in pants currently. Neither of them output correct result if a file is deleted. 1. "./pants changed" tasks. They used to have the correct output, but after #4350 , the logic of file matching in SpecSourceMapper was changed. We used to do a regular expression matching to check if the modified file is owned by a target. Now we check if a modified file is in a target's source file set. For a deleted file, it will not be in any target's source file set, thus no target will be considered as the owner of the deleted file, giving incorrect result. 2. "./pants --changed-XXX list". This uses EngineSourceMapper which implements similar logic as above. It never works on deleted files. ### Solution I restored the removed file matching logic (using regular expression). In both SpecSourceMapper and EngineSourceMapper, first check if a file exists. It it exists, then check if it's in target's file set. If not, then it is a deleted file, and use re matching logic on it. Fixes #4529

wisechengyi added 8 commits March 20, 2017 18:06

getting source paths with respect to exclude

430c5a1

remove go

405e119

comments

055dbfc

oh regex....

227aa8c

more doc

f5a661f

$$$$$$$

40d37b5

add test

62700f1

star

9664a29

wisechengyi changed the title ~~Getting sources in SourceField should respect target spec~~ Have getting sources from SourceField respect target spec; Fix Go source excludes Mar 21, 2017

wisechengyi changed the title ~~Have getting sources from SourceField respect target spec; Fix Go source excludes~~ Have SourceField.relative_to_buildroot respect target spec; Fix Go source excludes Mar 21, 2017

wisechengyi added 2 commits March 20, 2017 23:48

sha

a358cd6

Merge branch 'master' into go_fix_2

495623e

wisechengyi mentioned this pull request Mar 21, 2017

fix glob_to_regex #4351

Closed

wisechengyi added 3 commits March 21, 2017 13:56

Merge branch 'master' into go_fix_2

f0f3513

new

25dfc5d

format

6b6cc3e

wisechengyi requested review from stuhood, benjyw, JieGhost and baroquebobcat March 21, 2017 22:17

stuhood reviewed Mar 23, 2017

View reviewed changes

baroquebobcat reviewed Mar 23, 2017

View reviewed changes

wisechengyi added 6 commits March 23, 2017 17:22

Merge branch 'master' into go_fix_2

16a576a

add more test

9e83138

format

3213d70

rev

0dc9db8

remove private

18c91d4

push down

a501d6a

benjyw reviewed Mar 24, 2017

View reviewed changes

JieGhost approved these changes Mar 24, 2017

View reviewed changes

stuhood requested changes Mar 24, 2017

View reviewed changes

wisechengyi added 4 commits March 24, 2017 17:12

cleanup a few things

8a231a6

rename

6e8e7dc

remove

a8eb245

import

1ad6a97

wisechengyi changed the title ~~Have SourceField.relative_to_buildroot respect target spec; Fix Go source excludes~~ Fix Go source excludes Mar 25, 2017

wisechengyi changed the title ~~Fix Go source excludes~~ Fix Go source excludes; Cleanup old filespec matching Mar 25, 2017

wisechengyi added 7 commits March 24, 2017 23:20

Merge branch 'master' into go_fix_2

39cba5a

add filespec

db98f0b

remove empty filespec target

25385a4

-fix tests with actual files

867057a

fix exclude

66ecd63

add test and comments

8435198

fmt

5df0d63

stuhood approved these changes Mar 27, 2017

View reviewed changes

wisechengyi mentioned this pull request Mar 27, 2017

clean up SpecSourceMapper #4391

Closed

wisechengyi merged commit a7c5542 into pantsbuild:master Mar 27, 2017

wisechengyi deleted the go_fix_2 branch March 27, 2017 19:27

This was referenced Apr 28, 2017

pants changed missed a target changed by a deleted file #4529

Closed

Make "changed" tasks work with deleted files #4546

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Go source excludes; Cleanup old filespec matching #4350

Fix Go source excludes; Cleanup old filespec matching #4350

wisechengyi commented Mar 21, 2017 •

edited

Loading

stuhood commented Mar 23, 2017

stuhood Mar 23, 2017 •

edited

Loading

wisechengyi Mar 24, 2017

baroquebobcat Mar 23, 2017

stuhood Mar 23, 2017

JieGhost Mar 23, 2017

wisechengyi Mar 24, 2017 •

edited

Loading

wisechengyi commented Mar 24, 2017

benjyw Mar 24, 2017

stuhood Mar 24, 2017

wisechengyi Mar 25, 2017

JieGhost left a comment

stuhood Mar 24, 2017

wisechengyi Mar 25, 2017

stuhood commented Mar 27, 2017

stuhood left a comment

Fix Go source excludes; Cleanup old filespec matching #4350

Fix Go source excludes; Cleanup old filespec matching #4350

Conversation

wisechengyi commented Mar 21, 2017 • edited Loading

Problem

Other Change

stuhood commented Mar 23, 2017

stuhood Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wisechengyi Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

wisechengyi commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JieGhost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood commented Mar 27, 2017

stuhood left a comment

Choose a reason for hiding this comment

wisechengyi commented Mar 21, 2017 •

edited

Loading

stuhood Mar 23, 2017 •

edited

Loading

wisechengyi Mar 24, 2017 •

edited

Loading