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

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

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

[Zac Medico]

On 12/30/2017 01:45 PM, Mike Gilbert wrote:
> On Fri, Dec 29, 2017 at 7:19 PM, Zac Medico  wrote:
>> +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
>> +
>> +
> 
> This seems weirdly complex. I guess the goal was to reduce the
> sys.version_info check to once per import?

Yeah, and generally it's cleanest to create compatibility shims in cases
like this.

> The __doc__ trick is nifty, but I'm not sure I would ever want to use it 
> myself.

Yeah, the alternative of writing the docstring twice wasn't very appealing.

> Anyway, just my thoughts as an unpracticed python programmer. It looks
> like this code will get the job done.

-- 
Thanks,
Zac

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

[Mike Gilbert]

On Fri, Dec 29, 2017 at 7:19 PM, Zac Medico  wrote:
> +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
> +
> +

This seems weirdly complex. I guess the goal was to reduce the
sys.version_info check to once per import?

The __doc__ trick is nifty, but I'm not sure I would ever want to use it myself.

Anyway, just my thoughts as an unpracticed python programmer. It looks
like this code will get the job done.

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

[Zac Medico]

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):
+   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.