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

Temporarily restore recursive behaviour for bundle filesets #4630

Merged
merged 5 commits into from May 27, 2017

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

stuhood commented May 27, 2017

Problem

The fileset argument to jvm_app bundles is approximately like the sources argument on a target, with the exception that if a directory is matched, its contents are implicitly recursively included. Usually triggering recursive inclusion requires **/* wildcarding or rglobs.

Additionally, the v2 engine broke the recursive inclusion by excluding directories from the FilesetWithSpec that was created in the case of a bundle.

Solution

Restore capturing directories for FilesetWithSpecs (but only in the context of bundles, for now), and deprecate the inconsistent default-recursive behaviour of the fileset argument.

This is attempt two for this patch. The primary differences are:

  1. paths are created from shortest to longest, such that explicit directory symlinks are always created before their contents if both are matched
  2. collisions between files is allowed rather than exploding (in case a parent symlinked directory contains a child matched file). This is also allowed in source globs... matching a file twice is fine.

Result

Users are encouraged to switch to explicit recursive globs, which will allow us to remove the inconsistent behavior of the fileset argument. Fixes #4622.

stuhood added some commits May 26, 2017

Temporarily restore recursive behaviour for bundle filesets (#4624)
### Problem

The `fileset` argument to `jvm_app` `bundles` is approximately like the `sources` argument on a target, with the exception that if a directory is matched, its contents are implicitly recursively included. Usually triggering recursive inclusion requires `**/*` wildcarding or `rglobs`.

Additionally, the v2 engine broke the recursive inclusion by excluding directories from the `FilesetWithSpec` that was created in the case of a `bundle`.

### Solution

Restore capturing directories for `FilesetWithSpec`s (but only in the context of bundles, for now), and deprecate the inconsistent default-recursive behaviour of the `fileset` argument.

### Result

Users are encouraged to switch to explicit recursive globs, which will allow us to remove the inconsistent behavior of the `fileset` argument. Fixes #4622.

@stuhood stuhood requested review from baroquebobcat , jsirois and kwlzn May 27, 2017

@kwlzn

kwlzn approved these changes May 27, 2017

Copy link
Member

kwlzn left a comment

lgtm!

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented May 27, 2017

I'll wait to land this till it has passed our internal sandbox.

Validates that at least one path was matched by a bundle, and (temporarily: see the
deprecation) symlinks matched directories to recursively include their contents.

This comment has been minimized.

@jsirois

jsirois May 27, 2017

Member

Kill the extra blank line.

lambda: True,
'1.5.0.dev0',
'default recursive inclusion of files in directory',
'''The bundle filespec `{spec}` corresponds to exactly one directory: if you'd like to '''

This comment has been minimized.

@jsirois

jsirois May 27, 2017

Member

Consider s/'''/"/g here and next 2 lines to more simply capture the you'd on this line.

stuhood added some commits May 27, 2017

@stuhood stuhood merged commit 79babbc into pantsbuild:master May 27, 2017

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/restore-temporary-bundle-recursion branch May 27, 2017

stuhood added a commit that referenced this pull request May 27, 2017

Temporarily restore recursive behaviour for bundle filesets (#4630)
### Problem

The `fileset` argument to `jvm_app` `bundles` is approximately like the `sources` argument on a target, with the exception that if a directory is matched, its contents are implicitly recursively included. Usually triggering recursive inclusion requires `**/*` wildcarding or `rglobs`.

Additionally, the v2 engine broke the recursive inclusion by excluding directories from the `FilesetWithSpec` that was created in the case of a `bundle`.

### Solution

Restore capturing directories for `FilesetWithSpec`s (but only in the context of bundles, for now), and deprecate the inconsistent default-recursive behaviour of the `fileset` argument.

This is attempt two for this patch. The primary differences are:
1. paths are created from shortest to longest, such that explicit directory symlinks are always created before their contents if both are matched
2. collisions between files is allowed rather than exploding (in case a parent symlinked directory contains a child matched file). This is also allowed in source globs... matching a file twice is fine.

### Result

Users are encouraged to switch to explicit recursive globs, which will allow us to remove the inconsistent behavior of the `fileset` argument. Fixes #4622.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment