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