Re: [gentoo-portage-dev] [PATCH] Manifest._apply_max_mtime: account for removals and renames (bug 567920)

[Michał Górny]

On Tue, 15 Dec 2015 08:50:10 -0800
Zac Medico  wrote:

> On 12/15/2015 12:43 AM, Michał Górny wrote:
> > On Tue, 15 Dec 2015 00:10:03 -0800
> > Zac Medico  wrote:
> >   
> >> Include directory mtimes in the max mtime calculation, in order
> >> to account for removals and renames.  
> > 
> > Mildly irrelevant but I think this reached the point when we want to
> > make this optional, and off by default. In other words, we want
> > 'repoman manifest' run explicitly to work the old way.
> >   
> 
> I suppose we could support a "stable-mtime = true" attribute that would
> be set in layout.conf and/or repos.conf.

No. This is about tool behavior. Tool behavior should be set
in the tool, not in the object the tool is working on. It should be
a command-line option to repoman or egencache.

-- 
Best regards,
Michał Górny



pgpyM19BNOzGj.pgp
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH] Manifest._apply_max_mtime: account for removals and renames (bug 567920)

[Zac Medico]

On 12/15/2015 12:43 AM, Michał Górny wrote:
> On Tue, 15 Dec 2015 00:10:03 -0800
> Zac Medico  wrote:
> 
>> Include directory mtimes in the max mtime calculation, in order
>> to account for removals and renames.
> 
> Mildly irrelevant but I think this reached the point when we want to
> make this optional, and off by default. In other words, we want
> 'repoman manifest' run explicitly to work the old way.
> 

I suppose we could support a "stable-mtime = true" attribute that would
be set in layout.conf and/or repos.conf.

Theoretically, the difference in behavior is negligible except where
comparisons by tools like rsync are concerned. I have to admit that I
was a little bit nervous that something like bug 567920 might come up.
Now that directory mtimes are accounted for, I'm practically certain
that any relevant changes will cause the Manifest mtime to increase.
-- 
Thanks,
Zac

[gentoo-portage-dev] [PATCH] Manifest._apply_max_mtime: account for removals and renames (bug 567920)

[Zac Medico]

Include directory mtimes in the max mtime calculation, in order
to account for removals and renames.

Fixes: 6dacd0ed9f6d ("Manifest.write: stable/predictable Manifest mtime for 
rsync (bug 557962)")
X-Gentoo-Bug: 567920
X-Gentoo-Bug-url: https://bugs.gentoo.org/show_bug.cgi?id=567920
---
 pym/portage/manifest.py | 40 +---
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/pym/portage/manifest.py b/pym/portage/manifest.py
index f5cf0f5..451f869 100644
--- a/pym/portage/manifest.py
+++ b/pym/portage/manifest.py
@@ -282,7 +282,8 @@ class Manifest(object):
try:
myentries = list(self._createManifestEntries())
update_manifest = True
-   existing_st = None
+   preserved_stats = {}
+   preserved_stats[self.pkgdir.rstrip(os.sep)] = 
os.stat(self.pkgdir)
if myentries and not force:
try:
f = 
io.open(_unicode_encode(self.getFullname(),
@@ -290,7 +291,7 @@ class Manifest(object):
mode='r', 
encoding=_encodings['repo.content'],
errors='replace')
oldentries = 
list(self._parseManifestLines(f))
-   existing_st = os.fstat(f.fileno())
+   preserved_stats[self.getFullname()] = 
os.fstat(f.fileno())
f.close()
if len(oldentries) == len(myentries):
update_manifest = False
@@ -312,7 +313,7 @@ class Manifest(object):
# non-empty for all currently known use 
cases.
write_atomic(self.getFullname(), 
"".join("%s\n" %
_unicode(myentry) for myentry 
in myentries))
-   self._apply_max_mtime(existing_st, 
myentries)
+   self._apply_max_mtime(preserved_stats, 
myentries)
rval = True
else:
# With thin manifest, there's no need 
to have
@@ -332,17 +333,21 @@ class Manifest(object):
raise
return rval
 
-   def _apply_max_mtime(self, existing_st, entries):
+   def _apply_max_mtime(self, preserved_stats, entries):
"""
Set the Manifest mtime to the max mtime of all relevant files
-   (the existing Manifest mtime is included in order to account for
-   eclass modifications that change DIST entries). This results in 
a
+   and directories. Directory mtimes account for file renames and
+   removals. The existing Manifest mtime accounts for eclass
+   modifications that change DIST entries. This results in a
stable/predictable mtime, which is useful when converting thin
manifests to thick manifests for distribution via rsync. For
portability, the mtime is set with 1 second resolution.
 
@param existing_st: stat result for existing Manifest
@type existing_st: posix.stat_result
+   @param preserved_stats: maps paths to preserved stat results
+   that should be used instead place of os.stat() calls
+   @type preserved_stats: dict
@param entries: list of current Manifest2Entry instances
@type entries: list
"""
@@ -350,18 +355,31 @@ class Manifest(object):
# it always rounds down. Note that stat_result.st_mtime will 
round
# up from 0.9 to 1.0 when precision is lost during 
conversion
# from nanosecond resolution to float.
-   max_mtime = None if existing_st is None else 
existing_st[stat.ST_MTIME]
+   max_mtime = None
+   _update_max = (lambda st: max_mtime if max_mtime is not None
+   and max_mtime > st[stat.ST_MTIME] else 
st[stat.ST_MTIME])
+   _stat = (lambda path: preserved_stats[path] if path in 
preserved_stats
+   else os.stat(path))
+
+   for stat_result in preserved_stats.values():
+   max_mtime = _update_max(stat_result)
+
+   dirs = set()
for entry in entries:
if entry.type == 'DIST':
continue
abs_path = (os.path.join(self.pkgdir, 'files', 
entry.name) if
entry.type == 'AUX' else 
os.path.join(self.pkgdir, entry.name))
-