Add Support for "exclude" to globs in BUILD files

Review Request #3828 — Created May 5, 2016 and submitted

yujiec
pants
3228, 3338
pants-reviews
jsirois, stuhood, zundel

New Engine has to support "exclude" feature in globs to allow us to parse all BUILD files in the pantsbuild/pants repo.

Changes made:
1. in BaseGlobs class, handle "exclude" argument
2. LazyFileContent now takes 2 pathglobs, 1 for matching globs, 1 for excluding globs.
3. "exclude" logic is added to LazyFileContent class.
4. add a testproject "file_set" under testprojects/tests/python/pants/file_sets/. It has 1 BUILD file and multiple empty \*.py files.
5. add new tests in engine/exp/legacy/test_filemap_integration
6. fix a bug in apply_path_wildcard and apply_path_dir_wildcard in fs.py. fnmatch should take basename of a path.

Things to note:
1. Nesting exclude is supported. (ie: exclude=\[globs('...', exclude=\[...\])\])

https://travis-ci.org/pantsbuild/pants/builds/128960483

  • 0
  • 0
  • 2
  • 1
  • 3
Description From Last Updated
  1. 
      
  2. src/python/pants/engine/exp/fs.py (Diff revision 1)
     
     

    Please move comments to the line before the code they're commenting on... when they're at the end of a line like this it's difficult to use complete sentences.

    1. Stu, I am sorry I didn't make this clear, but I didn't mean for it to be a comment.
      actually I want to ask you this: will d.path ends with '/'? I believe it won't but just want to make sure because a trailing slash will make basename output to be empty string.

    2. No, I don't think it will. But you can trust that the inputs are definitely directories in this context, so you could safely append the slash if it isn't there already.

    3. I actually want to do the opposite - remove the trailing slash.
      basename('xxx/yyy') = 'yyy' but basename('xxx/yyy/') = ''.

      But since d.path won't end with '/' I think it is good now.

  3. Can remove the TODO here: I think any other kwarg should be unsupported for now.

  4. src/python/pants/engine/exp/legacy/globs.py (Diff revision 1)
     
     
     

    should use:

    excluded_patterns.extend(exclude)
    

    ... rather than the for loop.

  5. excluded.value will likely be a list/tuple, so this will run in linear time.

    Recommend converting the excluded.value to a set before this comprehension.

    1. done.

    2. oops, should take set out of comprehension.

    3. done.

  6. src/python/pants/source/wrapped_globs.py (Diff revision 1)
     
     
     

    since this doesn't use the cls reference, you can make it a static method:

    @staticmethod
    def process_raw_excludes(raw_excludes):
    
  7. Excellent! Would be good in general to break this into more, smaller tests... that generally helps with debuggability.

  8. 
      
  1. Thanks!

    Please add some additional reviewers... maybe @zundel and @jsirois

    1. Also, would you mind adding a testcase to highlight that item 6 is fixed? Something like this would work:

      self.assert_walk(Files, ['**/3.t*t'], ['a/3.txt'])
      

      (see https://rbcommons.com/s/twitter/r/3840/)

    2. done.

  2. 
      
  1. 
      
  2. This issue has been closed. Please remove it from docstring.

  3. src/python/pants/source/wrapped_globs.py (Diff revision 3)
     
     
     
     
     
     
     
     

    from twitter.common.collections import maybe_list should already do so.

    >>> from twitter.common.collections import maybe_list
    >>> 
    >>> maybe_list('123')
    ['123']
    >>> maybe_list(['123'])
    ['123']
    
    1. I don't think it fits the need here. Element can be string, list of string and also globs object. In case of globs object, maybe_list will throw exception.

    2. maybe_list will apply list conversion for all expected_types, while in this case, for 2 expected types, string - we want to convert it to list, baseglob - we don't.
      Extra logic will need to be applied if we want to use maybe_list which is not worth it. Thus drop the issue.

  4. Might be better to obtain TEST_EXCLUDE_FILES by scanning all the files under testprojects/tests/python/pants/file_sets/, and do an assert to make sure there are no discrepencies/surprises.

    1. are you suggesting adding a new test case to check whether os.walk gives the same result as TEXT_EXCLUDE_FILES?

    2. Yes, because for example if someone else wants adds some files to the test set, it may cause all the tests to fail because some are not expected. Asserting on TEXT_EXCLUDE_FILES (better at setup stage if all tests are independent) could help send a clear signal that these tests are expecting this set of files. It is not an issue for 1 or 2 files, but it is hard to tell in this case just by eyeballing.

    3. makes sense.
      done.

  5. 
      
  1. I looked over the tests and they look good to me.

  2. 
      
  1. Thanks!

  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as a5edfe0b335c6bb59feea50fc6822c70cc76a880

Loading...