Skip to content

Commit

Permalink
Review feedback, and remove dead code from pantsbuild#4027.
Browse files Browse the repository at this point in the history
  • Loading branch information
@stuhood
stuhood committed Jan 10, 2019
1 parent 5136195 commit 8b13a05
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 77 deletions.
7 changes: 3 additions & 4 deletions src/rust/engine/fs/src/glob_matching.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ pub trait GlobMatching<E: Display + Send + Sync + 'static>: VFS<E> {
///
/// TODO: Should handle symlink loops (which would exhibit as an infinite loop in expand).
///
fn canonicalize(&self, symbolic_path: PathBuf, link: &Link) -> BoxFuture<Option<PathStat>, E> {
fn canonicalize(&self, symbolic_path: PathBuf, link: Link) -> BoxFuture<Option<PathStat>, E> {
GlobMatchingImplementation::canonicalize(self, symbolic_path, link)
}

Expand Down Expand Up @@ -121,7 +121,7 @@ trait GlobMatchingImplementation<E: Display + Send + Sync + 'static>: VFS<E> {
future::ok(None).to_boxed()
} else {
match stat {
Stat::Link(l) => context.canonicalize(stat_symbolic_path, l),
Stat::Link(l) => context.canonicalize(stat_symbolic_path, l.clone()),
Stat::Dir(d) => future::ok(Some(PathStat::dir(
stat_symbolic_path.to_owned(),
d.clone(),
Expand Down Expand Up @@ -399,8 +399,7 @@ trait GlobMatchingImplementation<E: Display + Send + Sync + 'static>: VFS<E> {
}
}

fn canonicalize(&self, symbolic_path: PathBuf, link: &Link) -> BoxFuture<Option<PathStat>, E> {
let link = link.clone();
fn canonicalize(&self, symbolic_path: PathBuf, link: Link) -> BoxFuture<Option<PathStat>, E> {
// Read the link, which may result in PathGlob(s) that match 0 or 1 Path.
let context = self.clone();
self
Expand Down
2 changes: 1 addition & 1 deletion src/rust/engine/fs/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -763,7 +763,7 @@ impl PathStatGetter<io::Error> for Arc<PosixFS> {
.and_then(move |maybe_stat| {
match maybe_stat {
// Note: This will drop PathStats for symlinks which don't point anywhere.
Some(Stat::Link(link)) => fs.canonicalize(link.0.clone(), &link),
Some(Stat::Link(link)) => fs.canonicalize(link.0.clone(), link),
Some(Stat::Dir(dir)) => {
future::ok(Some(PathStat::dir(dir.0.clone(), dir))).to_boxed()
}
Expand Down
94 changes: 22 additions & 72 deletions tests/python/pants_test/engine/test_fs.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,17 @@
import os
import tarfile
import unittest
from builtins import object, open, str
from builtins import open, str
from contextlib import contextmanager

from future.utils import PY2, text_type

from pants.base.project_tree import Dir, Link
from pants.engine.fs import (EMPTY_DIRECTORY_DIGEST, Digest, DirectoryToMaterialize, FilesContent,
MergedDirectories, PathGlobs, PathGlobsAndRoot, Snapshot, UrlToFetch,
create_fs_rules)
from pants.engine.scheduler import ExecutionError
from pants.util.contextutil import http_server, temporary_dir
from pants.util.dirutil import relative_symlink
from pants.util.meta import AbstractClass
from pants_test.engine.scheduler_test_base import SchedulerTestBase
from pants_test.test_base import TestBase
Expand All @@ -30,14 +30,6 @@
from http.server import BaseHTTPRequestHandler


class DirectoryListing(object):
"""TODO: See #4027."""


class ReadLink(object):
"""TODO: See #4027."""


class FSTest(TestBase, SchedulerTestBase, AbstractClass):

_original_src = os.path.join(os.path.dirname(__file__), 'examples/fs_test/fs_test.tar')
Expand All @@ -57,16 +49,19 @@ def specs(filespecs):
else:
return PathGlobs(include=filespecs)

def assert_walk_dirs(self, filespecs_or_globs, paths, ignore_patterns=None):
self.assert_walk_snapshot('dirs', filespecs_or_globs, paths, ignore_patterns=ignore_patterns)
def assert_walk_dirs(self, filespecs_or_globs, paths, **kwargs):
self.assert_walk_snapshot('dirs', filespecs_or_globs, paths, **kwargs)

def assert_walk_files(self, filespecs_or_globs, paths, ignore_patterns=None):
self.assert_walk_snapshot('files', filespecs_or_globs, paths, ignore_patterns=ignore_patterns)
def assert_walk_files(self, filespecs_or_globs, paths, **kwargs):
self.assert_walk_snapshot('files', filespecs_or_globs, paths, **kwargs)

def assert_walk_snapshot(self, field, filespecs_or_globs, paths, ignore_patterns=None):
def assert_walk_snapshot(self, field, filespecs_or_globs, paths, ignore_patterns=None, prepare=None):
with self.mk_project_tree(ignore_patterns=ignore_patterns) as project_tree:
scheduler = self.mk_scheduler(rules=create_fs_rules(), project_tree=project_tree)
if prepare:
prepare(project_tree)
result = self.execute(scheduler, Snapshot, self.specs(filespecs_or_globs))[0]
print('>>> got Snapshot {} containing {} and {}'.format(result, result.dirs, result.files))
self.assertEqual(sorted([p.path for p in getattr(result, field)]), sorted(paths))

def assert_content(self, filespecs_or_globs, expected_content):
Expand All @@ -85,17 +80,6 @@ def assert_digest(self, filespecs_or_globs, expected_files):
self.assertEqual(set(expected_files), {f.path for f in result.files})
self.assertTrue(result.directory_digest.fingerprint is not None)

def assert_fsnodes(self, filespecs_or_globs, subject_product_pairs):
with self.mk_project_tree() as project_tree:
scheduler = self.mk_scheduler(rules=create_fs_rules(), project_tree=project_tree)
request = self.execute_request(scheduler, Snapshot, self.specs(filespecs_or_globs))

# Validate that FilesystemNodes for exactly the given subjects are reachable under this
# request.
fs_nodes = [n for n, _ in scheduler.product_graph.walk(roots=request.roots)
if type(n) is "TODO: need a new way to filter for FS intrinsics"]
self.assertEqual({(n.subject, n.product) for n in fs_nodes}, set(subject_product_pairs))

def test_walk_literal(self):
self.assert_walk_files(['4.txt'], ['4.txt'])
self.assert_walk_files(['a/b/1.txt', 'a/b/2'], ['a/b/1.txt', 'a/b/2'])
Expand Down Expand Up @@ -141,6 +125,18 @@ def test_walk_single_star(self):
def test_walk_parent_link(self):
self.assert_walk_files(['c.ln/../3.txt'], ['c.ln/../3.txt'])

def test_walk_escaping_symlink(self):
link = 'subdir/escaping'
dest = '../../this-is-probably-nonexistent'
def prepare(project_tree):
link_path = os.path.join(project_tree.build_root, link)
dest_path = os.path.join(project_tree.build_root, dest)
relative_symlink(dest_path, link_path)

exc_reg = '.*While expanding link.*{}.*may not traverse outside of the buildroot.*{}.*'.format(link, dest)
with self.assertRaisesRegexp(Exception, exc_reg):
self.assert_walk_files([link], [], prepare=prepare)

def test_walk_recursive_all(self):
self.assert_walk_files(['**'], ['4.txt',
'a/3.txt',
Expand Down Expand Up @@ -231,52 +227,6 @@ def test_files_content_symlink(self):
def test_files_digest_literal(self):
self.assert_digest(['a/3.txt', '4.txt'], ['a/3.txt', '4.txt'])

@unittest.skip('Skipped to expedite landing #3821; see: #4027.')
def test_nodes_file(self):
self.assert_fsnodes(['4.txt'], [
(Dir(''), DirectoryListing),
])

@unittest.skip('Skipped to expedite landing #3821; see: #4027.')
def test_nodes_symlink_file(self):
self.assert_fsnodes(['c.ln/2'], [
(Dir(''), DirectoryListing),
(Link('c.ln'), ReadLink),
(Dir('a'), DirectoryListing),
(Dir('a/b'), DirectoryListing),
])
self.assert_fsnodes(['d.ln/b/1.txt'], [
(Dir(''), DirectoryListing),
(Link('d.ln'), ReadLink),
(Dir('a'), DirectoryListing),
(Dir('a/b'), DirectoryListing),
])

@unittest.skip('Skipped to expedite landing #3821; see: #4027.')
def test_nodes_symlink_globbed_dir(self):
self.assert_fsnodes(['*/2'], [
# Scandir for the root.
(Dir(''), DirectoryListing),
# Read links to determine whether they're actually directories.
(Link('c.ln'), ReadLink),
(Link('d.ln'), ReadLink),
# Scan second level destinations: `a/b` is matched via `c.ln`.
(Dir('a'), DirectoryListing),
(Dir('a/b'), DirectoryListing),
])

@unittest.skip('Skipped to expedite landing #3821; see: #4027.')
def test_nodes_symlink_globbed_file(self):
self.assert_fsnodes(['d.ln/b/*.txt'], [
# NB: Needs to scandir every Dir on the way down to track whether
# it is traversing a symlink.
(Dir(''), DirectoryListing),
# Traverse one symlink.
(Link('d.ln'), ReadLink),
(Dir('a'), DirectoryListing),
(Dir('a/b'), DirectoryListing),
])

def test_snapshot_from_outside_buildroot(self):
with temporary_dir() as temp_dir:
with open(os.path.join(temp_dir, "roland"), "w") as f:
Expand Down

0 comments on commit 8b13a05

Please sign in to comment.