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

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

src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py

@@ -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

src/python/pants/java/nailgun_executor.py

@@ -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),

src/python/pants/option/options_bootstrapper.py

@@ -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:

src/python/pants/pantsd/process_manager.py

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

src/python/pants/pantsd/watchman.py

@@ -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',

src/python/pants/releases/reversion.py

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

src/python/pants/util/BUILD

@@ -56,6 +56,7 @@ python_library(
   dependencies = [
     ':strutil',
     '3rdparty/python:future',
+    'src/python/pants/base:deprecated',
   ],
 )
 

src/python/pants/util/dirutil.py

@@ -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()

tests/python/pants_test/backend/jvm/subsystems/test_incomplete_custom_scala.py

@@ -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):

tests/python/pants_test/backend/jvm/tasks/test_bundle_create.py

@@ -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',

tests/python/pants_test/backend/jvm/tasks/test_consolidate_classpath.py

@@ -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',

tests/python/pants_test/backend/jvm/tasks/test_junit_run.py

@@ -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)]))
 

tests/python/pants_test/base/test_exception_sink_integration.py

@@ -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)

tests/python/pants_test/binaries/test_binary_util.py

@@ -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)):

tests/python/pants_test/build_graph/test_subproject_integration.py

@@ -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))
 

tests/python/pants_test/engine/legacy/test_address_mapper.py

@@ -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()

tests/python/pants_test/engine/legacy/test_console_rule_integration.py

@@ -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")')

tests/python/pants_test/jvm/jvm_task_test_base.py

@@ -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)])
 

tests/python/pants_test/pantsd/test_pantsd_integration.py

@@ -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::'])

tests/python/pants_test/pantsd/test_process_manager.py

@@ -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):

tests/python/pants_test/pantsd/test_watchman.py

@@ -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'],

tests/python/pants_test/test_base.py

@@ -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]