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 Conjunction field to PathGlobs and use it to remove false positive glob expansion failure warnings #6278

Merged
merged 24 commits into from Aug 29, 2018

Conversation

Projects
None yet
3 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Jul 30, 2018

Problem

See #6236.

Solution

  • Introduce the enum wrapper for datatypes which have a known, finite set of values, and apply it to GlobMatchErrorBehavior.
  • Add Conjunction enum to PathGlobs in python and rust.
  • Print warnings unconditionally to stderr from rust.
  • Decide whether the PathGlobs match succeeded or failed depending on the value of the Conjunction object.

Result

  • Warnings or errors don't display if a target doesn't match every one of its default_sources_globs.
  • Resolves #6236.

@cosmicexplorer cosmicexplorer requested review from stuhood , benjyw and illicitonion Jul 30, 2018

@@ -9,7 +9,7 @@
from pants.base.project_tree import Dir, File
from pants.engine.rules import RootRule
from pants.option.global_options import GlobMatchErrorBehavior
from pants.util.objects import Collection, datatype
from pants.util.objects import Collection, datatype, enum

This comment has been minimized.

@stuhood

stuhood Jul 30, 2018

Member

IMO, the enum bit is out of scope for a bugfix like this. Would be good to do this with a simple fix, and then discuss adding more datatype features later.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 11, 2018

Contributor

I specifically noted that the enum method should be considered experimental and unstable in its docstring in f6172a9!

@stuhood
Copy link
Member

stuhood left a comment

Looking again, the enum bit doesn't add much total volume. Fine with landing it as is, but please add a docstring for enum that indicates that it is experimental.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:path-globs-conjuction-selector branch from 1113d3d to 926415f Aug 11, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Aug 11, 2018

I fixed many of the errors that caused everything to fail before, but I'm getting errors running some of the engine tests locally on my linux box with ./pants -ldebug test tests/python/pants_test/engine:: like:

  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
                     
                     self = <pants.engine.native.Native object at 0x7fc22f8da810>
                     
                         @memoized_property
                         def binary(self):
                           """Load and return the path to the native engine binary."""
                           lib_name = '{}.so'.format(NATIVE_ENGINE_MODULE)
                           lib_path = os.path.join(safe_mkdtemp(), lib_name)
                           with closing(pkg_resources.resource_stream(__name__, lib_name)) as input_fp:
                             # NB: The header stripping code here must be coordinated with header insertion code in
                             #     build-support/bin/native/bootstrap_code.sh
                             engine_version = input_fp.readline().decode('utf-8').strip()
                             repo_version = input_fp.readline().decode('utf-8').strip()
                             logger.debug('using {} built at {}'.format(engine_version, repo_version))
                             with open(lib_path, 'wb') as output_fp:
                     >         output_fp.write(input_fp.read())
                     E         IOError: [Errno 28] No space left on device

with the code at 4ee1dad. I'll investigate.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:path-globs-conjuction-selector branch from 4ee1dad to e642f69 Aug 20, 2018

@cosmicexplorer cosmicexplorer changed the title [WIP] Add Conjunction field to PathGlobs and use it to remove false positive glob expansion failure warnings Add Conjunction field to PathGlobs and use it to remove false positive glob expansion failure warnings Aug 21, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Aug 21, 2018

This is no longer WIP and is ready for review.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:path-globs-conjuction-selector branch from ac6eacc to c026b0b Aug 21, 2018

cosmicexplorer added some commits Jul 30, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@@ -306,3 +307,8 @@ def create(cls, value):

def __repr__(self):
return '{} {}'.format(self.action, self.val)


class Conjunction(enum('conjunction', ['or', 'and'])):

This comment has been minimized.

@stuhood

stuhood Aug 21, 2018

Member

While having a Conjuction enum is elegant, I'm a bit concerned that it obfuscates what it is being used for in the context of PathGlobs expansion.

I think that it would probably be much clearer for this enum to be something like "all_match" or "any_match", even if that meant that it was a single-use type. That would make it significantly more self-documenting.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 21, 2018

Contributor

Done in b5eec2d! Totally agreed that this is much clearer, I definitely don't want to introduce "algebra" we don't need yet -- my intention with the class was for this sole purpose so I'm 100% behind the rename.

@@ -442,33 +442,53 @@ impl StrictGlobMatching {
}
}

#[derive(Debug)]
pub enum Conjunction {

This comment has been minimized.

@stuhood

stuhood Aug 21, 2018

Member

Ditto making this a single-use, self-documenting type rather than a generic conjunction.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 21, 2018

Contributor

Also done in b5eec2d!

warn!("{}", msg);
// NB: warn!() is blocked when using a console task e.g. list, so we print
// unconditionally to stderr here.
eprintln!("WARN] {}", msg);

This comment has been minimized.

@stuhood

stuhood Aug 21, 2018

Member

This change isn't related to the rest of the PR, and should probably land separately. It significantly affects the verbosity of pantsd runs.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 21, 2018

Contributor

0aa2db3 reverts this. I will make a new issue in a second to track the underlying problem.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 21, 2018

Contributor

I also think...there may not even need to be a followup issue, as all the tests are passing without any being skipped? Will try to gain some more clarity.

cosmicexplorer added some commits Aug 21, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks.

@@ -313,6 +322,8 @@ trait GlobMatchingImplementation<E: Send + Sync + 'static>: VFS<E> {
} else {
// TODO(#5683): this doesn't have any useful context (the stack trace) without
// being thrown -- this needs to be provided, otherwise this is unusable.
// NB: warn!() is blocked when using a console task e.g. list, so we print

This comment has been minimized.

@stuhood

stuhood Aug 21, 2018

Member

Stale comment.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 22, 2018

Contributor

Removed in d029419!

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:path-globs-conjuction-selector branch from c858173 to d029419 Aug 22, 2018

cosmicexplorer added some commits Aug 22, 2018

@stuhood stuhood modified the milestones: 1.9.x, 1.8.x Aug 23, 2018

cosmicexplorer added some commits Aug 25, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 28, 2018

This bug is still affecting the 1.8.x branch, so would be awesome to get this in if possible. Just restarted the travis shards, in case they were flaky.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Aug 29, 2018

I was under the impression there were a lot more errors to solve and it looks like they were almost all flaky. The remaining python 3 error should be fixed, will check CI as it runs to make sure.

@cosmicexplorer cosmicexplorer merged commit 5155a70 into pantsbuild:master Aug 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ity ity removed the needs-cherrypick label Sep 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment