Re: [gentoo-portage-dev] [PATCH v2] Move INSTALL_MASK handling into merging

[Zac Medico]

On 06/12/2016 01:43 PM, Zac Medico wrote:
> On 06/12/2016 01:29 PM, Zac Medico wrote:
>> On 05/22/2016 01:21 AM, Michał Górny wrote:
>>> diff --git a/pym/portage/package/ebuild/config.py 
>>> b/pym/portage/package/ebuild/config.py
>>> index fcc7ce5..9d13703 100644
>>> --- a/pym/portage/package/ebuild/config.py
>>> +++ b/pym/portage/package/ebuild/config.py
>>> @@ -1774,14 +1774,14 @@ class config(object):
>>> _eapi_cache.clear()
>>>  
>>> # Prepare the final value of INSTALL_MASK
>>> -   install_mask = self["INSTALL_MASK"].split()
>>> +   install_mask = self.get("INSTALL_MASK", "").split()
>>> if 'nodoc' in self.features:
>>> install_mask.append("/usr/share/doc")
>>> if 'noinfo' in self.features:
>>> install_mask.append("/usr/share/info")
>>> if 'noman' in self.features:
>>> install_mask.append("/usr/share/man")
>>> -   self["INSTALL_MASK"] = ' '.join(install_mask)
>>> +   self.install_mask = install_mask
>>>  
>>> def _grab_pkg_env(self, penv, container, protected_keys=None):
>>> if protected_keys is None:
>>>
>>
>> The config.reset method should reset self.install_mask to match the
>> global INSTALL_MASK setting.
>>
> 
> For the benefit of those who may not be as familiar with the config
> class, here is some of the related irc discussion:
> 
>  mgorny: config.setcpv put's the config into per-package state,
> and config.reset reverts it to global state
>  i see
>  so how does that affect me? ;-D
>  because yout set self.install_mask in config.setcpv
>  so you also have to handle global setting of that attribute
>  every attribute has global and per-package state
>  for example the usemask attribute is similar
>  initially it has global use.mask settings
>  hmm, so you mean i can't rely on both being combined properly
> in .setcpv() ?
>  hmm, but wait, do i need to care about global state at all?
>  yes, that's what config.reset is for
>  it transitions back to global state
>  but is there any case when i need the global state?
>  i don't think the mask is used without packages
>  if that's the case, the you can set it to None whien the
> config is in global mode
>  careful though, setcpv returns early in some cases
>  and you set self.install_mask at the very end of that method
>  so it's not guaranteed to execute
>  so you have end up with self.install_mask having the global
> setting after a setcpv call
>  which is fine if there's no package.env setting for that package
> 

 mgorny: also, you should copy self.install_mask in the clone
part of the config constructor
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH v2] Move INSTALL_MASK handling into merging

[Zac Medico]

On 06/12/2016 01:29 PM, Zac Medico wrote:
> On 05/22/2016 01:21 AM, Michał Górny wrote:
>> diff --git a/pym/portage/package/ebuild/config.py 
>> b/pym/portage/package/ebuild/config.py
>> index fcc7ce5..9d13703 100644
>> --- a/pym/portage/package/ebuild/config.py
>> +++ b/pym/portage/package/ebuild/config.py
>> @@ -1774,14 +1774,14 @@ class config(object):
>>  _eapi_cache.clear()
>>  
>>  # Prepare the final value of INSTALL_MASK
>> -install_mask = self["INSTALL_MASK"].split()
>> +install_mask = self.get("INSTALL_MASK", "").split()
>>  if 'nodoc' in self.features:
>>  install_mask.append("/usr/share/doc")
>>  if 'noinfo' in self.features:
>>  install_mask.append("/usr/share/info")
>>  if 'noman' in self.features:
>>  install_mask.append("/usr/share/man")
>> -self["INSTALL_MASK"] = ' '.join(install_mask)
>> +self.install_mask = install_mask
>>  
>>  def _grab_pkg_env(self, penv, container, protected_keys=None):
>>  if protected_keys is None:
>>
> 
> The config.reset method should reset self.install_mask to match the
> global INSTALL_MASK setting.
> 

For the benefit of those who may not be as familiar with the config
class, here is some of the related irc discussion:

 mgorny: config.setcpv put's the config into per-package state,
and config.reset reverts it to global state
 i see
 so how does that affect me? ;-D
 because yout set self.install_mask in config.setcpv
 so you also have to handle global setting of that attribute
 every attribute has global and per-package state
 for example the usemask attribute is similar
 initially it has global use.mask settings
 hmm, so you mean i can't rely on both being combined properly
in .setcpv() ?
 hmm, but wait, do i need to care about global state at all?
 yes, that's what config.reset is for
 it transitions back to global state
 but is there any case when i need the global state?
 i don't think the mask is used without packages
 if that's the case, the you can set it to None whien the
config is in global mode
 careful though, setcpv returns early in some cases
 and you set self.install_mask at the very end of that method
 so it's not guaranteed to execute
 so you have end up with self.install_mask having the global
setting after a setcpv call
 which is fine if there's no package.env setting for that package
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH v2] Move INSTALL_MASK handling into merging

[Zac Medico]

On 05/22/2016 01:21 AM, Michał Górny wrote:
> diff --git a/pym/portage/package/ebuild/config.py 
> b/pym/portage/package/ebuild/config.py
> index fcc7ce5..9d13703 100644
> --- a/pym/portage/package/ebuild/config.py
> +++ b/pym/portage/package/ebuild/config.py
> @@ -1774,14 +1774,14 @@ class config(object):
>   _eapi_cache.clear()
>  
>   # Prepare the final value of INSTALL_MASK
> - install_mask = self["INSTALL_MASK"].split()
> + install_mask = self.get("INSTALL_MASK", "").split()
>   if 'nodoc' in self.features:
>   install_mask.append("/usr/share/doc")
>   if 'noinfo' in self.features:
>   install_mask.append("/usr/share/info")
>   if 'noman' in self.features:
>   install_mask.append("/usr/share/man")
> - self["INSTALL_MASK"] = ' '.join(install_mask)
> + self.install_mask = install_mask
>  
>   def _grab_pkg_env(self, penv, container, protected_keys=None):
>   if protected_keys is None:
> 

The config.reset method should reset self.install_mask to match the
global INSTALL_MASK setting.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH v2] Move INSTALL_MASK handling into merging

[Zac Medico]

On 06/12/2016 03:05 AM, Michał Górny wrote:
> Dnia 12 czerwca 2016 11:49:26 CEST, Zac Medico  
> napisał(a):
>> On 06/12/2016 02:28 AM, Michał Górny wrote:
>>> Dnia 12 czerwca 2016 11:10:55 CEST, Zac Medico 
>> napisał(a):
 On 05/22/2016 01:21 AM, Michał Górny wrote:
> Introduce a new logic for INSTALL_MASK handling in merging code,
> replacing the old code that removed matching files and directories
> from imagedir in bash. The new code actually ignores matching files
> on-the-fly while testing for file collisions and merging files.
> The files are still written to CONTENTS, and output using "###"
>> zing
> to indicate being masked, yet are not actually merged to the
 filesystem.

 Since collision-protect relies on existing files in its collision
>> test,
 install-masked files are no longer going to trigger collisions.
>> Then,
 since the install-masked files are still written to CONTENTS, it's
 possible for the unmerge of one package to unmerge colliding files
>> that
 belong to another package!

 There are a number of ways to solve this problem. For example, we
>> could
 have the unmerge code ignore any files in CONTENTS that match the
 INSTALL_MASK value that was used at merge time.
>>>
>>> Hmm, thinking about this more widely (i.e. actually thinking rather
>> than mimicking the old behavior), I think it would be better to
>> actually use the original file set for collision-protect. This will
>> make it possible to detect collisions that would otherwise be hidden
>> via INSTALL_MASK.
>>
>> Even then, we have to carefully consider how the absence of installed
>> files affects the collision test. It's safest for the unmerge code to
>> be
>> aware of the merge time INSTALL_MASK setting, and not try to unmerge
>> the
>> install-masked files.
> 
> But then it wouldn't unmerge the newly masked files as well.

I'm suggesting to save the INSTALL_MASK setting from merge time, so any
new INSTALL_MASK settings since then are irrelevant.

> Getting this right will require a lot of effort, and we're less likely to 
> screw something up if we keep it simple.

You can use exactly the same pattern matching code for merge and
unmerge. It's worthwhile, given the say that collision-protect relies on
file existence in the collision test.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH v2] Move INSTALL_MASK handling into merging

[Michał Górny]

Dnia 12 czerwca 2016 11:49:26 CEST, Zac Medico  napisał(a):
>On 06/12/2016 02:28 AM, Michał Górny wrote:
>> Dnia 12 czerwca 2016 11:10:55 CEST, Zac Medico 
>napisał(a):
>>> On 05/22/2016 01:21 AM, Michał Górny wrote:
 Introduce a new logic for INSTALL_MASK handling in merging code,
 replacing the old code that removed matching files and directories
 from imagedir in bash. The new code actually ignores matching files
 on-the-fly while testing for file collisions and merging files.
 The files are still written to CONTENTS, and output using "###"
>zing
 to indicate being masked, yet are not actually merged to the
>>> filesystem.
>>>
>>> Since collision-protect relies on existing files in its collision
>test,
>>> install-masked files are no longer going to trigger collisions.
>Then,
>>> since the install-masked files are still written to CONTENTS, it's
>>> possible for the unmerge of one package to unmerge colliding files
>that
>>> belong to another package!
>>>
>>> There are a number of ways to solve this problem. For example, we
>could
>>> have the unmerge code ignore any files in CONTENTS that match the
>>> INSTALL_MASK value that was used at merge time.
>> 
>> Hmm, thinking about this more widely (i.e. actually thinking rather
>than mimicking the old behavior), I think it would be better to
>actually use the original file set for collision-protect. This will
>make it possible to detect collisions that would otherwise be hidden
>via INSTALL_MASK.
>
>Even then, we have to carefully consider how the absence of installed
>files affects the collision test. It's safest for the unmerge code to
>be
>aware of the merge time INSTALL_MASK setting, and not try to unmerge
>the
>install-masked files.

But then it wouldn't unmerge the newly masked files as well. Getting this right 
will require a lot of effort, and we're less likely to screw something up if we 
keep it simple.


-- 
Best regards,
Michał Górny (by phone)

Re: [gentoo-portage-dev] [PATCH v2] Move INSTALL_MASK handling into merging

[Zac Medico]

On 06/12/2016 02:28 AM, Michał Górny wrote:
> Dnia 12 czerwca 2016 11:10:55 CEST, Zac Medico  
> napisał(a):
>> On 05/22/2016 01:21 AM, Michał Górny wrote:
>>> Introduce a new logic for INSTALL_MASK handling in merging code,
>>> replacing the old code that removed matching files and directories
>>> from imagedir in bash. The new code actually ignores matching files
>>> on-the-fly while testing for file collisions and merging files.
>>> The files are still written to CONTENTS, and output using "###" zing
>>> to indicate being masked, yet are not actually merged to the
>> filesystem.
>>
>> Since collision-protect relies on existing files in its collision test,
>> install-masked files are no longer going to trigger collisions. Then,
>> since the install-masked files are still written to CONTENTS, it's
>> possible for the unmerge of one package to unmerge colliding files that
>> belong to another package!
>>
>> There are a number of ways to solve this problem. For example, we could
>> have the unmerge code ignore any files in CONTENTS that match the
>> INSTALL_MASK value that was used at merge time.
> 
> Hmm, thinking about this more widely (i.e. actually thinking rather than 
> mimicking the old behavior), I think it would be better to actually use the 
> original file set for collision-protect. This will make it possible to detect 
> collisions that would otherwise be hidden via INSTALL_MASK.

Even then, we have to carefully consider how the absence of installed
files affects the collision test. It's safest for the unmerge code to be
aware of the merge time INSTALL_MASK setting, and not try to unmerge the
install-masked files.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH v2] Move INSTALL_MASK handling into merging

[Michał Górny]

Dnia 12 czerwca 2016 11:10:55 CEST, Zac Medico  napisał(a):
>On 05/22/2016 01:21 AM, Michał Górny wrote:
>> Introduce a new logic for INSTALL_MASK handling in merging code,
>> replacing the old code that removed matching files and directories
>> from imagedir in bash. The new code actually ignores matching files
>> on-the-fly while testing for file collisions and merging files.
>> The files are still written to CONTENTS, and output using "###" zing
>> to indicate being masked, yet are not actually merged to the
>filesystem.
>
>Since collision-protect relies on existing files in its collision test,
>install-masked files are no longer going to trigger collisions. Then,
>since the install-masked files are still written to CONTENTS, it's
>possible for the unmerge of one package to unmerge colliding files that
>belong to another package!
>
>There are a number of ways to solve this problem. For example, we could
>have the unmerge code ignore any files in CONTENTS that match the
>INSTALL_MASK value that was used at merge time.

Hmm, thinking about this more widely (i.e. actually thinking rather than 
mimicking the old behavior), I think it would be better to actually use the 
original file set for collision-protect. This will make it possible to detect 
collisions that would otherwise be hidden via INSTALL_MASK.

-- 
Best regards,
Michał Górny (by phone)

Re: [gentoo-portage-dev] [PATCH v2] Move INSTALL_MASK handling into merging

[Zac Medico]

On 05/22/2016 01:21 AM, Michał Górny wrote:
> Introduce a new logic for INSTALL_MASK handling in merging code,
> replacing the old code that removed matching files and directories
> from imagedir in bash. The new code actually ignores matching files
> on-the-fly while testing for file collisions and merging files.
> The files are still written to CONTENTS, and output using "###" zing
> to indicate being masked, yet are not actually merged to the filesystem.

Since collision-protect relies on existing files in its collision test,
install-masked files are no longer going to trigger collisions. Then,
since the install-masked files are still written to CONTENTS, it's
possible for the unmerge of one package to unmerge colliding files that
belong to another package!

There are a number of ways to solve this problem. For example, we could
have the unmerge code ignore any files in CONTENTS that match the
INSTALL_MASK value that was used at merge time.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH v2] Move INSTALL_MASK handling into merging

[Zac Medico]

On 05/22/2016 01:21 AM, Michał Górny wrote:
> + def _is_install_masked(self, relative_path):
> + ret = False
> + for pattern in self.settings.install_mask:
> + # absolute path pattern
> + if pattern.startswith('/'):
> + # match either exact path or one of parent dirs
> + # the latter is done via matching pattern/*
> + if (fnmatch.fnmatch(relative_path, pattern[1:])
> + or 
> fnmatch.fnmatch(relative_path, pattern[1:] + '/*')):
> + ret = True
> + break
> + # filename
> + else:
> + if 
> fnmatch.fnmatch(os.path.basename(relative_path), pattern):
> + ret = True
> + break
> + return ret
> +

This is a hot spot, so it should use a pre-compiled regular expression,
using | to join the results of fnmatch.translate(pattern) calls for each
pattern.
-- 
Thanks,
Zac