Skip to content
Permalink
Browse files

deprecate implicit usage of binary_mode=True and mode='wb' in dirutil…

… methods (pantsbuild#7120)

### Problem

*Resolves pantsbuild#6543. See also [the python 3 migration project](https://github.com/pantsbuild/pants/projects/10).*

There has been [a TODO](https://github.com/pantsbuild/pants/blob/6fcd7f7d0f8787910cfac01ec2895cdbd5cee66f/src/python/pants/util/dirutil.py#L109) pointing to pantsbuild#6543 to deprecate the `binary_mode` argument to `pants.util.dirutil.safe_file_dump()`, which wasn't canonicalized with a `deprecated_conditional`. This is because `binary_mode` doesn't quite make sense the way it does with file read methods `read_file()` and `maybe_read_file()`, because a file can be appended to as well as truncated (as opposed to reads).

Separately, defaulting `binary_mode=True` for file read methods means more explicit conversions to unicode in a python 3 world,

### Solution

- Deprecate the `binary_mode` argument to `safe_file_dump()`, as well as not explicitly specifying the `mode` argument.
  - `safe_file_dump()` now also defaults `payload=''`.
  - Also deprecate not specifying the `mode='wb'` argument in `safe_file_dump()`.
- Deprecate not explicitly specifying the `binary_mode` argument in `{maybe_,}read_file()` and `temporary_file()` so that it can be given a default of unicode when pants finishes [migrating to python 3](https://github.com/pantsbuild/pants/projects/10) -- see pantsbuild#7121.
- Update usages of `safe_file_dump()` across the repo.

### Result

Pants plugins will see a deprecation warning if they fail to explicitly specify the `binary_mode` for file read methods in preparation for switching the default to unicode for [the python 3 switchover](https://github.com/pantsbuild/pants/projects/10). Several ambiguities in the `safe_file_dump()` method are alleviated.

pantsbuild#7121 covers the eventual switchover to a default of `binary_mode=False` after the python 3 migration completes.
  • Loading branch information...
cosmicexplorer committed Feb 6, 2019
1 parent 224c2a0 commit 84cf9a75dbf68cf7126fe8372ab9b2f48720464d
@@ -60,9 +60,11 @@ def stdout_contents(wu):
return f.read().rstrip()


def dump_digest(output_dir, digest):
safe_file_dump('{}.digest'.format(output_dir),
'{}:{}'.format(digest.fingerprint, digest.serialized_bytes_length), mode='w')
def write_digest(output_dir, digest):
safe_file_dump(
'{}.digest'.format(output_dir),
mode='w',
payload='{}:{}'.format(digest.fingerprint, digest.serialized_bytes_length))


def load_digest(output_dir):
@@ -823,7 +825,7 @@ def _runtool_hermetic(self, main, tool_name, args, distribution, tgt=None, input
raise TaskError(res.stderr)

if output_dir:
dump_digest(output_dir, res.output_directory_digest)
write_digest(output_dir, res.output_directory_digest)
self.context._scheduler.materialize_directories((
DirectoryToMaterialize(
# NB the first element here is the root to materialize into, not the dir to snapshot
@@ -228,8 +228,8 @@ def ensure_connectable(self, nailgun):
def _spawn_nailgun_server(self, fingerprint, jvm_options, classpath, stdout, stderr, stdin):
"""Synchronously spawn a new nailgun server."""
# Truncate the nailguns stdout & stderr.
safe_file_dump(self._ng_stdout, b'')
safe_file_dump(self._ng_stderr, b'')
safe_file_dump(self._ng_stdout, b'', mode='wb')
safe_file_dump(self._ng_stderr, b'', mode='wb')

jvm_options = jvm_options + [self._PANTS_NG_BUILDROOT_ARG,
self._create_owner_arg(self._workdir),
@@ -103,7 +103,7 @@ def create(cls, env=None, args=None):
short_flags = set()

def filecontent_for(path):
return FileContent(ensure_text(path), read_file(path))
return FileContent(ensure_text(path), read_file(path, binary_mode=True))

def capture_the_flags(*args, **kwargs):
for arg in args:
@@ -191,7 +191,7 @@ def write_metadata_by_name(self, name, metadata_key, metadata_value):
"""
self._maybe_init_metadata_dir_by_name(name)
file_path = self._metadata_file_path(name, metadata_key)
safe_file_dump(file_path, metadata_value, binary_mode=False)
safe_file_dump(file_path, metadata_value, mode='w')

def await_metadata_by_name(self, name, metadata_key, timeout, caster=None):
"""Block up to a timeout for process metadata to arrive on disk.
@@ -82,7 +82,7 @@ def _normalize_watchman_path(self, watchman_path):
def _maybe_init_metadata(self):
safe_mkdir(self._watchman_work_dir)
# Initialize watchman with an empty, but valid statefile so it doesn't complain on startup.
safe_file_dump(self._state_file, b'{}')
safe_file_dump(self._state_file, b'{}', mode='wb')

def _construct_cmd(self, cmd_parts, state_file, sock_file, pid_file, log_file, log_level):
return [part for part in cmd_parts] + ['--no-save-state',
@@ -30,7 +30,7 @@ def replace_in_file(workspace, src_file_path, from_str, to_str):
return None

dst_file_path = src_file_path.replace(from_str, to_str)
safe_file_dump(os.path.join(workspace, dst_file_path), data.replace(from_bytes, to_bytes))
safe_file_dump(os.path.join(workspace, dst_file_path), data.replace(from_bytes, to_bytes), mode='wb')
if src_file_path != dst_file_path:
os.unlink(os.path.join(workspace, src_file_path))
return dst_file_path
@@ -88,7 +88,7 @@ def rewrite_record_file(workspace, src_record_file, mutated_file_tuples):
output_line = line
output_records.append(output_line)

safe_file_dump(file_name, '\r\n'.join(output_records) + '\r\n', binary_mode=False)
safe_file_dump(file_name, '\r\n'.join(output_records) + '\r\n', mode='w')


# The wheel METADATA file will contain a line like: `Version: 1.11.0.dev3+7951ec01`.
@@ -56,6 +56,7 @@ python_library(
dependencies = [
':strutil',
'3rdparty/python:future',
'src/python/pants/base:deprecated',
],
)

@@ -16,6 +16,7 @@
from collections import defaultdict
from contextlib import contextmanager

from pants.base.deprecated import deprecated_conditional
from pants.util.strutil import ensure_text


@@ -100,25 +101,37 @@ def safe_mkdir_for_all(paths):
created_dirs.add(dir_to_make)


def safe_file_dump(filename, payload, binary_mode=None, mode=None):
# TODO(#6742): payload should be Union[str, bytes] in type hint syntax, but from
# https://pythonhosted.org/an_example_pypi_project/sphinx.html#full-code-example it doesn't appear
# that is possible to represent in docstring type syntax.
def safe_file_dump(filename, payload='', binary_mode=None, mode=None):
"""Write a string to a file.
This method is "safe" to the extent that `safe_open` is "safe". See the explanation on the method
doc there.
TODO: The `binary_mode` flag should be deprecated and removed from existing callsites. Once
`binary_mode` is removed, mode can directly default to `wb`.
see https://github.com/pantsbuild/pants/issues/6543
When `payload` is an empty string (the default), this method can be used as a concise way to
create an empty file along with its containing directory (or truncate it if it already exists).
:param string filename: The filename of the file to write to.
:param string payload: The string to write to the file.
:param bool binary_mode: Write to file as bytes or unicode. Mutually exclusive with mode.
:param string mode: A mode argument for the python `open` builtin. Mutually exclusive with
binary_mode.
"""
deprecated_conditional(
lambda: binary_mode is not None,
removal_version='1.16.0.dev2',
entity_description='The binary_mode argument in safe_file_dump()',
hint_message='Use the mode argument instead!')
if binary_mode is not None and mode is not None:
raise AssertionError('Only one of `binary_mode` and `mode` may be specified.')

deprecated_conditional(
lambda: mode is None,
removal_version='1.16.0.dev2',
entity_description='Not specifying mode explicitly in safe_file_dump()',
hint_message="Function will default to unicode ('w') when pants migrates to python 3!")
if mode is None:
if binary_mode is False:
mode = 'w'
@@ -129,28 +142,46 @@ def safe_file_dump(filename, payload, binary_mode=None, mode=None):
f.write(payload)


def maybe_read_file(filename, binary_mode=True):
def maybe_read_file(filename, binary_mode=None):
"""Read and return the contents of a file in a single file.read().
:param string filename: The filename of the file to read.
:param bool binary_mode: Read from file as bytes or unicode.
:returns: The contents of the file, or opening the file fails for any reason
:rtype: string
"""
# TODO(#7121): Default binary_mode=False after the python 3 switchover!
deprecated_conditional(
lambda: binary_mode is None,
removal_version='1.16.0.dev2',
entity_description='Not specifying binary_mode explicitly in maybe_read_file()',
hint_message='Function will default to unicode when pants migrates to python 3!')
if binary_mode is None:
binary_mode = True

try:
return read_file(filename, binary_mode=binary_mode)
except IOError:
return None


def read_file(filename, binary_mode=True):
def read_file(filename, binary_mode=None):
"""Read and return the contents of a file in a single file.read().
:param string filename: The filename of the file to read.
:param bool binary_mode: Read from file as bytes or unicode.
:returns: The contents of the file.
:rtype: string
"""
# TODO(#7121): Default binary_mode=False after the python 3 switchover!
deprecated_conditional(
lambda: binary_mode is None,
removal_version='1.16.0.dev2',
entity_description='Not specifying binary_mode explicitly in read_file()',
hint_message='Function will default to unicode when pants migrates to python 3!')
if binary_mode is None:
binary_mode = True

mode = 'rb' if binary_mode else 'r'
with open(filename, mode) as f:
return f.read()
@@ -37,7 +37,7 @@ def tmp_custom_scala(self, path_suffix):
def tmp_scalastyle_config(self):
with temporary_dir(root_dir=get_buildroot()) as scalastyle_dir:
path = os.path.join(scalastyle_dir, 'config.xml')
safe_file_dump(path, '''<scalastyle/>''', binary_mode=False)
safe_file_dump(path, '''<scalastyle/>''', mode='w')
yield '--lint-scalastyle-config={}'.format(path)

def pants_run(self, options=None):
@@ -38,7 +38,7 @@ def add_consolidated_bundle(self, context, tgt, files_dict):
entry_path = safe_mkdtemp(dir=target_dir)
classpath_dir = safe_mkdtemp(dir=target_dir)
for rel_path, content in files_dict.items():
safe_file_dump(os.path.join(entry_path, rel_path), content, binary_mode=False)
safe_file_dump(os.path.join(entry_path, rel_path), content, mode='w')

# Create Jar to mimic consolidate classpath behavior.
jarpath = os.path.join(classpath_dir, 'output-0.jar')
@@ -71,12 +71,12 @@ def setUp(self):
JarDependency(org='org.gnu', name='gary', rev='4.0.0',
ext='tar.gz')])

safe_file_dump(os.path.join(self.build_root, 'resources/foo/file'), '// dummy content', binary_mode=False)
safe_file_dump(os.path.join(self.build_root, 'resources/foo/file'), '// dummy content', mode='w')
self.resources_target = self.make_target('//resources:foo-resources', Resources,
sources=['foo/file'])

# This is so that payload fingerprint can be computed.
safe_file_dump(os.path.join(self.build_root, 'foo/Foo.java'), '// dummy content', binary_mode=False)
safe_file_dump(os.path.join(self.build_root, 'foo/Foo.java'), '// dummy content', mode='w')
self.java_lib_target = self.make_target('//foo:foo-library', JavaLibrary, sources=['Foo.java'])

self.binary_target = self.make_target(spec='//foo:foo-binary',
@@ -48,12 +48,12 @@ def setUp(self):
JarDependency(org='org.gnu', name='gary', rev='4.0.0',
ext='tar.gz')])

safe_file_dump(os.path.join(self.build_root, 'resources/foo/file'), '// dummy content', binary_mode=False)
safe_file_dump(os.path.join(self.build_root, 'resources/foo/file'), '// dummy content', mode='w')
self.resources_target = self.make_target('//resources:foo-resources', Resources,
sources=['foo/file'])

# This is so that payload fingerprint can be computed.
safe_file_dump(os.path.join(self.build_root, 'foo/Foo.java'), '// dummy content', binary_mode=False)
safe_file_dump(os.path.join(self.build_root, 'foo/Foo.java'), '// dummy content', mode='w')
self.java_lib_target = self.make_target('//foo:foo-library', JavaLibrary, sources=['Foo.java'])

self.binary_target = self.make_target(spec='//foo:foo-binary',
@@ -218,7 +218,7 @@ def test_request_classes_by_source(self):

# Existing files (with and without the method name) should trigger.
srcfile = os.path.join(self.test_workdir, 'this.is.a.source.file.scala')
safe_file_dump(srcfile, 'content!', binary_mode=False)
safe_file_dump(srcfile, 'content!', mode='w')
self.assertTrue(JUnitRun.request_classes_by_source([srcfile]))
self.assertTrue(JUnitRun.request_classes_by_source(['{}#method'.format(srcfile)]))

@@ -198,7 +198,7 @@ def test_reset_interactive_output_stream(self):

with temporary_dir() as tmpdir:
some_file = os.path.join(tmpdir, 'some_file')
safe_file_dump(some_file, b'', binary_mode=True)
safe_file_dump(some_file, b'', mode='wb')
redirected_pants_run = self.run_pants([
"--lifecycle-stubs-new-interactive-stream-output-file={}".format(some_file),
] + lifecycle_stub_cmdline)
@@ -346,11 +346,11 @@ def test_select_argv(self):
"""Test invoking binary_util.py as a standalone script."""
with temporary_dir() as tmp_dir:
config_file_loc = os.path.join(tmp_dir, 'pants.ini')
safe_file_dump(config_file_loc, """\
safe_file_dump(config_file_loc, mode='w', payload="""\
[GLOBAL]
allow_external_binary_tool_downloads: True
pants_bootstrapdir: {}
""".format(tmp_dir), binary_mode=False)
""".format(tmp_dir))
expected_output_glob = os.path.join(
tmp_dir, 'bin', 'cmake', '*', '*', '3.9.5', 'cmake')
with environment_as(PANTS_CONFIG_FILES='[{!r}]'.format(config_file_loc)):
@@ -76,7 +76,7 @@
def harness():
try:
for name, content in BUILD_FILES.items():
safe_file_dump(name, dedent(content), binary_mode=False)
safe_file_dump(name, dedent(content), mode='w')
yield
finally:
safe_rmtree(SUBPROJ_SPEC)
@@ -102,7 +102,7 @@ def test_subproject_with_flag(self):
"""
with harness():
# Has dependencies below the subproject.
pants_args = ['--subproject-roots={}'.format(SUBPROJ_ROOT),
pants_args = ['--subproject-roots={}'.format(SUBPROJ_ROOT),
'dependencies', SUBPROJ_SPEC]
self.assert_success(self.run_pants(pants_args))

@@ -40,15 +40,15 @@ def create_build_files(self):
safe_mkdir(dir_b)
safe_mkdir(dir_a_subdir)

safe_file_dump(os.path.join(self.build_root, 'BUILD'), 'target(name="a")\ntarget(name="b")', binary_mode=False)
safe_file_dump(os.path.join(self.build_root, 'BUILD.other'), 'target(name="c")', binary_mode=False)
safe_file_dump(os.path.join(self.build_root, 'BUILD'), 'target(name="a")\ntarget(name="b")', mode='w')
safe_file_dump(os.path.join(self.build_root, 'BUILD.other'), 'target(name="c")', mode='w')

safe_file_dump(os.path.join(dir_a, 'BUILD'), 'target(name="a")\ntarget(name="b")', binary_mode=False)
safe_file_dump(os.path.join(dir_a, 'BUILD.other'), 'target(name="c")', binary_mode=False)
safe_file_dump(os.path.join(dir_a, 'BUILD'), 'target(name="a")\ntarget(name="b")', mode='w')
safe_file_dump(os.path.join(dir_a, 'BUILD.other'), 'target(name="c")', mode='w')

safe_file_dump(os.path.join(dir_b, 'BUILD'), 'target(name="a")', binary_mode=False)
safe_file_dump(os.path.join(dir_b, 'BUILD'), 'target(name="a")', mode='w')

safe_file_dump(os.path.join(dir_a_subdir, 'BUILD'), 'target(name="a")', binary_mode=False)
safe_file_dump(os.path.join(dir_a_subdir, 'BUILD'), 'target(name="a")', mode='w')

def test_is_valid_single_address(self):
self.create_build_files()
@@ -78,7 +78,7 @@ def test_v2_list_loop(self):
rel_tmpdir = fast_relpath(tmpdir, get_buildroot())

def dump(content):
safe_file_dump(os.path.join(tmpdir, 'BUILD'), content, mode="w")
safe_file_dump(os.path.join(tmpdir, 'BUILD'), content, mode='w')

# Dump an initial target before starting the loop.
dump('target(name="one")')
@@ -53,7 +53,7 @@ def add_to_runtime_classpath(self, context, tgt, files_dict):
safe_mkdir(target_dir)
classpath_dir = safe_mkdtemp(dir=target_dir)
for rel_path, content in files_dict.items():
safe_file_dump(os.path.join(classpath_dir, rel_path), content, binary_mode=False)
safe_file_dump(os.path.join(classpath_dir, rel_path), content, mode='w')
# Add to the classpath.
runtime_classpath.add_for_target(tgt, [('default', classpath_dir)])

@@ -358,17 +358,17 @@ def test_pantsd_invalidation_stale_sources(self):
pantsd_run(['help'])
checker.assert_started()

safe_file_dump(test_build_file, "python_library(sources=globs('some_non_existent_file.py'))", binary_mode=False)
safe_file_dump(test_build_file, "python_library(sources=globs('some_non_existent_file.py'))", mode='w')
result = pantsd_run(export_cmd)
checker.assert_running()
assertNotRegex(self, result.stdout_data, has_source_root_regex)

safe_file_dump(test_build_file, "python_library(sources=globs('*.py'))", binary_mode=False)
safe_file_dump(test_build_file, "python_library(sources=globs('*.py'))", mode='w')
result = pantsd_run(export_cmd)
checker.assert_running()
assertNotRegex(self, result.stdout_data, has_source_root_regex)

safe_file_dump(test_src_file, 'import this\n', binary_mode=False)
safe_file_dump(test_src_file, 'import this\n', mode='w')
result = pantsd_run(export_cmd)
checker.assert_running()
assertRegex(self, result.stdout_data, has_source_root_regex)
@@ -385,7 +385,7 @@ def test_pantsd_parse_exception_success(self):

try:
safe_mkdir(test_path, clean=True)
safe_file_dump(test_build_file, "{}()".format(invalid_symbol), binary_mode=False)
safe_file_dump(test_build_file, "{}()".format(invalid_symbol), mode='w')
for _ in range(3):
with self.pantsd_run_context(success=False) as (pantsd_run, checker, _, _):
result = pantsd_run(['list', 'testprojects::'])
@@ -149,7 +149,7 @@ def test_deadline_until(self):
def test_wait_for_file(self):
with temporary_dir() as td:
test_filename = os.path.join(td, 'test.out')
safe_file_dump(test_filename, 'test', binary_mode=False)
safe_file_dump(test_filename, 'test', mode='w')
self.pmm._wait_for_file(test_filename, timeout=.1)

def test_wait_for_file_timeout(self):
@@ -57,12 +57,13 @@ def test_resolve_watchman_path_provided_exception(self):
metadata_base_dir=self.subprocess_dir)

def test_maybe_init_metadata(self):
# TODO(#7106): is this the right path to patch?
with mock.patch('pants.pantsd.watchman.safe_mkdir', **self.PATCH_OPTS) as mock_mkdir, \
mock.patch('pants.pantsd.watchman.safe_file_dump', **self.PATCH_OPTS) as mock_file_dump:
self.watchman._maybe_init_metadata()

mock_mkdir.assert_called_once_with(self._watchman_dir)
mock_file_dump.assert_called_once_with(self._state_file, b'{}')
mock_file_dump.assert_called_once_with(self._state_file, b'{}', mode='wb')

def test_construct_cmd(self):
output = self.watchman._construct_cmd(['cmd', 'parts', 'etc'],
@@ -632,7 +632,7 @@ def make_snapshot(self, files):
"""
with temporary_dir() as temp_dir:
for file_name, content in files.items():
safe_file_dump(os.path.join(temp_dir, file_name), content)
safe_file_dump(os.path.join(temp_dir, file_name), content, mode='w')
return self.scheduler.capture_snapshots((
PathGlobsAndRoot(PathGlobs(('**',)), text_type(temp_dir)),
))[0]
Oops, something went wrong.

0 comments on commit 84cf9a7

Please sign in to comment.
You can’t perform that action at this time.