Re: [gentoo-portage-dev] [PATCH 2/2] sys-apps/portage: exclude /var/tmp/ccache from tmpfiles cleanup

[Zac Medico]

On 01/01/2018 05:00 PM, Mike Gilbert wrote:
> On Mon, Jan 1, 2018 at 6:23 PM, Zac Medico  wrote:
>> On 12/31/2017 07:22 AM, Mike Gilbert wrote:
>>> By default, systemd-tmpfiles removes files older than 30 days from /var/tmp.
>>> The default portage config sets CCACHE_DIR=/var/tmp/ccache.
>>>
>>> Bug: https://bugs.gentoo.org/490676#c14
>>> Package-Manager: Portage-2.3.19_p3, Repoman-2.3.6_p37
>>> ---
>>>  sys-apps/portage/files/portage-ccache.conf | 2 ++
>>>  sys-apps/portage/portage-2.3.19.ebuild | 4 +++-
>>>  sys-apps/portage/portage-.ebuild   | 4 +++-
>>>  3 files changed, 8 insertions(+), 2 deletions(-)
>>>  create mode 100644 sys-apps/portage/files/portage-ccache.conf
>>
>> Looks good. Maybe we should also call tmpfiles_process in pkg_postinst?
> 
> Calling tmpfiles_process is only relevant for tmpfiles.d snippets that
> create files/directories, which normally only happens on system
> startup.
> 
> This one prevents files/directories from being removed during the
> recurring "clean" job, which runs once every 24 hours on systemd.
> OpenRC systems have no such process.

Ok, thanks for the explanation. The patches look good. Please go ahead
and apply them.
-- 
Thanks,
Zac



signature.asc
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH 2/2] sys-apps/portage: exclude /var/tmp/ccache from tmpfiles cleanup

[Mike Gilbert]

On Mon, Jan 1, 2018 at 6:23 PM, Zac Medico  wrote:
> On 12/31/2017 07:22 AM, Mike Gilbert wrote:
>> By default, systemd-tmpfiles removes files older than 30 days from /var/tmp.
>> The default portage config sets CCACHE_DIR=/var/tmp/ccache.
>>
>> Bug: https://bugs.gentoo.org/490676#c14
>> Package-Manager: Portage-2.3.19_p3, Repoman-2.3.6_p37
>> ---
>>  sys-apps/portage/files/portage-ccache.conf | 2 ++
>>  sys-apps/portage/portage-2.3.19.ebuild | 4 +++-
>>  sys-apps/portage/portage-.ebuild   | 4 +++-
>>  3 files changed, 8 insertions(+), 2 deletions(-)
>>  create mode 100644 sys-apps/portage/files/portage-ccache.conf
>
> Looks good. Maybe we should also call tmpfiles_process in pkg_postinst?

Calling tmpfiles_process is only relevant for tmpfiles.d snippets that
create files/directories, which normally only happens on system
startup.

This one prevents files/directories from being removed during the
recurring "clean" job, which runs once every 24 hours on systemd.
OpenRC systems have no such process.

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

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

[Zac Medico]

Bug: https://bugs.gentoo.org/642632
---
[PATCH v3] update _is_install_allowed docstring to specify that source_stat
should be obtained with stat() rather than lstat()

 bin/doins.py| 34 +-
 pym/portage/tests/bin/test_doins.py |  6 --
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/bin/doins.py b/bin/doins.py
index 92e450979..9e6566097 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,9 @@ 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):
+   # Raise an exception if stat(source) fails, intentionally.
+   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 +208,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,22 +218,23 @@ 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, using 
stat()
+   rather than lstat(), in order to match the 
`install`
+   command
dest: path to the dest file.
 
Returns:
True if it should succeed.
"""
# 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)
+   # lstat() for dest.
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 != 

Re: [gentoo-portage-dev] [PATCH 2/2] sys-apps/portage: exclude /var/tmp/ccache from tmpfiles cleanup

[Zac Medico]

On 12/31/2017 07:22 AM, Mike Gilbert wrote:
> By default, systemd-tmpfiles removes files older than 30 days from /var/tmp.
> The default portage config sets CCACHE_DIR=/var/tmp/ccache.
> 
> Bug: https://bugs.gentoo.org/490676#c14
> Package-Manager: Portage-2.3.19_p3, Repoman-2.3.6_p37
> ---
>  sys-apps/portage/files/portage-ccache.conf | 2 ++
>  sys-apps/portage/portage-2.3.19.ebuild | 4 +++-
>  sys-apps/portage/portage-.ebuild   | 4 +++-
>  3 files changed, 8 insertions(+), 2 deletions(-)
>  create mode 100644 sys-apps/portage/files/portage-ccache.conf

Looks good. Maybe we should also call tmpfiles_process in pkg_postinst?
-- 
Thanks,
Zac



signature.asc
Description: OpenPGP digital signature