Skip to content

Commit

Permalink
deprecate implicit usage of binary_mode=True and mode='wb' in dirutil…
Browse files Browse the repository at this point in the history
… methods (#7120)

### Problem

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

#7121 covers the eventual switchover to a default of `binary_mode=False` after the python 3 migration completes.
  • Loading branch information
cosmicexplorer authored Feb 6, 2019
1 parent 224c2a0 commit 84cf9a7
Show file tree
Hide file tree
Showing 23 changed files with 82 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/python/pants/java/nailgun_executor.py
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/option/options_bootstrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/pantsd/process_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/pantsd/watchman.py
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
4 changes: 2 additions & 2 deletions src/python/pants/releases/reversion.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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`.
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/util/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ python_library(
dependencies = [
':strutil',
'3rdparty/python:future',
'src/python/pants/base:deprecated',
],
)

Expand Down
43 changes: 37 additions & 6 deletions src/python/pants/util/dirutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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'
Expand All @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)]))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions tests/python/pants_test/binaries/test_binary_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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))

Expand Down
12 changes: 6 additions & 6 deletions tests/python/pants_test/engine/legacy/test_address_mapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")')
Expand Down
2 changes: 1 addition & 1 deletion tests/python/pants_test/jvm/jvm_task_test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)])

Expand Down
8 changes: 4 additions & 4 deletions tests/python/pants_test/pantsd/test_pantsd_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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::'])
Expand Down
2 changes: 1 addition & 1 deletion tests/python/pants_test/pantsd/test_process_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
3 changes: 2 additions & 1 deletion tests/python/pants_test/pantsd/test_watchman.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
Expand Down
2 changes: 1 addition & 1 deletion tests/python/pants_test/test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
Loading

0 comments on commit 84cf9a7

Please sign in to comment.