Re: [gentoo-portage-dev] [PATCH] egencache: add --stable-mtime option

[Michał Górny]

On Sat, 19 Dec 2015 15:51:29 -0800
Zac Medico  wrote:

> On 12/16/2015 09:49 AM, Michał Górny wrote:
> > On Wed, 16 Dec 2015 14:38:03 +0100
> > Alexander Berntsen  wrote:
> >   
> >> -BEGIN PGP SIGNED MESSAGE-
> >> Hash: SHA512
> >>
> >> On 16/12/15 06:33, Zac Medico wrote:  
> >>> Disable Manifest "stable mtime" behavior by default, and add a
> >>> corresponding egencache option.
> >> This message tells me nothing about why we need to do this.  
> > 
> > We need do this because we changed the behavior and the new behavior is
> > counter-intuitive. We already had a number of bugs caused by it,
> > and while it's used by Infra, it's at least unexpected when someone
> > manually runs 'repoman manifest'.  
> 
> Just because it was historically buggy does not mean that it will always
> be that way. I believe that it will be very safe once we've fixed it to
> include the mtimes of all relevant directories in the max mtime calculation.
> 
> > I mean, before this all started 'repoman manifest' just updated
> > the Manifest which meant it's mtime changed. Nowadays, it also sets
> > mtime to some value in the past, which means running 'repoman manifest'
> > may result in updated Manifest having mtime older than the old
> > Manifest. As a result, people using rsync are in trouble. And this has
> > been reported too by overlay owners.  
> 
> We should get our facts straight. It's not possible for the updated
> Manifest to have an older mtime than the old manifest, because the mtime
> of the old Manfiest is included in the max mtime calculation.

Unless you remove the old Manifest (and distfiles) to have Portage
refetch the files and recalc the digests.

-- 
Best regards,
Michał Górny



pgp7bnwuwRPH4.pgp
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH] egencache: add --stable-mtime option

[Zac Medico]

On 12/20/2015 01:24 AM, Michał Górny wrote:
> On Sat, 19 Dec 2015 15:51:29 -0800
> Zac Medico  wrote:
> 
>> On 12/16/2015 09:49 AM, Michał Górny wrote:
>>> On Wed, 16 Dec 2015 14:38:03 +0100
>>> Alexander Berntsen  wrote:
>>>   
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA512

 On 16/12/15 06:33, Zac Medico wrote:  
> Disable Manifest "stable mtime" behavior by default, and add a
> corresponding egencache option.
 This message tells me nothing about why we need to do this.  
>>>
>>> We need do this because we changed the behavior and the new behavior is
>>> counter-intuitive. We already had a number of bugs caused by it,
>>> and while it's used by Infra, it's at least unexpected when someone
>>> manually runs 'repoman manifest'.  
>>
>> Just because it was historically buggy does not mean that it will always
>> be that way. I believe that it will be very safe once we've fixed it to
>> include the mtimes of all relevant directories in the max mtime calculation.
>>
>>> I mean, before this all started 'repoman manifest' just updated
>>> the Manifest which meant it's mtime changed. Nowadays, it also sets
>>> mtime to some value in the past, which means running 'repoman manifest'
>>> may result in updated Manifest having mtime older than the old
>>> Manifest. As a result, people using rsync are in trouble. And this has
>>> been reported too by overlay owners.  
>>
>> We should get our facts straight. It's not possible for the updated
>> Manifest to have an older mtime than the old manifest, because the mtime
>> of the old Manfiest is included in the max mtime calculation.
> 
> Unless you remove the old Manifest (and distfiles) to have Portage
> refetch the files and recalc the digests.

Removing the Manifest will bump the mtime of its parent directory, and
that mtime is included in the max mtime calculation.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] egencache: add --stable-mtime option

[Brian Dolbec]

On Sat, 19 Dec 2015 15:51:29 -0800
Zac Medico  wrote:

> On 12/16/2015 09:49 AM, Michał Górny wrote:
> > On Wed, 16 Dec 2015 14:38:03 +0100
> > Alexander Berntsen  wrote:
> >   
> >> -BEGIN PGP SIGNED MESSAGE-
> >> Hash: SHA512
> >>
> >> On 16/12/15 06:33, Zac Medico wrote:  
> >>> Disable Manifest "stable mtime" behavior by default, and add a
> >>> corresponding egencache option.
> >> This message tells me nothing about why we need to do this.  
> > 
> > We need do this because we changed the behavior and the new
> > behavior is counter-intuitive. We already had a number of bugs
> > caused by it, and while it's used by Infra, it's at least
> > unexpected when someone manually runs 'repoman manifest'.  
> 
> Just because it was historically buggy does not mean that it will
> always be that way. I believe that it will be very safe once we've
> fixed it to include the mtimes of all relevant directories in the max
> mtime calculation.
> 
> > I mean, before this all started 'repoman manifest' just updated
> > the Manifest which meant it's mtime changed. Nowadays, it also sets
> > mtime to some value in the past, which means running 'repoman
> > manifest' may result in updated Manifest having mtime older than
> > the old Manifest. As a result, people using rsync are in trouble.
> > And this has been reported too by overlay owners.  
> 
> We should get our facts straight. It's not possible for the updated
> Manifest to have an older mtime than the old manifest, because the
> mtime of the old Manfiest is included in the max mtime calculation.


I'm generally in favour of this one, infra is changing how they
generate the Changelogs, etc.  So, making this an option makes sense
to me.  
-- 
Brian Dolbec 

Re: [gentoo-portage-dev] [PATCH] egencache: add --stable-mtime option

[Zac Medico]

On 12/16/2015 09:49 AM, Michał Górny wrote:
> On Wed, 16 Dec 2015 14:38:03 +0100
> Alexander Berntsen  wrote:
> 
>> -BEGIN PGP SIGNED MESSAGE-
>> Hash: SHA512
>>
>> On 16/12/15 06:33, Zac Medico wrote:
>>> Disable Manifest "stable mtime" behavior by default, and add a
>>> corresponding egencache option.  
>> This message tells me nothing about why we need to do this.
> 
> We need do this because we changed the behavior and the new behavior is
> counter-intuitive. We already had a number of bugs caused by it,
> and while it's used by Infra, it's at least unexpected when someone
> manually runs 'repoman manifest'.

Just because it was historically buggy does not mean that it will always
be that way. I believe that it will be very safe once we've fixed it to
include the mtimes of all relevant directories in the max mtime calculation.

> I mean, before this all started 'repoman manifest' just updated
> the Manifest which meant it's mtime changed. Nowadays, it also sets
> mtime to some value in the past, which means running 'repoman manifest'
> may result in updated Manifest having mtime older than the old
> Manifest. As a result, people using rsync are in trouble. And this has
> been reported too by overlay owners.

We should get our facts straight. It's not possible for the updated
Manifest to have an older mtime than the old manifest, because the mtime
of the old Manfiest is included in the max mtime calculation.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] egencache: add --stable-mtime option

[Michał Górny]

On Wed, 16 Dec 2015 14:38:03 +0100
Alexander Berntsen  wrote:

> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA512
> 
> On 16/12/15 06:33, Zac Medico wrote:
> > Disable Manifest "stable mtime" behavior by default, and add a
> > corresponding egencache option.  
> This message tells me nothing about why we need to do this.

We need do this because we changed the behavior and the new behavior is
counter-intuitive. We already had a number of bugs caused by it,
and while it's used by Infra, it's at least unexpected when someone
manually runs 'repoman manifest'.

I mean, before this all started 'repoman manifest' just updated
the Manifest which meant it's mtime changed. Nowadays, it also sets
mtime to some value in the past, which means running 'repoman manifest'
may result in updated Manifest having mtime older than the old
Manifest. As a result, people using rsync are in trouble. And this has
been reported too by overlay owners.

-- 
Best regards,
Michał Górny



pgpKVumO5Xwex.pgp
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH] egencache: add --stable-mtime option

[Alexander Berntsen]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512

On 16/12/15 06:33, Zac Medico wrote:
> Disable Manifest "stable mtime" behavior by default, and add a
> corresponding egencache option.
This message tells me nothing about why we need to do this.
- -- 
Alexander
berna...@gentoo.org
https://secure.plaimi.net/~alexander
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQIcBAEBCgAGBQJWcWk7AAoJENQqWdRUGk8B+vAQAIM0vppqXsIfCs3IlP7pmCme
VXJx++RS85r7+81Jx8eWzoVoiT5liX2obkqIhob+4gaZisF9WEOmO71plojbtDKe
DYGYsV97lRr0DEM52eQp51cbl8Le5Q4RujJm3qX3JQkH4CyvKdB05NNTAMjBMb3c
j7hk4CxFFqsD5CDWbE4c+3VyClEuB2pAO6rB4nuF8YKkwAip4B0TCWD9hyeiLe+V
5bubknSDDzA2McAaiYdlb7SF0Y6Hr2DOkogzhzf4R+xSEYcVT/RiDiG7oxkAZ1JA
GWNxnGYKni+TynwqYkueL1j9+rGLOvAVVzXKWB6R7qO9QBfGw8Zx3QJoCGbkx0Qj
Nhrq/u9F/sbFCyE50CnNqqcxxAEyxsnCk4SemO61tkG92c4sAQDrD4SGGUY41JHM
Hf1VnZsg+LBpH/5oz6n+ZsVHid1T8ISel6V/NSUEeQO+JDspEUZyFMrUKOMcGWGw
A2DJS2UGufWRxe9Q8cZkvtP4Q8meE+8gHW1m2iKZt+ghZn7Zos7zgfT6lZw6w69F
KtWd++JmTAGnH4Gyx857eGgzY7gxUFJTHSsMdUzps7nI+4NppyP3AqNZxDv/ETBx
d1jtwjKW7wFmglPXjK+vEMGj0rFJQbJJAjg9sxMQF1oUXgp7jFRmlHKSLOhHvXQc
R2l8HvdysLCUK1/pxAYf
=yW/c
-END PGP SIGNATURE-

Re: [gentoo-portage-dev] [PATCH] egencache: add --stable-mtime option

[Zac Medico]

On 12/16/2015 05:38 AM, Alexander Berntsen wrote:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA512
> 
> On 16/12/15 06:33, Zac Medico wrote:
>> Disable Manifest "stable mtime" behavior by default, and add a
>> corresponding egencache option.
> This message tells me nothing about why we need to do this.

Yeah, I'm not so sure that we need to do this, so let's hold off for now.
-- 
Thanks,
Zac

[gentoo-portage-dev] [PATCH] egencache: add --stable-mtime option

[Zac Medico]

Disable Manifest "stable mtime" behavior by default, and add
a corresponding egencache option.

Suggested-by: Michał Górny 
---
 bin/egencache  |  6 +-
 man/egencache.1|  3 +++
 pym/portage/manifest.py| 14 +-
 .../package/ebuild/_parallel_manifest/ManifestProcess.py   |  6 --
 .../package/ebuild/_parallel_manifest/ManifestScheduler.py |  7 +--
 .../package/ebuild/_parallel_manifest/ManifestTask.py  |  8 +---
 6 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/bin/egencache b/bin/egencache
index 7e3387e..07665e8 100755
--- a/bin/egencache
+++ b/bin/egencache
@@ -120,6 +120,9 @@ def parse_args(args):
choices=('y', 'n'),
metavar="",
help="manually override layout.conf sign-manifests setting")
+   common.add_argument("--stable-mtime",
+   action="store_true",
+   help="apply stable mtime to generated manifests (for rsync)")
common.add_argument("--strict-manifests",
choices=('y', 'n'),
metavar="",
@@ -1151,7 +1154,8 @@ def egencache_main(args):
force_sign_key=force_sign_key,
max_jobs=options.jobs,
max_load=options.load_average,
-   event_loop=event_loop)
+   event_loop=event_loop,
+   manifest_kwargs=dict(stable_mtime=options.stable_mtime))
 
signum = run_main_scheduler(scheduler)
if signum is not None:
diff --git a/man/egencache.1 b/man/egencache.1
index 7fd17c2..081e8c1 100644
--- a/man/egencache.1
+++ b/man/egencache.1
@@ -100,6 +100,9 @@ Manually override layout.conf sign-manifests setting.
 .BR "\-\-strict\-manifests< y | n >"
 Manually override "strict" FEATURES setting.
 .TP
+.BR "\-\-stable\-mtime"
+Apply stable mtime to generated manifests (for rsync).
+.TP
 .BR "\-\-thin\-manifests< y | n >"
 Manually override layout.conf thin-manifests setting.
 .TP
diff --git a/pym/portage/manifest.py b/pym/portage/manifest.py
index 818515f..0724272 100644
--- a/pym/portage/manifest.py
+++ b/pym/portage/manifest.py
@@ -128,7 +128,7 @@ class Manifest(object):
def __init__(self, pkgdir, distdir=None, fetchlist_dict=None,
manifest1_compat=DeprecationWarning, from_scratch=False, 
thin=False,
allow_missing=False, allow_create=True, hashes=None,
-   find_invalid_path_char=None):
+   find_invalid_path_char=None, stable_mtime=False):
""" Create new Manifest instance for package in pkgdir.
Do not parse Manifest file if from_scratch == True (only 
for internal use)
The fetchlist_dict parameter is required only for 
generation of
@@ -145,6 +145,7 @@ class Manifest(object):
find_invalid_path_char = _find_invalid_path_char
self._find_invalid_path_char = find_invalid_path_char
self.pkgdir = _unicode_decode(pkgdir).rstrip(os.sep) + os.sep
+   self.stable_mtime = stable_mtime
self.fhashdict = {}
self.hashes = set()
 
@@ -283,7 +284,6 @@ class Manifest(object):
myentries = list(self._createManifestEntries())
update_manifest = True
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(),
@@ -291,7 +291,9 @@ class Manifest(object):
mode='r', 
encoding=_encodings['repo.content'],
errors='replace')
oldentries = 
list(self._parseManifestLines(f))
-   preserved_stats[self.getFullname()] = 
os.fstat(f.fileno())
+   if self.stable_mtime:
+   
preserved_stats[self.getFullname()] = os.fstat(f.fileno())
+   
preserved_stats[self.pkgdir.rstrip(os.sep)] = os.stat(self.pkgdir)
f.close()
if len(oldentries) == len(myentries):
update_manifest = False
@@ -313,7 +315,8 @@ class Manifest(object):
# non-empty for all currently known use 
cases.
write_atomic(self.getFullname(), 
"".join("%s\n" %
_unicode(myentry) for