Skip to content
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

Allow Directories to be un-nested #7699

Merged
merged 7 commits into from May 14, 2019

Conversation

Projects
None yet
3 participants
@illicitonion
Copy link
Contributor

commented May 10, 2019

Open questions:

  1. Should non-matching files cause errors, or cause those files to just be discarded? i.e. should stripping "prefix" from "nope/no" return an empty directory, or an error?
  2. Is the answer different if some files do match?

Alternatives to consider:

  1. Exposing more implementation details of Snapshot up to the Python side and allowing this manipulation to happen client-side.
  2. Having Target snapshots be source-root relative (and keep track of that source-root) rather than build-root relative. I suspect that would just lead to us needing the inverse API (nesting), which we'll probably still need at some point. It would save round-tripping through the engine (good), but would make it more likely that people will end up handing the engine unknown snapshots (bad).

Relates to #7697

@Eric-Arellano
Copy link
Contributor

left a comment

Woohoo, thank you! Although worth Stu's review for Rust.

Daniel and I discussed via Slack that I will follow up to use this new mechanism to create a Rule @rule(PrefixStrippedDirectory, [DirectoryDigest, Prefix]) that implements source root normalization. He suggested teaching TargetAdaptor to do this, e.g. source_root_relative_snapshot = yield Get(Digest, PrefixStrippedDigest(adaptor.snapshot.directory_digest, adaptor.source_root)).

For now, the rule will likely only support the default source roots outlined at https://www.pantsbuild.org/setup_repo.html#source-roots. Once it becomes necessary, we can add support for custom source roots.

Show resolved Hide resolved src/rust/engine/fs/src/snapshot.rs
Show resolved Hide resolved src/python/pants/engine/fs.py Outdated
Show resolved Hide resolved src/rust/engine/fs/src/snapshot.rs
Show resolved Hide resolved src/python/pants/engine/fs.py Outdated
@stuhood
Copy link
Member

left a comment

Thanks a lot! Two behaviour tweaks I think.

Show resolved Hide resolved src/python/pants/engine/fs.py Outdated
Show resolved Hide resolved src/rust/engine/fs/src/snapshot.rs Outdated
Show resolved Hide resolved src/rust/engine/fs/src/snapshot.rs Outdated
Show resolved Hide resolved src/rust/engine/fs/src/snapshot.rs Outdated

# Strip empty prefix:
zero_prefix_stripped_digest = assert_single_element(self.scheduler.product_request(
Digest,

This comment has been minimized.

Copy link
@stuhood

stuhood May 10, 2019

Member

Oh, so I missed the first time around: I think that in the case of both MergedDirectories and PrefixStrippedDirectory, usages parse pretty strangely:

digest = yield Get(Digest, PrefixStrippedDirectory(..))

PrefixStrippedDirectory "sounds" like the output of stripping the prefix (and MergedDirectories sounds like the output of merging directories). The upside of the current naming is that you don't need a name for the output... so if we can rename these to sound like inputs, that would work. If we still want to use these names, it feels like we'd want to add separate types to act as inputs.

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 13, 2019

Author Contributor

#6635 (comment) ;)

Renamed to DirectoryWithPrefixToStrip - will do MergedDirectories in a separate PR if you're happy with it :)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 13, 2019

Contributor

I was going to suggest using Request, which it sounds like that's out (which I think I agree with).

DirectoryWithPrefixToStrip is a great alternative name. It's quite clear that the prefix will be stripped, rather than the datatype already being stripped.

MergedDirectories would be great to call something like DirectoriesToMerge.

This comment has been minimized.

Copy link
@stuhood

stuhood May 13, 2019

Member

#6635 (comment) ;)

Renamed to DirectoryWithPrefixToStrip - will do MergedDirectories in a separate PR if you're happy with it :)

Yea, I think that maybe I thought that we were naming the outputs, rather than the inputs? Sorry for the miscommunication.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/strip-prefix branch from 9ca32a9 to 46231ce May 13, 2019

@@ -762,6 +762,7 @@ def new_scheduler(self,
type_directory_digest,
type_snapshot,
type_merge_snapshots_request,
type_prefix_stripped_directory,

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 13, 2019

Contributor

Does this need to be updated too?

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 13, 2019

Author Contributor

Updated :) Thanks!


# Strip empty prefix:
zero_prefix_stripped_digest = assert_single_element(self.scheduler.product_request(
Digest,

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 13, 2019

Contributor

I was going to suggest using Request, which it sounds like that's out (which I think I agree with).

DirectoryWithPrefixToStrip is a great alternative name. It's quite clear that the prefix will be stripped, rather than the datatype already being stripped.

MergedDirectories would be great to call something like DirectoriesToMerge.

illicitonion added some commits May 13, 2019

@Eric-Arellano Eric-Arellano merged commit 55f11c6 into pantsbuild:master May 14, 2019

1 check passed

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

@illicitonion illicitonion deleted the twitter:dwagnerhall/strip-prefix branch May 14, 2019

Eric-Arellano added a commit that referenced this pull request May 14, 2019

Add support for source roots in V2 `./pants test` (#7696)
### Problem
Running tests over our codebase, e.g. `./pants --no-v1 --v2 test tests/python/pants_test/util:strutil`, will fail because it won't be able to resolve the module `pants`.

```
$ ./pants --no-v1 --v2 test tests/python/pants_test/util:strutil
tests/python/pants_test/util:strutil stdout:
============================= test session starts ==============================
platform darwin -- Python 3.6.8, pytest-3.6.4, py-1.8.0, pluggy-0.7.1
rootdir: /Users/eric/DocsLocal/code/projects/pants/.pants.d/process-execution8hQxGD, inifile:
plugins: cov-2.4.0, timeout-1.2.1
collected 0 items / 1 errors

==================================== ERRORS ====================================
________ ERROR collecting tests/python/pants_test/util/test_strutil.py _________
ImportError while importing test module '/Users/eric/DocsLocal/code/projects/pants/.pants.d/process-execution8hQxGD/tests/python/pants_test/util/test_strutil.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
tests/python/pants_test/util/test_strutil.py:10: in <module>
    from pants.util.strutil import camelcase, ensure_binary, ensure_text, pluralize, strip_prefix
E   ModuleNotFoundError: No module named 'pants'
!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!!
=========================== 1 error in 0.14 seconds ============================


tests/python/pants_test/util:strutil                                            .....   FAILURE
Tests failed
```

The issue is that we are not properly handling source roots: https://www.pantsbuild.org/setup_repo.html#source-roots.

### Solution
* Link to the new `DirectoryWithPrefixToStrip` builtin Rust rule from #7699 to strip the sources' prefixes before getting passed to Pytest. 
   * This means that Pytest never needs to know what a source root is.
   * Uses the `SourceRootConfig` subsystem to dynamically determine that file's source root prefix.
* Create empty `__init__.py` files for each package.
   * For some reason, this was necessary to get the modules recognized, even on Python 3 despite its support of namespace packages.
   * We do not add any value to `__init__.py`, which is standard to use an empty `__init__.py` in Python. However, this contrasts with some V1 code like https://github.com/pantsbuild/pants/blob/55529b030102fca1b44c9b094fc90a49b4717909/src/python/pants/backend/python/subsystems/pex_build_util.py#L325-L330. To keep things simple for now, we don't add this line—everything is working without it. Down the road, this line may potentially be necessary to add but for now there is no reason.

### Result
`./pants --no-v1 --v2 test tests/python/pants_test/util:strutil` now passes, along with the new integration test.

This does mean that Pytest's `output` will be changed when using V2 in that it now only outputs the stripped paths. For now, this is okay because the V2 test runner will still print the whole path in the surrounding log and it is not worth the effort to re-inject the full path in Pytest's output.

Eric-Arellano added a commit that referenced this pull request May 15, 2019

Rename V2's `MergedDirectories` to `DirectoriesToMerge` (#7730)
`MergedDirectories` is a confusing name because it suggests that the directories are already merged (i.e. the output), whereas really this is the type of the directories that _will_ be merged (i.e. the input).

Refer to #7699 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.