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

Specify unicode vs bytes for Path and FileContent types #6303

Merged
merged 12 commits into from Aug 7, 2018
3 changes: 2 additions & 1 deletion src/python/pants/backend/graph_info/tasks/cloc.py
Expand Up @@ -5,6 +5,7 @@
from __future__ import absolute_import, division, print_function, unicode_literals

import os
from builtins import open

from future.utils import text_type

Expand Down Expand Up @@ -47,7 +48,7 @@ def console_output(self, targets):
with open(list_file, 'w') as list_file_out:
for input_file in sorted(input_files):
list_file_out.write(input_file)
list_file_out.write(b'\n')
list_file_out.write('\n')
list_file_snapshot = self.context._scheduler.capture_snapshots((
PathGlobsAndRoot(
PathGlobs(('input_files_list',)),
Expand Down
6 changes: 3 additions & 3 deletions src/python/pants/engine/fs.py
Expand Up @@ -4,15 +4,15 @@

from __future__ import absolute_import, division, print_function, unicode_literals

from future.utils import text_type
from future.utils import binary_type, text_type

from pants.base.project_tree import Dir, File
from pants.engine.rules import RootRule
from pants.option.global_options import GlobMatchErrorBehavior
from pants.util.objects import Collection, datatype


class FileContent(datatype(['path', 'content'])):
class FileContent(datatype([('path', text_type), ('content', binary_type)])):
"""The content of a file."""

def __repr__(self):
Expand All @@ -22,7 +22,7 @@ def __str__(self):
return repr(self)


class Path(datatype(['path', 'stat'])):
class Path(datatype([('path', text_type), 'stat'])):
"""A filesystem path, holding both its symbolic path name, and underlying canonical Stat.

Both values are relative to the ProjectTree's buildroot.
Expand Down
56 changes: 28 additions & 28 deletions src/python/pants/engine/scheduler.py
Expand Up @@ -116,34 +116,34 @@ def __init__(
self._register_rules(rule_index)

self._scheduler = native.new_scheduler(
self._tasks,
self._root_subject_types,
project_tree.build_root,
work_dir,
project_tree.ignore_patterns,
execution_options,
DirectoryDigest,
Snapshot,
FileContent,
FilesContent,
Path,
Dir,
File,
Link,
FallibleExecuteProcessResult,
has_products_constraint,
constraint_for(Address),
constraint_for(Variants),
constraint_for(PathGlobs),
constraint_for(DirectoryDigest),
constraint_for(Snapshot),
constraint_for(FilesContent),
constraint_for(Dir),
constraint_for(File),
constraint_for(Link),
constraint_for(ExecuteProcessRequest),
constraint_for(FallibleExecuteProcessResult),
constraint_for(GeneratorType),
tasks=self._tasks,
root_subject_types=self._root_subject_types,
build_root=project_tree.build_root,
work_dir=work_dir,
ignore_patterns=project_tree.ignore_patterns,
execution_options=execution_options,
construct_directory_digest=DirectoryDigest,
construct_snapshot=Snapshot,
construct_file_content=FileContent,
construct_files_content=FilesContent,
construct_path_stat=Path,
construct_dir=Dir,
construct_file=File,
construct_link=Link,
construct_process_result=FallibleExecuteProcessResult,
constraint_has_products=has_products_constraint,
constraint_address=constraint_for(Address),
constraint_variants=constraint_for(Variants),
constraint_path_globs=constraint_for(PathGlobs),
constraint_directory_digest=constraint_for(DirectoryDigest),
constraint_snapshot=constraint_for(Snapshot),
constraint_files_content=constraint_for(FilesContent),
constraint_dir=constraint_for(Dir),
constraint_file=constraint_for(File),
constraint_link=constraint_for(Link),
constraint_process_request=constraint_for(ExecuteProcessRequest),
constraint_process_result=constraint_for(FallibleExecuteProcessResult),
constraint_generator=constraint_for(GeneratorType),
)

# If configured, visualize the rule graph before asserting that it is valid.
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/option/options_bootstrapper.py
Expand Up @@ -147,7 +147,7 @@ def register_global(*args, **kwargs):
def construct_and_set_bootstrap_options(self):
"""Populates the internal bootstrap_options cache."""
def filecontent_for(path):
with open(path, 'r') as fh:
with open(path, 'rb') as fh:
return FileContent(path, fh.read())

# N.B. This adaptor is meant to simulate how we would co-operatively invoke options bootstrap
Expand Down
3 changes: 2 additions & 1 deletion src/rust/engine/src/nodes.rs
Expand Up @@ -703,7 +703,8 @@ impl Snapshot {
}

fn store_path(item: &Path) -> Value {
externs::store_bytes(item.as_os_str().as_bytes())
// TODO(illicitonion): see https://github.com/pantsbuild/pants/issues/6302
externs::store_utf8(&item.to_string_lossy())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you add this to externs.rs just below store_utf8:

///
/// Store a buffer of utf8 bytes to pass to Python. This will end up as a Python `unicode`.
///
#[cfg(unix)]
pub fn store_utf8_osstr(utf8: &OsStr) -> Value {
  use std::os::unix::ffi::OsStrExt;
  let bytes = utf8.as_bytes();
  with_externs(|e| (e.store_utf8)(e.context, bytes.as_ptr(), bytes.len() as u64).into())
}

and here instead call externs::store_utf8(item.as_os_str());

you should be able to close #6302 :)

}

fn store_dir(core: &Arc<Core>, item: &Dir) -> Value {
Expand Down
15 changes: 8 additions & 7 deletions tests/python/pants_test/engine/test_build_files.py
Expand Up @@ -7,8 +7,6 @@
import os
import unittest

from future.utils import text_type

from pants.base.project_tree import Dir, File
from pants.base.specs import SiblingAddresses, SingleAddress, Specs
from pants.build_graph.address import Address
Expand All @@ -34,8 +32,8 @@ def test_empty(self):
"""Test that parsing an empty BUILD file results in an empty AddressFamily."""
address_mapper = AddressMapper(JsonParser(TestTable()))
af = run_rule(parse_address_family, address_mapper, Dir('/dev/null'), {
(Snapshot, PathGlobs): lambda _: Snapshot(DirectoryDigest(text_type("abc"), 10), (File('/dev/null/BUILD'),)),
(FilesContent, DirectoryDigest): lambda _: FilesContent([FileContent('/dev/null/BUILD', '')]),
(Snapshot, PathGlobs): lambda _: Snapshot(DirectoryDigest('abc', 10), (File('/dev/null/BUILD'),)),
(FilesContent, DirectoryDigest): lambda _: FilesContent([FileContent('/dev/null/BUILD', b'')]),
})
self.assertEqual(len(af.objects_by_name), 0)

Expand All @@ -45,7 +43,8 @@ def test_duplicated(self):
"""Test that matching the same Spec twice succeeds."""
address = SingleAddress('a', 'a')
address_mapper = AddressMapper(JsonParser(TestTable()))
snapshot = Snapshot(DirectoryDigest(text_type('xx'), 2), (Path('a/BUILD', File('a/BUILD')),))
snapshot = Snapshot(DirectoryDigest('xx', 2),
(Path('a/BUILD', File('a/BUILD')),))
address_family = AddressFamily('a', {'a': ('a/BUILD', 'this is an object!')})

bfas = run_rule(addresses_from_address_families, address_mapper, Specs([address, address]), {
Expand All @@ -60,7 +59,8 @@ def test_tag_filter(self):
"""Test that targets are filtered based on `tags`."""
spec = SiblingAddresses('root')
address_mapper = AddressMapper(JsonParser(TestTable()))
snapshot = Snapshot(DirectoryDigest(text_type('xx'), 2), (Path('root/BUILD', File('root/BUILD')),))
snapshot = Snapshot(DirectoryDigest('xx', 2),
(Path('root/BUILD', File('root/BUILD')),))
address_family = AddressFamily('root',
{'a': ('root/BUILD', TargetAdaptor()),
'b': ('root/BUILD', TargetAdaptor(tags={'integration'})),
Expand All @@ -81,7 +81,8 @@ def test_exclude_pattern(self):
"""Test that targets are filtered based on exclude patterns."""
spec = SiblingAddresses('root')
address_mapper = AddressMapper(JsonParser(TestTable()))
snapshot = Snapshot(DirectoryDigest(text_type('xx'), 2), (Path('root/BUILD', File('root/BUILD')),))
snapshot = Snapshot(DirectoryDigest('xx', 2),
(Path('root/BUILD', File('root/BUILD')),))
address_family = AddressFamily('root',
{'exclude_me': ('root/BUILD', TargetAdaptor()),
'not_me': ('root/BUILD', TargetAdaptor()),
Expand Down
2 changes: 1 addition & 1 deletion tests/python/pants_test/engine/test_isolated_process.py
Expand Up @@ -320,7 +320,7 @@ def test_write_file(self):

self.assertEqual(
files_content_result.dependencies,
(FileContent("roland", "European Burmese"),)
(FileContent("roland", b"European Burmese"),)
)

def test_exercise_python_side_of_timeout_implementation(self):
Expand Down
2 changes: 1 addition & 1 deletion tests/python/pants_test/option/test_options.py
Expand Up @@ -1348,7 +1348,7 @@ class OptionsTestStringPayloads(OptionsTest):
"""Runs the same tests as OptionsTest, but backed with `Config.loads` vs `Config.load`."""

def _create_config_from_strings(self, config):
with io.StringIO('') as fp:
with io.BytesIO('') as fp:
self._write_config_to_file(fp, config)
fp.seek(0)
payload = fp.read()
Expand Down