Patchwork setup: Set mode 644 or 755 on installed files

login
register
mail settings
Submitter Kyle Lippincott
Date Oct. 1, 2014, 9 p.m.
Message ID <b168f18ef708e5c834eb.1412197244@dragonair.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/6070/
State Accepted
Headers show

Comments

Kyle Lippincott - Oct. 1, 2014, 9 p.m.
# HG changeset patch
# User Kyle Lippincott <spectral@google.com>
# Date 1412122434 25200
#      Tue Sep 30 17:13:54 2014 -0700
# Node ID b168f18ef708e5c834eb5ce9f6804c033e769082
# Parent  939ce500c92a3dcc0e10815242361ff70a6fcae9
setup: Set mode 644 or 755 on installed files
Mads Kiilerich - Oct. 2, 2014, 12:40 a.m.
On 10/01/2014 11:00 PM, Kyle Lippincott wrote:
> # HG changeset patch
> # User Kyle Lippincott <spectral@google.com>
> # Date 1412122434 25200
> #      Tue Sep 30 17:13:54 2014 -0700
> # Node ID b168f18ef708e5c834eb5ce9f6804c033e769082
> # Parent  939ce500c92a3dcc0e10815242361ff70a6fcae9
> setup: Set mode 644 or 755 on installed files

The Scripture says that the subject line should be all lower case.

A description that explains a bit about the context and the "why" would 
be nice. The docstring do however seem to explain most of it.

It is sad that such a minor change requires so much code. But distutils :-(

Couldn't we just set umask in this script or in Makefile ... or fail 
early if it is bad?

/Mads

> diff -r 939ce500c92a -r b168f18ef708 setup.py
> --- a/setup.py	Mon Sep 29 17:23:38 2014 -0500
> +++ b/setup.py	Tue Sep 30 17:13:54 2014 -0700

> @@ -375,6 +376,39 @@
>                                         libraries=[],
>                                         output_dir=self.build_temp)
>   
> +class hginstalllib(install_lib):
> +    '''
> +    This is a specialization of install_lib that replaces the copy_file used
> +    there so that it supports setting the mode of files after copying them,
> +    instead of just preserving the mode that the files originally had.  If your
> +    system has a umask of something like 027, preserving the permissions when
> +    copying will lead to a broken install.
> +
> +    Note that just passing keep_permissions=False to copy_file would be
> +    insufficient, as it might still be applying a umask.
> +    '''
> +
> +    def run(self):
> +        realcopyfile = file_util.copy_file
> +        def copyfileandsetmode(*args, **kwargs):
> +            src, dst = args[0], args[1]
> +            dst, copied = realcopyfile(*args, **kwargs)
> +            if copied:
> +                st = os.stat(src)
> +                # Persist executable bit (apply it to group and other if user
> +                # has it)
> +                if st[stat.ST_MODE] & stat.S_IXUSR:
> +                    setmode = 0755
> +                else:
> +                    setmode = 0644
> +                os.chmod(dst, (stat.S_IMODE(st[stat.ST_MODE]) & ~0777) |
> +                         setmode)
> +        file_util.copy_file = copyfileandsetmode
> +        try:
> +            install_lib.run(self)
> +        finally:
> +            file_util.copy_file = realcopyfile
> +
>
Kyle Lippincott - Oct. 2, 2014, 1:16 a.m.
On Wed, Oct 1, 2014 at 5:40 PM, Mads Kiilerich <mads@kiilerich.com> wrote:

> On 10/01/2014 11:00 PM, Kyle Lippincott wrote:
>
>> # HG changeset patch
>> # User Kyle Lippincott <spectral@google.com>
>> # Date 1412122434 25200
>> #      Tue Sep 30 17:13:54 2014 -0700
>> # Node ID b168f18ef708e5c834eb5ce9f6804c033e769082
>> # Parent  939ce500c92a3dcc0e10815242361ff70a6fcae9
>> setup: Set mode 644 or 755 on installed files
>>
>
> The Scripture says that the subject line should be all lower case.
>

Yeah, mpm mentioned it to me already; he said it could be fixed in flight,
which I assume meant I shouldn't send another patch with just that fixed.


>
> A description that explains a bit about the context and the "why" would be
> nice. The docstring do however seem to explain most of it.
>

This is my first patch, so perhaps a naive process question: should I send
another patch with the capitalization fix and an expanded description, or
is that something that I should expect whomever applies the patch to
correct?


> It is sad that such a minor change requires so much code. But distutils :-(
>

Yeah.  :(


>
> Couldn't we just set umask in this script or in Makefile ... or fail early
> if it is bad?


I think at that point it would be too late, because the files in my repo
will have already had the umask (as of the time of my 'hg clone') applied
to them.  distutils's copy_file defaults to keep_permissions (and
install_lib provides us no way to specify or change this), which means the
umask at the time of running setup.py is, afaict, 100% irrelevant.


>
> /Mads
>
>  diff -r 939ce500c92a -r b168f18ef708 setup.py
>> --- a/setup.py  Mon Sep 29 17:23:38 2014 -0500
>> +++ b/setup.py  Tue Sep 30 17:13:54 2014 -0700
>>
>
>  @@ -375,6 +376,39 @@
>>                                         libraries=[],
>>                                         output_dir=self.build_temp)
>>   +class hginstalllib(install_lib):
>> +    '''
>> +    This is a specialization of install_lib that replaces the copy_file
>> used
>> +    there so that it supports setting the mode of files after copying
>> them,
>> +    instead of just preserving the mode that the files originally had.
>> If your
>> +    system has a umask of something like 027, preserving the permissions
>> when
>> +    copying will lead to a broken install.
>> +
>> +    Note that just passing keep_permissions=False to copy_file would be
>> +    insufficient, as it might still be applying a umask.
>> +    '''
>> +
>> +    def run(self):
>> +        realcopyfile = file_util.copy_file
>> +        def copyfileandsetmode(*args, **kwargs):
>> +            src, dst = args[0], args[1]
>> +            dst, copied = realcopyfile(*args, **kwargs)
>> +            if copied:
>> +                st = os.stat(src)
>> +                # Persist executable bit (apply it to group and other if
>> user
>> +                # has it)
>> +                if st[stat.ST_MODE] & stat.S_IXUSR:
>> +                    setmode = 0755
>> +                else:
>> +                    setmode = 0644
>> +                os.chmod(dst, (stat.S_IMODE(st[stat.ST_MODE]) & ~0777) |
>> +                         setmode)
>> +        file_util.copy_file = copyfileandsetmode
>> +        try:
>> +            install_lib.run(self)
>> +        finally:
>> +            file_util.copy_file = realcopyfile
>> +
>>
>>
>
Matt Mackall - Oct. 2, 2014, 10:45 p.m.
On Wed, 2014-10-01 at 14:00 -0700, Kyle Lippincott wrote:
> # HG changeset patch
> # User Kyle Lippincott <spectral@google.com>
> # Date 1412122434 25200
> #      Tue Sep 30 17:13:54 2014 -0700
> # Node ID b168f18ef708e5c834eb5ce9f6804c033e769082
> # Parent  939ce500c92a3dcc0e10815242361ff70a6fcae9
> setup: Set mode 644 or 755 on installed files

This got queued. Congrats on your first Mercurial patch.

Patch

diff -r 939ce500c92a -r b168f18ef708 setup.py
--- a/setup.py	Mon Sep 29 17:23:38 2014 -0500
+++ b/setup.py	Tue Sep 30 17:13:54 2014 -0700
@@ -63,7 +63,7 @@ 
         raise SystemExit(
             "Couldn't import standard bz2 (incomplete Python install).")
 
-import os, subprocess, time
+import os, stat, subprocess, time
 import re
 import shutil
 import tempfile
@@ -73,9 +73,10 @@ 
 from distutils.command.build import build
 from distutils.command.build_ext import build_ext
 from distutils.command.build_py import build_py
+from distutils.command.install_lib import install_lib
 from distutils.command.install_scripts import install_scripts
 from distutils.spawn import spawn, find_executable
-from distutils import cygwinccompiler
+from distutils import cygwinccompiler, file_util
 from distutils.errors import CCompilerError, DistutilsExecError
 from distutils.sysconfig import get_python_inc, get_config_var
 from distutils.version import StrictVersion
@@ -375,6 +376,39 @@ 
                                       libraries=[],
                                       output_dir=self.build_temp)
 
+class hginstalllib(install_lib):
+    '''
+    This is a specialization of install_lib that replaces the copy_file used
+    there so that it supports setting the mode of files after copying them,
+    instead of just preserving the mode that the files originally had.  If your
+    system has a umask of something like 027, preserving the permissions when
+    copying will lead to a broken install.
+
+    Note that just passing keep_permissions=False to copy_file would be
+    insufficient, as it might still be applying a umask.
+    '''
+
+    def run(self):
+        realcopyfile = file_util.copy_file
+        def copyfileandsetmode(*args, **kwargs):
+            src, dst = args[0], args[1]
+            dst, copied = realcopyfile(*args, **kwargs)
+            if copied:
+                st = os.stat(src)
+                # Persist executable bit (apply it to group and other if user
+                # has it)
+                if st[stat.ST_MODE] & stat.S_IXUSR:
+                    setmode = 0755
+                else:
+                    setmode = 0644
+                os.chmod(dst, (stat.S_IMODE(st[stat.ST_MODE]) & ~0777) |
+                         setmode)
+        file_util.copy_file = copyfileandsetmode
+        try:
+            install_lib.run(self)
+        finally:
+            file_util.copy_file = realcopyfile
+
 class hginstallscripts(install_scripts):
     '''
     This is a specialization of install_scripts that replaces the @LIBDIR@ with
@@ -426,6 +460,7 @@ 
             'build_ext': hgbuildext,
             'build_py': hgbuildpy,
             'build_hgextindex': buildhgextindex,
+            'install_lib': hginstalllib,
             'install_scripts': hginstallscripts,
             'build_hgexe': buildhgexe,
             }