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

[Zac Medico]

On 12/31/2017 11:55 AM, Alec Warner wrote:
> On Fri, Dec 29, 2017 at 7:19 PM, Zac Medico <zmedico@g.o
> <mailto: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?)

Updated the is_install_allowed docstring in v3.

> 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 ;(

The _doins function has an earlier stat call here:

    if os.path.islink(source):

We can implement more caching at some point, but it's beyond the scope
of this patch.
-- 
Thanks,
Zac