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

Merged
merged 3 commits into from May 26, 2017

Conversation

Projects
None yet
2 participants
@stuhood
Copy link
Member

stuhood commented May 26, 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.

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 added this to the 1.3.0 milestone May 26, 2017

@stuhood stuhood requested review from baroquebobcat , benjyw and kwlzn May 26, 2017

@kwlzn

kwlzn approved these changes May 26, 2017

Copy link
Member

kwlzn left a comment

lgtm! two tiny nits.

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.

@kwlzn

kwlzn May 26, 2017

Member

nit: the triple quotes dont seem necessary here.

@@ -381,9 +382,13 @@ def hydrate_bundles(bundles_field, snapshot_list):
for bundle, filespecs, snapshot in zipped:
spec_path = bundles_field.address.spec_path
kwargs = bundle.kwargs()
# NB: We `include_dirs=True` because bundle filesets frequently specify directories in order
# a (deprecated) default inclusion of their recursive contents. See the related deprecation
# in `pants.backend.jvm.tasks.bundle_create`.

This comment has been minimized.

@kwlzn

kwlzn May 26, 2017

Member

...frequently specify directories in order a (deprecated) default inclusion of their... eh?

@stuhood stuhood force-pushed the twitter:stuhood/temporary-recursive-bundles branch from df2018e to aa3796c May 26, 2017

@stuhood stuhood merged commit 272e8e6 into pantsbuild:master May 26, 2017

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/temporary-recursive-bundles branch May 26, 2017

stuhood added a commit that referenced this pull request 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 added a commit to twitter/pants that referenced this pull request May 26, 2017

jsirois added a commit that referenced this pull request May 26, 2017

stuhood added a commit to twitter/pants that referenced this pull request May 27, 2017

Temporarily restore recursive behaviour for bundle filesets (pantsbui…
…ld#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 pantsbuild#4622.

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

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