Re: [gentoo-portage-dev] [PATCH] FEATURES=case-insensitive-fs for bug #524236

2014-11-13 Thread Alexander Berntsen
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 13/11/14 02:22, Zac Medico wrote:
 +if case-insensitive-fs in portage.settings.features:
 +FIND_EXTANT_CONFIGS = \
 +FIND_EXTANT_CONFIGS.replace(-name '._cfg, -iname '._cfg)
 +
Splitting inside the replace will look nicer following PEP indentation
(as you won't need the '\').

 +Use case\-insensitive file name comparisions when merging and unmerging
 +files.
 +.TP
Maybe mention a) that most people can ignore this option, and b) who
it's actually for. Kind of in the kernel option help style.


In general I don't like this patch. It handles a bunch of cases separately by 
doing lower(), when I think instead it should be handled implicitly. The data 
should be in a structure such that it knows whether it is supposed to be upper 
or lowercase, and whatever's dealing with it should deal with it accordingly, 
rather than checking is this case insensitive? OK lowercase it before sending 
it wherever.

But if you think this is the best way, I'm not going to stand in the way of 
this patch.
- -- 
Alexander
berna...@gentoo.org
https://secure.plaimi.net/~alexander
-BEGIN PGP SIGNATURE-
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iF4EAREIAAYFAlRkiB0ACgkQRtClrXBQc7UudAD/V1r4AR5zA54Xz+LHBVNt0bnn
uQ9w+146L8WYK6pDN9gBAJxZbQREeOwxKSDjluZ1lq9XARf1rh/Eqzl58wIc8I4K
=c1Em
-END PGP SIGNATURE-



Re: [gentoo-portage-dev] [PATCH] FEATURES=case-insensitive-fs for bug #524236

2014-11-13 Thread Zac Medico
On 11/13/2014 02:29 AM, Alexander Berntsen wrote:
 On 13/11/14 02:22, Zac Medico wrote:
 +if case-insensitive-fs in portage.settings.features:
 +FIND_EXTANT_CONFIGS = \
 +FIND_EXTANT_CONFIGS.replace(-name '._cfg, -iname '._cfg)
 +
 Splitting inside the replace will look nicer following PEP indentation
 (as you won't need the '\').

Okay, I'll re-format it as you've suggested.

 +Use case\-insensitive file name comparisions when merging and unmerging
 +files.
 +.TP
 Maybe mention a) that most people can ignore this option, and b) who
 it's actually for. Kind of in the kernel option help style.

Okay, I'll add something about it only being needed for case-insensitive
file systems, which are usually not used.

 
 In general I don't like this patch. It handles a bunch of cases separately
 by doing lower(), when I think instead it should be handled implicitly.
 The data should be in a structure such that it knows whether it is supposed
 to be upper or lowercase, and whatever's dealing with it should deal with
 it accordingly, rather than checking is this case insensitive? OK
 lowercase it before sending it wherever.

Aside from the ConfigProtect constructor, which has a new
case_insensitive keyword parameter, all affected methods handle case
transformations implicitly, as far as API consumers are concerned.

However, we could improve efficiency for some usage patterns by
providing an alternative to dblink.getcontents that is oriented toward
case-insensitive handling. For example, every single
dblink._match_contents call currently has to transform all names to
lowercase, and generate a reverse mapping from lowercase back to
preserved case. The dblink._match_contents method would be more
efficient if we created an alternative to dblink.getcontents that
handled the transformations and cached the results. I intentionally did
not implement this optimization yet, since it's probably better to do it
in a separate patch, rather than complicate the current patch.

 But if you think this is the best way, I'm not going to stand in the way of 
 this patch.

As discussed above, think the current approach is pretty reasonable,
though I would like to optimize it with a separate patch.
-- 
Thanks,
Zac



[gentoo-portage-dev] [PATCH] fs_template._ensure_dirs: handle EEXIST (529120)

2014-11-13 Thread Zac Medico
There was a race inside fs_template._ensure_dirs which could cause it to
raise EEXIST if a concurrent process created the directory after
os.path.exists returned False. Fix it by using the util.ensure_dirs
function, which already handles EEXIST.

X-Gentoo-Bug: 529120
X-Gentoo-Bug-URL: https://bugs.gentoo.org/show_bug.cgi?id=529120
---
 pym/portage/cache/fs_template.py | 25 ++---
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/pym/portage/cache/fs_template.py b/pym/portage/cache/fs_template.py
index de4fe4b..fa44abc 100644
--- a/pym/portage/cache/fs_template.py
+++ b/pym/portage/cache/fs_template.py
@@ -10,7 +10,7 @@ from portage import os
 from portage.proxy.lazyimport import lazyimport
 lazyimport(globals(),
'portage.exception:PortageException',
-   'portage.util:apply_permissions',
+   'portage.util:apply_permissions,ensure_dirs',
 )
 del lazyimport
 
@@ -61,20 +61,15 @@ class FsBased(template.database):
 
for dir in 
path.lstrip(os.path.sep).rstrip(os.path.sep).split(os.path.sep):
base = os.path.join(base,dir)
-   if not os.path.exists(base):
-   if self._perms != -1:
-   um = os.umask(0)
-   try:
-   perms = self._perms
-   if perms == -1:
-   perms = 0
-   perms |= 0o755
-   os.mkdir(base, perms)
-   if self._gid != -1:
-   os.chown(base, -1, self._gid)
-   finally:
-   if self._perms != -1:
-   os.umask(um)
+   if ensure_dirs(base):
+   # We only call apply_permissions if ensure_dirs 
created
+   # a new directory, so as not to interfere with
+   # permissions of existing directories.
+   mode = self._perms
+   if mode == -1:
+   mode = 0
+   mode |= 0o755
+   apply_permissions(base, mode=mode, 
gid=self._gid)
 
def _prune_empty_dirs(self):
all_dirs = []
-- 
2.0.4




[gentoo-portage-dev] [PATCH] portageq: fix eroot parameter (bug #529200)

2014-11-13 Thread Zac Medico
The portageq eroot parameter has been broken since commit
c9f6aa9f0151adb3c86706eaef1914cdbdcf2b6d, due to premature instantiation
of portage.settings (before the ROOT variable was set). Premature access
to the portage.settings attribute must be avoided by using other
available means to determine the EPREFIX.

Fixes: c9f6aa9f0151 (Add cross-prefix support)
X-Gentoo-Bug: 529200
X-Gentoo-Bug-URL: https://bugs.gentoo.org/show_bug.cgi?id=529200
---
 bin/portageq|  9 -
 pym/portage/tests/emerge/test_simple.py | 10 --
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/bin/portageq b/bin/portageq
index 009f116..ef565d1 100755
--- a/bin/portageq
+++ b/bin/portageq
@@ -1392,7 +1392,14 @@ def main(argv):
sys.stderr.write(Run portageq with --help for info\n)
sys.stderr.flush()
sys.exit(os.EX_USAGE)
-   eprefix = portage.settings[EPREFIX]
+   # Calculate EPREFIX and ROOT that will be used to construct
+   # portage.settings later. It's tempting to use
+   # portage.settings[EPREFIX] here, but that would force
+   # instantiation of portage.settings, which we don't want to do
+   # until after we've calculated ROOT (see bug #529200).
+   eprefix = os.environ.get(EPREFIX, portage.const.EPREFIX)
+   if eprefix:
+   eprefix = portage.util.normalize_path(eprefix)
eroot = portage.util.normalize_path(argv[2])
 
if eprefix:
diff --git a/pym/portage/tests/emerge/test_simple.py 
b/pym/portage/tests/emerge/test_simple.py
index 6c20a07..0101362 100644
--- a/pym/portage/tests/emerge/test_simple.py
+++ b/pym/portage/tests/emerge/test_simple.py
@@ -217,6 +217,8 @@ pkg_preinst() {
self.assertFalse(test_ebuild is None)
 
cross_prefix = os.path.join(eprefix, cross_prefix)
+   cross_root = os.path.join(eprefix, cross_root)
+   cross_eroot = os.path.join(cross_root, eprefix.lstrip(os.sep))
 
test_commands = (
env_update_cmd,
@@ -318,6 +320,10 @@ pkg_preinst() {
portageq_cmd + (has_version, cross_prefix, 
dev-libs/A),
({EPREFIX : cross_prefix},) + \
portageq_cmd + (has_version, cross_prefix, 
dev-libs/B),
+
+   # Test ROOT support
+   ({ROOT: cross_root},) + emerge_cmd + (dev-libs/B,),
+   portageq_cmd + (has_version, cross_eroot, 
dev-libs/B),
)
 
distdir = playground.distdir
@@ -372,8 +378,8 @@ pkg_preinst() {
os.environ[__PORTAGE_TEST_HARDLINK_LOCKS]
 
updates_dir = os.path.join(test_repo_location, profiles, 
updates)
-   dirs = [cachedir, cachedir_pregen, cross_prefix, distdir, 
fake_bin,
-   portage_tmpdir, updates_dir,
+   dirs = [cachedir, cachedir_pregen, cross_eroot, cross_prefix,
+   distdir, fake_bin, portage_tmpdir, updates_dir,
user_config_dir, var_cache_edb]
etc_symlinks = (dispatch-conf.conf, etc-update.conf)
# Override things that may be unavailable, or may have 
portability
-- 
2.0.4