Allow Directories to be un-nested #7699

illicitonion · 2019-05-10T13:08:45Z

Open questions:

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?
Is the answer different if some files do match?

Alternatives to consider:

Exposing more implementation details of Snapshot up to the Python side and allowing this manipulation to happen client-side.
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

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.

src/rust/engine/fs/src/snapshot.rs

src/python/pants/engine/fs.py

src/rust/engine/fs/src/snapshot.rs

src/python/pants/engine/fs.py

stuhood

Thanks a lot! Two behaviour tweaks I think.

src/python/pants/engine/fs.py

src/rust/engine/fs/src/snapshot.rs

stuhood · 2019-05-10T23:09:02Z

tests/python/pants_test/engine/test_fs.py

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


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.

#6635 (comment) ;)

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

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.

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

Eric-Arellano · 2019-05-13T14:14:31Z

src/python/pants/engine/native.py

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


Does this need to be updated too?

Updated :) Thanks!

Eric-Arellano · 2019-05-13T14:17:56Z

tests/python/pants_test/engine/test_fs.py

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


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.

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

`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).

illicitonion requested review from stuhood, blorente and Eric-Arellano May 10, 2019 13:08

Eric-Arellano reviewed May 10, 2019

View reviewed changes

src/rust/engine/fs/src/snapshot.rs Show resolved Hide resolved

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

Eric-Arellano approved these changes May 10, 2019

View reviewed changes

src/rust/engine/fs/src/snapshot.rs Show resolved Hide resolved

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

stuhood approved these changes May 10, 2019

View reviewed changes

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 Show resolved Hide resolved

stuhood reviewed May 10, 2019

View reviewed changes

illicitonion added 2 commits May 13, 2019 15:07

Allow Directories to be un-nested

f144b5e

Error if not all have prefix (unless empty)

46231ce

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

Rename PrefixStrippedDirectory to DirectoryWithPrefixToStrip

060175f

Eric-Arellano approved these changes May 13, 2019

View reviewed changes

More renames

e53a1a9

Eric-Arellano mentioned this pull request May 13, 2019

Add support for source roots in V2 ./pants test #7696

Merged

illicitonion added 2 commits May 13, 2019 16:28

Fix imports

28d2e7b

Support py2

dc5296e

stuhood approved these changes May 13, 2019

View reviewed changes

Add missing newline

76c0fa9

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

illicitonion deleted the dwagnerhall/strip-prefix branch May 14, 2019 09:07

Eric-Arellano mentioned this pull request May 14, 2019

Rename V2's MergedDirectories to DirectoriesToMerge #7730

Merged

stuhood mentioned this pull request May 16, 2019

Consider how to relativize target Snapshots to source roots #7697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Directories to be un-nested #7699

Allow Directories to be un-nested #7699

illicitonion commented May 10, 2019

Eric-Arellano left a comment •

edited

Loading

stuhood left a comment

stuhood May 10, 2019 •

edited

Loading

illicitonion May 13, 2019

Eric-Arellano May 13, 2019

stuhood May 13, 2019

Eric-Arellano May 13, 2019

illicitonion May 13, 2019

Eric-Arellano May 13, 2019

Allow Directories to be un-nested #7699

Allow Directories to be un-nested #7699

Conversation

illicitonion commented May 10, 2019

Open questions:

Alternatives to consider:

Eric-Arellano left a comment • edited Loading

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

stuhood May 10, 2019 • edited Loading

Choose a reason for hiding this comment

illicitonion May 13, 2019

Choose a reason for hiding this comment

Eric-Arellano May 13, 2019

Choose a reason for hiding this comment

stuhood May 13, 2019

Choose a reason for hiding this comment

Eric-Arellano May 13, 2019

Choose a reason for hiding this comment

illicitonion May 13, 2019

Choose a reason for hiding this comment

Eric-Arellano May 13, 2019

Choose a reason for hiding this comment

Eric-Arellano left a comment •

edited

Loading

stuhood May 10, 2019 •

edited

Loading