Re: [gentoo-portage-dev] [PATCH v2] bin/doins.py: implement install -p option (bug 642632)

[Alec Warner]

On Fri, Dec 29, 2017 at 7:19 PM, Zac Medico <zmedico@g.o> wrote:

> Bug: https://bugs.gentoo.org/642632
> ---
> [PATCH v2] fix to copy atime, and split out _set_timestamps function
>
>  bin/doins.py                        | 28 +++++++++++++++++++++++++---
>  pym/portage/tests/bin/test_doins.py |  6 ++++--
>  2 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/bin/doins.py b/bin/doins.py
> index 92e450979..0d03d8fb2 100644
> --- a/bin/doins.py
> +++ b/bin/doins.py
> @@ -107,6 +107,7 @@ def _parse_install_options(
>         parser.add_argument('-g', '--group', default=-1, type=_parse_group)
>         parser.add_argument('-o', '--owner', default=-1, type=_parse_user)
>         parser.add_argument('-m', '--mode', default=0o755,
> type=_parse_mode)
> +       parser.add_argument('-p', '--preserve-timestamps',
> action='store_true')
>         split_options = shlex.split(options)
>         namespace, remaining = parser.parse_known_args(split_options)
>         # Because parsing '--mode' option is partially supported. If
> unknown
> @@ -139,6 +140,24 @@ def _set_attributes(options, path):
>                 os.chmod(path, options.mode)
>
>
> +def _set_timestamps(source_stat, dest):
> +       """Apply timestamps from source_stat to dest.
> +
> +       Args:
> +               source_stat: stat result for the source file.
> +               dest: path to the dest file.
> +       """
> +       os.utime(dest, (source_stat.st_atime, source_stat.st_mtime))
> +
> +
> +if sys.version_info >= (3, 3):
> +       def _set_timestamps_ns(source_stat, dest):
> +               os.utime(dest, ns=(source_stat.st_atime_ns,
> source_stat.st_mtime_ns))
> +
> +       _set_timestamps_ns.__doc__ = _set_timestamps.__doc__
> +       _set_timestamps = _set_timestamps_ns
> +
> +
>  class _InsInProcessInstallRunner(object):
>         """Implements `install` command behavior running in a process."""
>
> @@ -168,7 +187,8 @@ class _InsInProcessInstallRunner(object):
>                         True on success, otherwise False.
>                 """
>                 dest = os.path.join(dest_dir, os.path.basename(source))
> -               if not self._is_install_allowed(source, dest):
>

I'm going to be api picky here (so forgive me).

Previously is_install_allowed seemed to take great care with which stat was
used. But now callers have to just know to use os.stat (as opposed to
lstat, or other stats)?

Should we update the is_install_allowed comment (perhaps to say that
source_stat prefers os.stat?)

Part of me just wishes the stat was cached at a different layer (so that
code like this wasn't required just to save the syscall ;(

-A

+               sstat = os.stat(source)
> +               if not self._is_install_allowed(source, sstat, dest):
>                         return False
>
>                 # To emulate the `install` command, remove the dest file in
> @@ -187,6 +207,8 @@ class _InsInProcessInstallRunner(object):
>                                 movefile._copyxattr(
>                                         source, dest,
>                                         exclude=self._xattr_exclude)
> +                       if self._parsed_options.preserve_timestamps:
> +                               _set_timestamps(sstat, dest)
>                 except Exception:
>                         logging.exception(
>                                 'Failed to copy file: '
> @@ -195,13 +217,14 @@ class _InsInProcessInstallRunner(object):
>                         return False
>                 return True
>
> -       def _is_install_allowed(self, source, dest):
> +       def _is_install_allowed(self, source, source_stat, dest):
>                 """Returns if installing source into dest should work.
>
>                 This is to keep compatibility with the `install` command.
>
>                 Args:
>                         source: path to the source file.
> +                       source_stat: stat result for the source file.
>                         dest: path to the dest file.
>
>                 Returns:
> @@ -210,7 +233,6 @@ class _InsInProcessInstallRunner(object):
>                 # To match `install` command, use stat() for source, while
>                 # lstat() for dest. Raise an exception if stat(source)
> fails,
>                 # intentionally.
> -               source_stat = os.stat(source)
>                 try:
>                         dest_lstat = os.lstat(dest)
>                 except OSError as e:
> diff --git a/pym/portage/tests/bin/test_doins.py
> b/pym/portage/tests/bin/test_doins.py
> index 14d7adfa6..9b6c55d85 100644
> --- a/pym/portage/tests/bin/test_doins.py
> +++ b/pym/portage/tests/bin/test_doins.py
> @@ -38,13 +38,15 @@ class DoIns(setup_env.BinTestCase):
>                 self.init()
>                 try:
>                         env = setup_env.env
> -                       env['INSOPTIONS'] = '-m0644'
> +                       env['INSOPTIONS'] = '-pm0644'
>                         with open(os.path.join(env['S'], 'test'), 'w'):
>                                 pass
>                         doins('test')
>                         st = os.lstat(env['D'] + '/test')
>                         if stat.S_IMODE(st.st_mode) != 0o644:
>                                 raise tests.TestCase.failureException
> +                       if os.stat(os.path.join(env['S'],
> 'test')).st_mtime != st.st_mtime:
> +                               raise tests.TestCase.failureException
>                 finally:
>                         self.cleanup()
>
> @@ -145,7 +147,7 @@ class DoIns(setup_env.BinTestCase):
>                         env = setup_env.env
>                         # Use an option which doins.py does not know.
>                         # Then, fallback to `install` command is expected.
> -                       env['INSOPTIONS'] = '-p'
> +                       env['INSOPTIONS'] = '-b'
>                         with open(os.path.join(env['S'], 'test'), 'w'):
>                                 pass
>                         doins('test')
> --
> 2.13.6
>
>
>