[gentoo-portage-dev] [PATCH 0/3] INSTALL_MASK redesign, part I

[gentoo-portage-dev] [PATCH 0/3] INSTALL_MASK redesign, part I

[Michał Górny]

Hello, everyone.

As part of new GLEP effort, I'm working on improving INSTALL_MASK
support in Portage to make it cleaner and more featureful. Here's
the first set of commits resulting from this.

The previous implementation of INSTALL_MASK was done purely in bash.
Long story short, Portage removed all files matching one of INSTALL_MASK
patterns from the installation image.

My implementation is done purely as a filter in Python. The ebuild
configuration bits now generate final list of INSTALL_MASK. It is
afterwards used in vartree bits to filter files while checking for
collisions, merging and unmerging appropriately.

The major differences/improvements are:

* The files are actually left in image directory. Not that it makes any
  real difference but it is a bit cleaner and more like real mask this
  way.

* The pre-install "removing" output has been replaced by listing files
  with "###" zing in merging output (which means "not installed due
  to INSTALL_MASK").

* All masked files are now listed in vdb CONTENTS. Therefore, tools
  like app-portage/install-mask can now figure out to which packages
  masks were applied and rebuild them on mask changes appropriately.

* Mask exclusions are supported now. Which means you can do e.g.:
  INSTALL_MASK="/usr/share/locale -/usr/share/locale/foo".

* The code is now whitespace-safe. While patterns specified
  in INSTALL_MASK directly still can not contain spaces, pattern groups
  will be able to use them.

In a few days, I'll try to provide a part II that would implement
the actual mask groups. However, I may need some help adding support
for the configuration files.

I'm not touching PKG_INSTALL_MASK for now since it's harder and outside
of the scope of what I'm trying to achieve.



Michał Górny (3):
  portage.package.ebuild.config: Move FEATURES=no* handling there
  Move INSTALL_MASK handling into merging
  portage.dbapi.vartree: Support exclusions in INSTALL_MASK

 bin/misc-functions.sh                |  30 ----------
 pym/portage/dbapi/vartree.py         | 104 ++++++++++++++++++++++-------------
 pym/portage/package/ebuild/config.py |  10 ++++
 3 files changed, 76 insertions(+), 68 deletions(-)

-- 
2.8.3

[gentoo-portage-dev] [PATCH 1/3] portage.package.ebuild.config: Move FEATURES=no* handling there

[Michał Górny]

Move the code responsible for adding additional paths to INSTALL_MASK
into portage.package.ebuild.config.
---
 bin/misc-functions.sh                | 13 -------------
 pym/portage/package/ebuild/config.py | 10 ++++++++++
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/bin/misc-functions.sh b/bin/misc-functions.sh
index 58755a1..b42e1d6 100755
--- a/bin/misc-functions.sh
+++ b/bin/misc-functions.sh
@@ -314,20 +314,7 @@ preinst_mask() {
 	# in there in case any tools were built with -pg in CFLAGS.
 	cd "${T}"
 
-	# remove man pages, info pages, docs if requested
-	local f
-	for f in man info doc; do
-		if has no${f} $FEATURES; then
-			INSTALL_MASK="${INSTALL_MASK} /usr/share/${f}"
-		fi
-	done
-
 	install_mask "${ED}" "${INSTALL_MASK}"
-
-	# remove share dir if unnessesary
-	if has nodoc $FEATURES || has noman $FEATURES || has noinfo $FEATURES; then
-		rmdir "${ED}usr/share" &> /dev/null
-	fi
 }
 
 preinst_sfperms() {
diff --git a/pym/portage/package/ebuild/config.py b/pym/portage/package/ebuild/config.py
index 45b7d08..fcc7ce5 100644
--- a/pym/portage/package/ebuild/config.py
+++ b/pym/portage/package/ebuild/config.py
@@ -1773,6 +1773,16 @@ class config(object):
 		# setcpv triggers lazy instantiation of things like _use_manager.
 		_eapi_cache.clear()
 
+		# Prepare the final value of INSTALL_MASK
+		install_mask = self["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)
+
 	def _grab_pkg_env(self, penv, container, protected_keys=None):
 		if protected_keys is None:
 			protected_keys = ()
-- 
2.8.3

Re: [gentoo-portage-dev] [PATCH 1/3] portage.package.ebuild.config: Move FEATURES=no* handling there

[Zac Medico]

On 05/21/2016 11:56 PM, Michał Górny wrote:
> diff --git a/pym/portage/package/ebuild/config.py b/pym/portage/package/ebuild/config.py
> index 45b7d08..fcc7ce5 100644
> --- a/pym/portage/package/ebuild/config.py
> +++ b/pym/portage/package/ebuild/config.py
> @@ -1773,6 +1773,16 @@ class config(object):
>  		# setcpv triggers lazy instantiation of things like _use_manager.
>  		_eapi_cache.clear()
>  
> +		# Prepare the final value of INSTALL_MASK
> +		install_mask = self["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)
> +
>  	def _grab_pkg_env(self, penv, container, protected_keys=None):
>  		if protected_keys is None:
>  			protected_keys = ()
> 

I'm concerned that these values can be appended more than once, causing
the variable to contain duplicate values, because setcpv only calls
reset when the has_changed variable is True. So, we only want to call
this code when has_changed is True. In fact, this code can go
immediately after the reset call here:

		if has_changed:
			self.reset(keeping_pkg=1)
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH 1/3] portage.package.ebuild.config: Move FEATURES=no* handling there

[Zac Medico]

On 06/12/2016 12:19 AM, Zac Medico wrote:
> On 05/21/2016 11:56 PM, Michał Górny wrote:
>> diff --git a/pym/portage/package/ebuild/config.py b/pym/portage/package/ebuild/config.py
>> index 45b7d08..fcc7ce5 100644
>> --- a/pym/portage/package/ebuild/config.py
>> +++ b/pym/portage/package/ebuild/config.py
>> @@ -1773,6 +1773,16 @@ class config(object):
>>  		# setcpv triggers lazy instantiation of things like _use_manager.
>>  		_eapi_cache.clear()
>>  
>> +		# Prepare the final value of INSTALL_MASK
>> +		install_mask = self["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)
>> +
>>  	def _grab_pkg_env(self, penv, container, protected_keys=None):
>>  		if protected_keys is None:
>>  			protected_keys = ()
>>
> 
> I'm concerned that these values can be appended more than once, causing
> the variable to contain duplicate values, because setcpv only calls
> reset when the has_changed variable is True. So, we only want to call
> this code when has_changed is True. In fact, this code can go
> immediately after the reset call here:
> 
> 		if has_changed:
> 			self.reset(keeping_pkg=1)
> 

Actually, it's more tricky than that, because we need to account for
both global FEATURES settings and package.env FEATURES settings, and my
above statements do not account for the global settings.

Also need to consider the case where these features are enabled
globally, and then *disabled* via package.env!
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH 1/3] portage.package.ebuild.config: Move FEATURES=no* handling there

[Michał Górny]

Dnia 12 czerwca 2016 09:28:14 CEST, Zac Medico <zmedico@gentoo.org> napisał(a):
>On 06/12/2016 12:19 AM, Zac Medico wrote:
>> On 05/21/2016 11:56 PM, Michał Górny wrote:
>>> diff --git a/pym/portage/package/ebuild/config.py
>b/pym/portage/package/ebuild/config.py
>>> index 45b7d08..fcc7ce5 100644
>>> --- a/pym/portage/package/ebuild/config.py
>>> +++ b/pym/portage/package/ebuild/config.py
>>> @@ -1773,6 +1773,16 @@ class config(object):
>>>  		# setcpv triggers lazy instantiation of things like _use_manager.
>>>  		_eapi_cache.clear()
>>>  
>>> +		# Prepare the final value of INSTALL_MASK
>>> +		install_mask = self["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)
>>> +
>>>  	def _grab_pkg_env(self, penv, container, protected_keys=None):
>>>  		if protected_keys is None:
>>>  			protected_keys = ()
>>>
>> 
>> I'm concerned that these values can be appended more than once,
>causing
>> the variable to contain duplicate values, because setcpv only calls
>> reset when the has_changed variable is True. So, we only want to call
>> this code when has_changed is True. In fact, this code can go
>> immediately after the reset call here:
>> 
>> 		if has_changed:
>> 			self.reset(keeping_pkg=1)
>> 
>
>Actually, it's more tricky than that, because we need to account for
>both global FEATURES settings and package.env FEATURES settings, and my
>above statements do not account for the global settings.
>
>Also need to consider the case where these features are enabled
>globally, and then *disabled* via package.env!

Then maybe we should just ban them and tell people to use INSTALL_MASK directly instead. Their behavior is quite unclear anyway, considering later possible exclusions.


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

[gentoo-portage-dev] [PATCH 2/3] Move INSTALL_MASK handling into merging

[Michał Górny]

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.
---
 bin/misc-functions.sh                |  17 ------
 pym/portage/dbapi/vartree.py         | 102 ++++++++++++++++++++++-------------
 pym/portage/package/ebuild/config.py |   2 +-
 3 files changed, 65 insertions(+), 56 deletions(-)

diff --git a/bin/misc-functions.sh b/bin/misc-functions.sh
index b42e1d6..4086981 100755
--- a/bin/misc-functions.sh
+++ b/bin/misc-functions.sh
@@ -300,23 +300,6 @@ install_mask() {
 	set -${shopts}
 }
 
-preinst_mask() {
-	if [ -z "${D}" ]; then
-		 eerror "${FUNCNAME}: D is unset"
-		 return 1
-	fi
-
-	if ! ___eapi_has_prefix_variables; then
-		local ED=${D}
-	fi
-
-	# Make sure $PWD is not ${D} so that we don't leave gmon.out files
-	# in there in case any tools were built with -pg in CFLAGS.
-	cd "${T}"
-
-	install_mask "${ED}" "${INSTALL_MASK}"
-}
-
 preinst_sfperms() {
 	if [ -z "${D}" ]; then
 		 eerror "${FUNCNAME}: D is unset"
diff --git a/pym/portage/dbapi/vartree.py b/pym/portage/dbapi/vartree.py
index 6209a86..8e5ac43 100644
--- a/pym/portage/dbapi/vartree.py
+++ b/pym/portage/dbapi/vartree.py
@@ -2490,7 +2490,7 @@ class dblink(object):
 								(statobj.st_dev, statobj.st_ino),
 								[]).append(relative_path)
 
-					if is_owned:
+					if is_owned and not self._is_install_masked(relative_path[1:]):
 						show_unmerge("---", unmerge_desc["replaced"], file_type, obj)
 						continue
 					elif relative_path in cfgfiledict:
@@ -3687,6 +3687,24 @@ class dblink(object):
 	def _emerge_log(self, msg):
 		emergelog(False, msg)
 
+	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
+
 	def treewalk(self, srcroot, destroot, inforoot, myebuild, cleanup=0,
 		mydbapi=None, prev_mtimes=None, counter=None):
 		"""
@@ -3846,16 +3864,6 @@ class dblink(object):
 					max_dblnk = dblnk
 			self._installed_instance = max_dblnk
 
-		# Apply INSTALL_MASK before collision-protect, since it may
-		# be useful to avoid collisions in some scenarios.
-		# We cannot detect if this is needed or not here as INSTALL_MASK can be
-		# modified by bashrc files.
-		phase = MiscFunctionsProcess(background=False,
-			commands=["preinst_mask"], phase="preinst",
-			scheduler=self._scheduler, settings=self.settings)
-		phase.start()
-		phase.wait()
-
 		# We check for unicode encoding issues after src_install. However,
 		# the check must be repeated here for binary packages (it's
 		# inexpensive since we call os.walk() here anyway).
@@ -3927,6 +3935,10 @@ class dblink(object):
 
 					relative_path = fpath[srcroot_len:]
 
+					# filter on INSTALL_MASK
+					if self._is_install_masked(relative_path):
+						continue
+
 					if line_ending_re.search(relative_path) is not None:
 						paths_with_newlines.append(relative_path)
 
@@ -4642,6 +4654,7 @@ class dblink(object):
 		while mergelist:
 
 			relative_path = mergelist.pop()
+			instmasked = self._is_install_masked(relative_path)
 			mysrc = join(srcroot, relative_path)
 			mydest = join(destroot, relative_path)
 			# myrealdest is mydest without the $ROOT prefix (makes a difference if ROOT!="/")
@@ -4730,7 +4743,7 @@ class dblink(object):
 				destmd5 = None
 
 			moveme = True
-			if protected:
+			if protected and not instmasked:
 				mydest, protected, moveme = self._protect(cfgfiledict,
 					protect_if_modified, mymd5, myto, mydest,
 					myrealdest, mydmode, destmd5, mydest_link)
@@ -4758,7 +4771,7 @@ class dblink(object):
 				# we can simply test for existence of this file to see if the target has been merged yet
 				myrealto = normalize_path(os.path.join(destroot, myabsto))
 				if mydmode is not None and stat.S_ISDIR(mydmode):
-					if not protected:
+					if not protected and not instmasked:
 						# we can't merge a symlink over a directory
 						newdest = self._new_backup_path(mydest)
 						msg = []
@@ -4778,26 +4791,32 @@ class dblink(object):
 					# it later.
 					secondhand.append(mysrc[len(srcroot):])
 					continue
-				# unlinking no longer necessary; "movefile" will overwrite symlinks atomically and correctly
-				if moveme:
-					zing = ">>>"
-					mymtime = movefile(mysrc, mydest, newmtime=thismtime,
-						sstat=mystat, mysettings=self.settings,
-						encoding=_encodings['merge'])
 
-				try:
-					self._merged_path(mydest, os.lstat(mydest))
-				except OSError:
-					pass
+				if instmasked:
+					zing = "###"
+					# pass mymtime through from initial stat
+				else:
+					# unlinking no longer necessary; "movefile" will overwrite symlinks atomically and correctly
+					if moveme:
+						zing = ">>>"
+						mymtime = movefile(mysrc, mydest, newmtime=thismtime,
+							sstat=mystat, mysettings=self.settings,
+							encoding=_encodings['merge'])
+
+					try:
+						self._merged_path(mydest, os.lstat(mydest))
+					except OSError:
+						pass
 
 				if mymtime != None:
-					# Use lexists, since if the target happens to be a broken
-					# symlink then that should trigger an independent warning.
-					if not (os.path.lexists(myrealto) or
-						os.path.lexists(join(srcroot, myabsto))):
-						self._eqawarn('preinst',
-							[_("QA Notice: Symbolic link /%s points to /%s which does not exist.")
-							% (relative_path, myabsto)])
+					if not instmasked:
+						# Use lexists, since if the target happens to be a broken
+						# symlink then that should trigger an independent warning.
+						if not (os.path.lexists(myrealto) or
+							os.path.lexists(join(srcroot, myabsto))):
+							self._eqawarn('preinst',
+								[_("QA Notice: Symbolic link /%s points to /%s which does not exist.")
+								% (relative_path, myabsto)])
 
 					showMessage("%s %s -> %s\n" % (zing, mydest, myto))
 					if sys.hexversion >= 0x3030000:
@@ -4812,7 +4831,9 @@ class dblink(object):
 					return 1
 			elif stat.S_ISDIR(mymode):
 				# we are merging a directory
-				if mydmode != None:
+				if instmasked:
+					showMessage("### %s/\n" % mydest)
+				elif mydmode != None:
 					# destination exists
 
 					if bsd_chflags:
@@ -4899,10 +4920,11 @@ class dblink(object):
 					os.chown(mydest, mystat[4], mystat[5])
 					showMessage(">>> %s/\n" % mydest)
 
-				try:
-					self._merged_path(mydest, os.lstat(mydest))
-				except OSError:
-					pass
+				if not instmasked:
+					try:
+						self._merged_path(mydest, os.lstat(mydest))
+					except OSError:
+						pass
 
 				outfile.write("dir "+myrealdest+"\n")
 				# recurse and merge this directory
@@ -4911,7 +4933,7 @@ class dblink(object):
 
 			elif stat.S_ISREG(mymode):
 				# we are merging a regular file
-				if not protected and \
+				if not protected and not instmasked and \
 					mydmode is not None and stat.S_ISDIR(mydmode):
 						# install of destination is blocked by an existing directory with the same name
 						newdest = self._new_backup_path(mydest)
@@ -4925,9 +4947,11 @@ class dblink(object):
 						self._eerror("preinst", msg)
 						mydest = newdest
 
+				if instmasked:
+					zing = "###"
 				# whether config protection or not, we merge the new file the
 				# same way.  Unless moveme=0 (blocking directory)
-				if moveme:
+				elif moveme:
 					# Create hardlinks only for source files that already exist
 					# as hardlinks (having identical st_dev and st_ino).
 					hardlink_key = (mystat.st_dev, mystat.st_ino)
@@ -4960,7 +4984,9 @@ class dblink(object):
 			else:
 				# we are merging a fifo or device node
 				zing = "!!!"
-				if mydmode is None:
+				if instmasked:
+					zing = "###"
+				elif mydmode is None:
 					# destination doesn't exist
 					if movefile(mysrc, mydest, newmtime=thismtime,
 						sstat=mystat, mysettings=self.settings,
diff --git a/pym/portage/package/ebuild/config.py b/pym/portage/package/ebuild/config.py
index fcc7ce5..8bc5883 100644
--- a/pym/portage/package/ebuild/config.py
+++ b/pym/portage/package/ebuild/config.py
@@ -1781,7 +1781,7 @@ class config(object):
 			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:
-- 
2.8.3

Re: [gentoo-portage-dev] [PATCH 2/3] Move INSTALL_MASK handling into merging

[Michał Górny]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

On Sun, 22 May 2016 08:56:03 +0200
Michał Górny <mgorny@gentoo.org> 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.
> ---
>  bin/misc-functions.sh                |  17 ------
>  pym/portage/dbapi/vartree.py         | 102 ++++++++++++++++++++++-------------
>  pym/portage/package/ebuild/config.py |   2 +-
>  3 files changed, 65 insertions(+), 56 deletions(-)

This actually fails when INSTALL_MASK is not defined. I will be sending
another patch shortly.

-- 
Best regards,
Michał Górny
<http://dev.gentoo.org/~mgorny/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 949 bytes --]

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

[Michał Górny]

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.
---
 bin/misc-functions.sh                |  17 ------
 pym/portage/dbapi/vartree.py         | 102 ++++++++++++++++++++++-------------
 pym/portage/package/ebuild/config.py |   4 +-
 3 files changed, 66 insertions(+), 57 deletions(-)

diff --git a/bin/misc-functions.sh b/bin/misc-functions.sh
index b42e1d6..4086981 100755
--- a/bin/misc-functions.sh
+++ b/bin/misc-functions.sh
@@ -300,23 +300,6 @@ install_mask() {
 	set -${shopts}
 }
 
-preinst_mask() {
-	if [ -z "${D}" ]; then
-		 eerror "${FUNCNAME}: D is unset"
-		 return 1
-	fi
-
-	if ! ___eapi_has_prefix_variables; then
-		local ED=${D}
-	fi
-
-	# Make sure $PWD is not ${D} so that we don't leave gmon.out files
-	# in there in case any tools were built with -pg in CFLAGS.
-	cd "${T}"
-
-	install_mask "${ED}" "${INSTALL_MASK}"
-}
-
 preinst_sfperms() {
 	if [ -z "${D}" ]; then
 		 eerror "${FUNCNAME}: D is unset"
diff --git a/pym/portage/dbapi/vartree.py b/pym/portage/dbapi/vartree.py
index 6209a86..8e5ac43 100644
--- a/pym/portage/dbapi/vartree.py
+++ b/pym/portage/dbapi/vartree.py
@@ -2490,7 +2490,7 @@ class dblink(object):
 								(statobj.st_dev, statobj.st_ino),
 								[]).append(relative_path)
 
-					if is_owned:
+					if is_owned and not self._is_install_masked(relative_path[1:]):
 						show_unmerge("---", unmerge_desc["replaced"], file_type, obj)
 						continue
 					elif relative_path in cfgfiledict:
@@ -3687,6 +3687,24 @@ class dblink(object):
 	def _emerge_log(self, msg):
 		emergelog(False, msg)
 
+	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
+
 	def treewalk(self, srcroot, destroot, inforoot, myebuild, cleanup=0,
 		mydbapi=None, prev_mtimes=None, counter=None):
 		"""
@@ -3846,16 +3864,6 @@ class dblink(object):
 					max_dblnk = dblnk
 			self._installed_instance = max_dblnk
 
-		# Apply INSTALL_MASK before collision-protect, since it may
-		# be useful to avoid collisions in some scenarios.
-		# We cannot detect if this is needed or not here as INSTALL_MASK can be
-		# modified by bashrc files.
-		phase = MiscFunctionsProcess(background=False,
-			commands=["preinst_mask"], phase="preinst",
-			scheduler=self._scheduler, settings=self.settings)
-		phase.start()
-		phase.wait()
-
 		# We check for unicode encoding issues after src_install. However,
 		# the check must be repeated here for binary packages (it's
 		# inexpensive since we call os.walk() here anyway).
@@ -3927,6 +3935,10 @@ class dblink(object):
 
 					relative_path = fpath[srcroot_len:]
 
+					# filter on INSTALL_MASK
+					if self._is_install_masked(relative_path):
+						continue
+
 					if line_ending_re.search(relative_path) is not None:
 						paths_with_newlines.append(relative_path)
 
@@ -4642,6 +4654,7 @@ class dblink(object):
 		while mergelist:
 
 			relative_path = mergelist.pop()
+			instmasked = self._is_install_masked(relative_path)
 			mysrc = join(srcroot, relative_path)
 			mydest = join(destroot, relative_path)
 			# myrealdest is mydest without the $ROOT prefix (makes a difference if ROOT!="/")
@@ -4730,7 +4743,7 @@ class dblink(object):
 				destmd5 = None
 
 			moveme = True
-			if protected:
+			if protected and not instmasked:
 				mydest, protected, moveme = self._protect(cfgfiledict,
 					protect_if_modified, mymd5, myto, mydest,
 					myrealdest, mydmode, destmd5, mydest_link)
@@ -4758,7 +4771,7 @@ class dblink(object):
 				# we can simply test for existence of this file to see if the target has been merged yet
 				myrealto = normalize_path(os.path.join(destroot, myabsto))
 				if mydmode is not None and stat.S_ISDIR(mydmode):
-					if not protected:
+					if not protected and not instmasked:
 						# we can't merge a symlink over a directory
 						newdest = self._new_backup_path(mydest)
 						msg = []
@@ -4778,26 +4791,32 @@ class dblink(object):
 					# it later.
 					secondhand.append(mysrc[len(srcroot):])
 					continue
-				# unlinking no longer necessary; "movefile" will overwrite symlinks atomically and correctly
-				if moveme:
-					zing = ">>>"
-					mymtime = movefile(mysrc, mydest, newmtime=thismtime,
-						sstat=mystat, mysettings=self.settings,
-						encoding=_encodings['merge'])
 
-				try:
-					self._merged_path(mydest, os.lstat(mydest))
-				except OSError:
-					pass
+				if instmasked:
+					zing = "###"
+					# pass mymtime through from initial stat
+				else:
+					# unlinking no longer necessary; "movefile" will overwrite symlinks atomically and correctly
+					if moveme:
+						zing = ">>>"
+						mymtime = movefile(mysrc, mydest, newmtime=thismtime,
+							sstat=mystat, mysettings=self.settings,
+							encoding=_encodings['merge'])
+
+					try:
+						self._merged_path(mydest, os.lstat(mydest))
+					except OSError:
+						pass
 
 				if mymtime != None:
-					# Use lexists, since if the target happens to be a broken
-					# symlink then that should trigger an independent warning.
-					if not (os.path.lexists(myrealto) or
-						os.path.lexists(join(srcroot, myabsto))):
-						self._eqawarn('preinst',
-							[_("QA Notice: Symbolic link /%s points to /%s which does not exist.")
-							% (relative_path, myabsto)])
+					if not instmasked:
+						# Use lexists, since if the target happens to be a broken
+						# symlink then that should trigger an independent warning.
+						if not (os.path.lexists(myrealto) or
+							os.path.lexists(join(srcroot, myabsto))):
+							self._eqawarn('preinst',
+								[_("QA Notice: Symbolic link /%s points to /%s which does not exist.")
+								% (relative_path, myabsto)])
 
 					showMessage("%s %s -> %s\n" % (zing, mydest, myto))
 					if sys.hexversion >= 0x3030000:
@@ -4812,7 +4831,9 @@ class dblink(object):
 					return 1
 			elif stat.S_ISDIR(mymode):
 				# we are merging a directory
-				if mydmode != None:
+				if instmasked:
+					showMessage("### %s/\n" % mydest)
+				elif mydmode != None:
 					# destination exists
 
 					if bsd_chflags:
@@ -4899,10 +4920,11 @@ class dblink(object):
 					os.chown(mydest, mystat[4], mystat[5])
 					showMessage(">>> %s/\n" % mydest)
 
-				try:
-					self._merged_path(mydest, os.lstat(mydest))
-				except OSError:
-					pass
+				if not instmasked:
+					try:
+						self._merged_path(mydest, os.lstat(mydest))
+					except OSError:
+						pass
 
 				outfile.write("dir "+myrealdest+"\n")
 				# recurse and merge this directory
@@ -4911,7 +4933,7 @@ class dblink(object):
 
 			elif stat.S_ISREG(mymode):
 				# we are merging a regular file
-				if not protected and \
+				if not protected and not instmasked and \
 					mydmode is not None and stat.S_ISDIR(mydmode):
 						# install of destination is blocked by an existing directory with the same name
 						newdest = self._new_backup_path(mydest)
@@ -4925,9 +4947,11 @@ class dblink(object):
 						self._eerror("preinst", msg)
 						mydest = newdest
 
+				if instmasked:
+					zing = "###"
 				# whether config protection or not, we merge the new file the
 				# same way.  Unless moveme=0 (blocking directory)
-				if moveme:
+				elif moveme:
 					# Create hardlinks only for source files that already exist
 					# as hardlinks (having identical st_dev and st_ino).
 					hardlink_key = (mystat.st_dev, mystat.st_ino)
@@ -4960,7 +4984,9 @@ class dblink(object):
 			else:
 				# we are merging a fifo or device node
 				zing = "!!!"
-				if mydmode is None:
+				if instmasked:
+					zing = "###"
+				elif mydmode is None:
 					# destination doesn't exist
 					if movefile(mysrc, mydest, newmtime=thismtime,
 						sstat=mystat, mysettings=self.settings,
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:
-- 
2.8.3

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

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

[Michał Górny]

Dnia 12 czerwca 2016 11:10:55 CEST, Zac Medico <zmedico@gentoo.org> 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 06/12/2016 02:28 AM, Michał Górny wrote:
> Dnia 12 czerwca 2016 11:10:55 CEST, Zac Medico <zmedico@gentoo.org> 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:49:26 CEST, Zac Medico <zmedico@gentoo.org> napisał(a):
>On 06/12/2016 02:28 AM, Michał Górny wrote:
>> Dnia 12 czerwca 2016 11:10:55 CEST, Zac Medico <zmedico@gentoo.org>
>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 03:05 AM, Michał Górny wrote:
> Dnia 12 czerwca 2016 11:49:26 CEST, Zac Medico <zmedico@gentoo.org> napisał(a):
>> On 06/12/2016 02:28 AM, Michał Górny wrote:
>>> Dnia 12 czerwca 2016 11:10:55 CEST, Zac Medico <zmedico@gentoo.org>
>> 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

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

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

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

[gentoo-portage-dev] [PATCH 3/3] portage.dbapi.vartree: Support exclusions in INSTALL_MASK

[Michał Górny]

Allow INSTALL_MASK patterns to start with '-' to indicate that
a specific match is to be excluded from being masked. In this case,
the last matching pattern determines whether the file is actually
filtered out or kept.
---
 pym/portage/dbapi/vartree.py | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/pym/portage/dbapi/vartree.py b/pym/portage/dbapi/vartree.py
index 8e5ac43..d02d850 100644
--- a/pym/portage/dbapi/vartree.py
+++ b/pym/portage/dbapi/vartree.py
@@ -3690,19 +3690,21 @@ class dblink(object):
 	def _is_install_masked(self, relative_path):
 		ret = False
 		for pattern in self.settings.install_mask:
+			# if pattern starts with -, possibly exclude this path
+			pat_res = not pattern.startswith('-')
+			if not pat_res:
+				pattern = pattern[1:]
 			# 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
+					ret = pat_res
 			# filename
 			else:
 				if fnmatch.fnmatch(os.path.basename(relative_path), pattern):
-					ret = True
-					break
+					ret = pat_res
 		return ret
 
 	def treewalk(self, srcroot, destroot, inforoot, myebuild, cleanup=0,
-- 
2.8.3

Re: [gentoo-portage-dev] [PATCH 3/3] portage.dbapi.vartree: Support exclusions in INSTALL_MASK

[Zac Medico]

On 05/21/2016 11:56 PM, Michał Górny wrote:
> Allow INSTALL_MASK patterns to start with '-' to indicate that
> a specific match is to be excluded from being masked. In this case,
> the last matching pattern determines whether the file is actually
> filtered out or kept.
> ---
>  pym/portage/dbapi/vartree.py | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/pym/portage/dbapi/vartree.py b/pym/portage/dbapi/vartree.py
> index 8e5ac43..d02d850 100644
> --- a/pym/portage/dbapi/vartree.py
> +++ b/pym/portage/dbapi/vartree.py
> @@ -3690,19 +3690,21 @@ class dblink(object):
>  	def _is_install_masked(self, relative_path):
>  		ret = False
>  		for pattern in self.settings.install_mask:
> +			# if pattern starts with -, possibly exclude this path
> +			pat_res = not pattern.startswith('-')
> +			if not pat_res:
> +				pattern = pattern[1:]
>  			# 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
> +					ret = pat_res
>  			# filename
>  			else:
>  				if fnmatch.fnmatch(os.path.basename(relative_path), pattern):
> -					ret = True
> -					break
> +					ret = pat_res
>  		return ret
>  
>  	def treewalk(self, srcroot, destroot, inforoot, myebuild, cleanup=0,
> 

In order to implement this with pre-compiled patterns, we can use a
separate regular expression to represent all of the exclusions.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH 3/3] portage.dbapi.vartree: Support exclusions in INSTALL_MASK

[Michał Górny]

Dnia 12 czerwca 2016 11:20:36 CEST, Zac Medico <zmedico@gentoo.org> napisał(a):
>On 05/21/2016 11:56 PM, Michał Górny wrote:
>> Allow INSTALL_MASK patterns to start with '-' to indicate that
>> a specific match is to be excluded from being masked. In this case,
>> the last matching pattern determines whether the file is actually
>> filtered out or kept.
>> ---
>>  pym/portage/dbapi/vartree.py | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>> 
>> diff --git a/pym/portage/dbapi/vartree.py
>b/pym/portage/dbapi/vartree.py
>> index 8e5ac43..d02d850 100644
>> --- a/pym/portage/dbapi/vartree.py
>> +++ b/pym/portage/dbapi/vartree.py
>> @@ -3690,19 +3690,21 @@ class dblink(object):
>>  	def _is_install_masked(self, relative_path):
>>  		ret = False
>>  		for pattern in self.settings.install_mask:
>> +			# if pattern starts with -, possibly exclude this path
>> +			pat_res = not pattern.startswith('-')
>> +			if not pat_res:
>> +				pattern = pattern[1:]
>>  			# 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
>> +					ret = pat_res
>>  			# filename
>>  			else:
>>  				if fnmatch.fnmatch(os.path.basename(relative_path), pattern):
>> -					ret = True
>> -					break
>> +					ret = pat_res
>>  		return ret
>>  
>>  	def treewalk(self, srcroot, destroot, inforoot, myebuild,
>cleanup=0,
>> 
>
>In order to implement this with pre-compiled patterns, we can use a
>separate regular expression to represent all of the exclusions.

That won't work since exclusive and inclusive patterns can be stacked, and for this to work they are applied in order.

E.g. /usr/foo -/usr/foo/bar /usr/foo/bar/baz

Should remove all foo subdirectories except for bar, and also baz inside bar.


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

Re: [gentoo-portage-dev] [PATCH 3/3] portage.dbapi.vartree: Support exclusions in INSTALL_MASK

[Zac Medico]

On 06/12/2016 02:31 AM, Michał Górny wrote:
> Dnia 12 czerwca 2016 11:20:36 CEST, Zac Medico <zmedico@gentoo.org> napisał(a):
>> On 05/21/2016 11:56 PM, Michał Górny wrote:
>>> Allow INSTALL_MASK patterns to start with '-' to indicate that
>>> a specific match is to be excluded from being masked. In this case,
>>> the last matching pattern determines whether the file is actually
>>> filtered out or kept.
>>> ---
>>>  pym/portage/dbapi/vartree.py | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/pym/portage/dbapi/vartree.py
>> b/pym/portage/dbapi/vartree.py
>>> index 8e5ac43..d02d850 100644
>>> --- a/pym/portage/dbapi/vartree.py
>>> +++ b/pym/portage/dbapi/vartree.py
>>> @@ -3690,19 +3690,21 @@ class dblink(object):
>>>  	def _is_install_masked(self, relative_path):
>>>  		ret = False
>>>  		for pattern in self.settings.install_mask:
>>> +			# if pattern starts with -, possibly exclude this path
>>> +			pat_res = not pattern.startswith('-')
>>> +			if not pat_res:
>>> +				pattern = pattern[1:]
>>>  			# 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
>>> +					ret = pat_res
>>>  			# filename
>>>  			else:
>>>  				if fnmatch.fnmatch(os.path.basename(relative_path), pattern):
>>> -					ret = True
>>> -					break
>>> +					ret = pat_res
>>>  		return ret
>>>  
>>>  	def treewalk(self, srcroot, destroot, inforoot, myebuild,
>> cleanup=0,
>>>
>>
>> In order to implement this with pre-compiled patterns, we can use a
>> separate regular expression to represent all of the exclusions.
> 
> That won't work since exclusive and inclusive patterns can be stacked, and for this to work they are applied in order.
> 
> E.g. /usr/foo -/usr/foo/bar /usr/foo/bar/baz
> 
> Should remove all foo subdirectories except for bar, and also baz inside bar.

Hmm, I suppose there could be an optimized implementation to handle
INSTALL_MASK settings where there are no such ordering constraints.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH 3/3] portage.dbapi.vartree: Support exclusions in INSTALL_MASK

[Michał Górny]

Dnia 12 czerwca 2016 11:43:35 CEST, Zac Medico <zmedico@gentoo.org> napisał(a):
>On 06/12/2016 02:31 AM, Michał Górny wrote:
>> Dnia 12 czerwca 2016 11:20:36 CEST, Zac Medico <zmedico@gentoo.org>
>napisał(a):
>>> On 05/21/2016 11:56 PM, Michał Górny wrote:
>>>> Allow INSTALL_MASK patterns to start with '-' to indicate that
>>>> a specific match is to be excluded from being masked. In this case,
>>>> the last matching pattern determines whether the file is actually
>>>> filtered out or kept.
>>>> ---
>>>>  pym/portage/dbapi/vartree.py | 10 ++++++----
>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/pym/portage/dbapi/vartree.py
>>> b/pym/portage/dbapi/vartree.py
>>>> index 8e5ac43..d02d850 100644
>>>> --- a/pym/portage/dbapi/vartree.py
>>>> +++ b/pym/portage/dbapi/vartree.py
>>>> @@ -3690,19 +3690,21 @@ class dblink(object):
>>>>  	def _is_install_masked(self, relative_path):
>>>>  		ret = False
>>>>  		for pattern in self.settings.install_mask:
>>>> +			# if pattern starts with -, possibly exclude this path
>>>> +			pat_res = not pattern.startswith('-')
>>>> +			if not pat_res:
>>>> +				pattern = pattern[1:]
>>>>  			# 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
>>>> +					ret = pat_res
>>>>  			# filename
>>>>  			else:
>>>>  				if fnmatch.fnmatch(os.path.basename(relative_path), pattern):
>>>> -					ret = True
>>>> -					break
>>>> +					ret = pat_res
>>>>  		return ret
>>>>  
>>>>  	def treewalk(self, srcroot, destroot, inforoot, myebuild,
>>> cleanup=0,
>>>>
>>>
>>> In order to implement this with pre-compiled patterns, we can use a
>>> separate regular expression to represent all of the exclusions.
>> 
>> That won't work since exclusive and inclusive patterns can be
>stacked, and for this to work they are applied in order.
>> 
>> E.g. /usr/foo -/usr/foo/bar /usr/foo/bar/baz
>> 
>> Should remove all foo subdirectories except for bar, and also baz
>inside bar.
>
>Hmm, I suppose there could be an optimized implementation to handle
>INSTALL_MASK settings where there are no such ordering constraints.

Is this really worth the effort? I'm using the patch for a while and I/O's the bottleneck, not CPU. If you really insist on optimizing this, i guess we could precompile all patterns separately.


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

Re: [gentoo-portage-dev] [PATCH 3/3] portage.dbapi.vartree: Support exclusions in INSTALL_MASK

[Zac Medico]

On 06/12/2016 03:03 AM, Michał Górny wrote:
> Dnia 12 czerwca 2016 11:43:35 CEST, Zac Medico <zmedico@gentoo.org> napisał(a):
>> On 06/12/2016 02:31 AM, Michał Górny wrote:
>>> Dnia 12 czerwca 2016 11:20:36 CEST, Zac Medico <zmedico@gentoo.org>
>> napisał(a):
>>>> On 05/21/2016 11:56 PM, Michał Górny wrote:
>>>>> Allow INSTALL_MASK patterns to start with '-' to indicate that
>>>>> a specific match is to be excluded from being masked. In this case,
>>>>> the last matching pattern determines whether the file is actually
>>>>> filtered out or kept.
>>>>> ---
>>>>>  pym/portage/dbapi/vartree.py | 10 ++++++----
>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/pym/portage/dbapi/vartree.py
>>>> b/pym/portage/dbapi/vartree.py
>>>>> index 8e5ac43..d02d850 100644
>>>>> --- a/pym/portage/dbapi/vartree.py
>>>>> +++ b/pym/portage/dbapi/vartree.py
>>>>> @@ -3690,19 +3690,21 @@ class dblink(object):
>>>>>  	def _is_install_masked(self, relative_path):
>>>>>  		ret = False
>>>>>  		for pattern in self.settings.install_mask:
>>>>> +			# if pattern starts with -, possibly exclude this path
>>>>> +			pat_res = not pattern.startswith('-')
>>>>> +			if not pat_res:
>>>>> +				pattern = pattern[1:]
>>>>>  			# 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
>>>>> +					ret = pat_res
>>>>>  			# filename
>>>>>  			else:
>>>>>  				if fnmatch.fnmatch(os.path.basename(relative_path), pattern):
>>>>> -					ret = True
>>>>> -					break
>>>>> +					ret = pat_res
>>>>>  		return ret
>>>>>  
>>>>>  	def treewalk(self, srcroot, destroot, inforoot, myebuild,
>>>> cleanup=0,
>>>>>
>>>>
>>>> In order to implement this with pre-compiled patterns, we can use a
>>>> separate regular expression to represent all of the exclusions.
>>>
>>> That won't work since exclusive and inclusive patterns can be
>> stacked, and for this to work they are applied in order.
>>>
>>> E.g. /usr/foo -/usr/foo/bar /usr/foo/bar/baz
>>>
>>> Should remove all foo subdirectories except for bar, and also baz
>> inside bar.
>>
>> Hmm, I suppose there could be an optimized implementation to handle
>> INSTALL_MASK settings where there are no such ordering constraints.
> 
> Is this really worth the effort? I'm using the patch for a while and I/O's the bottleneck, not CPU. If you really insist on optimizing this, i guess we could precompile all patterns separately.

Since it's not going to make any difference when INSTALL_MASK is not
used, I guess it's fine to leave it unoptimized for now.
-- 
Thanks,
Zac

[gentoo-portage-dev] [PATCH 4/3] portage.package.ebuild.config: Support path groups from install-mask.conf

[Michał Górny]

---
 .../package/ebuild/_config/InstallMaskManager.py   | 59 ++++++++++++++++++++++
 pym/portage/package/ebuild/config.py               | 34 ++++++++++++-
 pym/portage/util/configparser.py                   | 19 ++++++-
 3 files changed, 110 insertions(+), 2 deletions(-)
 create mode 100644 pym/portage/package/ebuild/_config/InstallMaskManager.py

diff --git a/pym/portage/package/ebuild/_config/InstallMaskManager.py b/pym/portage/package/ebuild/_config/InstallMaskManager.py
new file mode 100644
index 0000000..96cb539
--- /dev/null
+++ b/pym/portage/package/ebuild/_config/InstallMaskManager.py
@@ -0,0 +1,59 @@
+# Copyright 2010-2016 Gentoo Foundation
+# Distributed under the terms of the GNU General Public License v2
+
+__all__ = (
+	'InstallMaskManager',
+)
+
+import sys
+
+from portage import os
+from portage.localization import _
+from portage.util import writemsg
+from portage.util.configparser import (SafeConfigParser, ConfigParserError,
+	MultiValueConfigParserDict, read_configs)
+
+
+class InstallMaskManager(object):
+	def __init__(self, repositories, abs_user_config, user_config=True):
+		self._groups = {}
+
+		# read repository defined groups
+		self._read_config_from_repositories(repositories)
+
+		if user_config:
+			self._read_config(os.path.join(abs_user_config, 'install-mask.conf'), True)
+
+	def _read_config_from_repositories(self, repositories):
+		for r in repositories.repos_with_profiles():
+			self._read_config(os.path.join(r.location, 'metadata', 'install-mask.conf'))
+
+	def _read_config(self, path, is_user_config=False):
+		# use separate parsers to detect collisions properly
+		cfp_kwargs = {}
+		if sys.hexversion >= 0x03020000:
+			cfp_kwargs['strict'] = False
+		parser = SafeConfigParser(dict_type=MultiValueConfigParserDict,
+			**cfp_kwargs)
+		try:
+			read_configs(parser, [path])
+		except ConfigParserError as e:
+			writemsg(
+				_("!!! Error while reading %s: %s\n") % (path, e),
+				noiselevel=-1)
+			return
+
+		for sname in parser.sections():
+			if not is_user_config and sname in self._groups:
+				writemsg(
+					_("!!! Error while reading %s: duplicate group %s found\n") % (path, sname),
+					noiselevel=-1)
+				continue
+			if not parser.has_option(sname, 'path'):
+				continue
+
+			paths = parser.get(sname, 'path').split('\n')
+			self._groups[sname] = paths
+
+	def expand_group(self, gname):
+		return self._groups[gname]
diff --git a/pym/portage/package/ebuild/config.py b/pym/portage/package/ebuild/config.py
index 9d13703..dfbd7f2 100644
--- a/pym/portage/package/ebuild/config.py
+++ b/pym/portage/package/ebuild/config.py
@@ -60,6 +60,7 @@ from portage.package.ebuild._config.features_set import features_set
 from portage.package.ebuild._config.KeywordsManager import KeywordsManager
 from portage.package.ebuild._config.LicenseManager import LicenseManager
 from portage.package.ebuild._config.UseManager import UseManager
+from portage.package.ebuild._config.InstallMaskManager import InstallMaskManager
 from portage.package.ebuild._config.LocationsManager import LocationsManager
 from portage.package.ebuild._config.MaskManager import MaskManager
 from portage.package.ebuild._config.VirtualsManager import VirtualsManager
@@ -277,6 +278,7 @@ class config(object):
 			# force instantiation of lazy immutable objects when cloning, so
 			# that they're not instantiated more than once
 			self._keywords_manager_obj = clone._keywords_manager
+			self._install_mask_manager_obj = clone._install_mask_manager
 			self._mask_manager_obj = clone._mask_manager
 
 			# shared mutable attributes
@@ -329,6 +331,7 @@ class config(object):
 		else:
 			# lazily instantiated objects
 			self._keywords_manager_obj = None
+			self._install_mask_manager_obj = None
 			self._mask_manager_obj = None
 			self._virtuals_manager_obj = None
 
@@ -1032,6 +1035,15 @@ class config(object):
 		return self._keywords_manager_obj
 
 	@property
+	def _install_mask_manager(self):
+		if self._install_mask_manager_obj is None:
+			self._install_mask_manager_obj = InstallMaskManager(
+				self.repositories,
+				self._locations_manager.abs_user_config,
+				user_config=self.local_config)
+		return self._install_mask_manager_obj
+
+	@property
 	def _mask_manager(self):
 		if self._mask_manager_obj is None:
 			self._mask_manager_obj = MaskManager(self.repositories,
@@ -1774,7 +1786,8 @@ class config(object):
 		_eapi_cache.clear()
 
 		# Prepare the final value of INSTALL_MASK
-		install_mask = self.get("INSTALL_MASK", "").split()
+		install_mask = list(self._replace_install_mask_groups(
+			self.get("INSTALL_MASK", "").split()))
 		if 'nodoc' in self.features:
 			install_mask.append("/usr/share/doc")
 		if 'noinfo' in self.features:
@@ -1782,6 +1795,25 @@ class config(object):
 		if 'noman' in self.features:
 			install_mask.append("/usr/share/man")
 		self.install_mask = install_mask
+		print(install_mask)
+
+	def _replace_install_mask_groups(self, vals):
+		for v in vals:
+			if v.startswith('@') or v.startswith('-@'):
+				neg = '-' if v.startswith('-') else ''
+				gname = v[2:] if neg else v[1:]
+				for p in self._expand_install_mask_group(gname):
+					yield '%s%s' % (neg, p)
+			else:
+				yield v
+
+	def _expand_install_mask_group(self, gname):
+		try:
+			return self._install_mask_manager.expand_group(gname)
+		except KeyError:
+			writemsg(_("!!! Undefined INSTALL_MASK group: '%s'!\n")
+				% gname, noiselevel=-1)
+			return ()
 
 	def _grab_pkg_env(self, penv, container, protected_keys=None):
 		if protected_keys is None:
diff --git a/pym/portage/util/configparser.py b/pym/portage/util/configparser.py
index c4c92a6..290bc5e 100644
--- a/pym/portage/util/configparser.py
+++ b/pym/portage/util/configparser.py
@@ -2,7 +2,8 @@
 # Distributed under the terms of the GNU General Public License v2
 
 __all__ = ['ConfigParserError', 'NoOptionError', 'ParsingError',
-	'RawConfigParser', 'SafeConfigParser', 'read_configs']
+	'RawConfigParser', 'SafeConfigParser', 'read_configs',
+	'MultiValueConfigParserDict']
 
 # the following scary compatibility thing provides two classes:
 # - SafeConfigParser that provides safe interpolation for values,
@@ -74,3 +75,19 @@ def read_configs(parser, paths):
 			read_file(p, **kwargs)
 		else:
 			raise TypeError("Unsupported type %r of element %r of 'paths' argument" % (type(p), p))
+
+
+class MultiValueConfigParserDict(dict):
+	"""
+	A special variant of dict that stores all subsequent values assigned
+	to its keys as a list, and returns this list when retrieved. Meant
+	to be used with ConfigParser to process multi-key config files.
+	"""
+
+	def __setitem__(self, k, v):
+		if isinstance(v, list):
+			if not k in self:
+				super(MultiValueConfigParserDict, self).__setitem__(k, [])
+			self[k].extend(v)
+		else:
+			super(MultiValueConfigParserDict, self).__setitem__(k, v)
-- 
2.8.3

Re: [gentoo-portage-dev] [PATCH 4/3] portage.package.ebuild.config: Support path groups from install-mask.conf

[Michał Górny]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

On Tue, 31 May 2016 17:58:34 +0200
Michał Górny <mgorny@gentoo.org> wrote:

> ---
>  .../package/ebuild/_config/InstallMaskManager.py   | 59 ++++++++++++++++++++++
>  pym/portage/package/ebuild/config.py               | 34 ++++++++++++-
>  pym/portage/util/configparser.py                   | 19 ++++++-
>  3 files changed, 110 insertions(+), 2 deletions(-)
>  create mode 100644 pym/portage/package/ebuild/_config/InstallMaskManager.py

Please disregard this one. After discussion with ulm, the spec is
changing and I will be providing an updated patch later. However,
the remaining three still apply.

-- 
Best regards,
Michał Górny
<http://dev.gentoo.org/~mgorny/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 949 bytes --]