#147 Fedora 31: pathfix.py: add -k (keep) and -a (add) CLI options, fix all *.py files
Merged 4 years ago by churchyard. Opened 4 years ago by pkopkan.
rpms/ pkopkan/python3 f31  into  f31

@@ -0,0 +1,323 @@ 

+ commit b8f85fc797aeee09a50718f402e0c95e68467d50

+ Author: Patrik Kopkan <pkopkan@redhat.com>

+ Date:   Mon Nov 4 10:45:49 2019 +0100

+ 

+     commit 211987de0de73e2747e14fcfa217b20c1ec7d817

+     Author: Patrik Kopkan <pkopkan@redhat.com>

+     Date:   Fri Sep 6 15:09:43 2019 +0200

+     

+         bpo-37064: add -a to Tools/Scripts/pathfix.py

+         - this option enables to add single literal

+         flag to kept flags

+     

+     commit 3f43ceff186da09978d0aff257bb18b8ac7611f7

+     Author: Victor Stinner <vstinner@redhat.com>

+     Date:   Thu Sep 5 18:09:46 2019 +0200

+     

+         bpo-37064: Skip test_tools.test_pathfix if installed (GH-15705)

+     

+         If Python is installed, skip test_tools.test_pathfix test because

+         Tools/scripts/pathfix.py script is not installed.

+     

+     commit 50254ac4c179cb412e90682098c97db786143929

+     Author: PatrikKopkan <kopkanpatrik@gmail.com>

+     Date:   Thu Sep 5 16:54:54 2019 +0200

+     

+         bpo-37064: Add option -k to Tools/scripts/pathfix.py (GH-15548)

+     

+         Add flag -k to pathscript.py script: preserve shebang flags

+ 

+     From 2b7dc40b2af6578181808ba73c1533fc114e55df Mon Sep 17 00:00:00 2001

+     From: Ruediger Pluem <r.pluem@gmx.de>

+     Date: Fri, 11 Oct 2019 15:36:50 +0200

+     Subject: [PATCH] bpo-38347: find pathfix for Python scripts whose name contain

+      a '-' (GH-16536)

+     

+      pathfix.py: Assume all files that end on '.py' are Python scripts when working recursively.

+     ---

+      Lib/test/test_tools/test_pathfix.py           | 34 +++++++++++++++----

+      .../2019-10-02-09-48-42.bpo-38347.2Tq5D1.rst  |  1 +

+      Tools/scripts/pathfix.py                      |  5 +--

+      3 files changed, 30 insertions(+), 10 deletions(-)

+      create mode 100644 Misc/NEWS.d/next/Tools-Demos/2019-10-02-09-48-42.bpo-38347.2Tq5D1.rst

+ diff --git a/Lib/test/test_tools/test_pathfix.py b/Lib/test/test_tools/test_pathfix.py

+ new file mode 100644

+ index 0000000..ec36117

+ --- /dev/null

+ +++ b/Lib/test/test_tools/test_pathfix.py

+ @@ -0,0 +1,129 @@

+ +import os

+ +import subprocess

+ +import sys

+ +import unittest

+ +from test import support

+ +from test.test_tools import import_tool, scriptsdir, skip_if_missing

+ +

+ +

+ +# need Tools/script/ directory: skip if run on Python installed on the system

+ +skip_if_missing()

+ +

+ +

+ +class TestPathfixFunctional(unittest.TestCase):

+ +    script = os.path.join(scriptsdir, 'pathfix.py')

+ +

+ +    def setUp(self):

+ +        self.addCleanup(support.unlink, support.TESTFN)

+ +

+ +    def pathfix(self, shebang, pathfix_flags, exitcode=0, stdout='', stderr='',

+ +                directory=''):

+ +        if directory:

+ +            # bpo-38347: Test filename should contain lowercase, uppercase,

+ +            # "-", "_" and digits.

+ +            filename = os.path.join(directory, 'script-A_1.py')

+ +            pathfix_arg = directory

+ +        else:

+ +            filename = support.TESTFN

+ +            pathfix_arg = filename

+ +

+ +        with open(filename, 'w', encoding='utf8') as f:

+ +            f.write(f'{shebang}\n' + 'print("Hello world")\n')

+ +

+ +        proc = subprocess.run(

+ +            [sys.executable, self.script,

+ +             *pathfix_flags, '-n', pathfix_arg],

+ +            capture_output=True, text=1)

+ +

+ +        if stdout == '' and proc.returncode == 0:

+ +            stdout = f'{filename}: updating\n'

+ +        self.assertEqual(proc.returncode, exitcode, proc)

+ +        self.assertEqual(proc.stdout, stdout, proc)

+ +        self.assertEqual(proc.stderr, stderr, proc)

+ +

+ +        with open(filename, 'r', encoding='utf8') as f:

+ +            output = f.read()

+ +

+ +        lines = output.split('\n')

+ +        self.assertEqual(lines[1:], ['print("Hello world")', ''])

+ +        new_shebang = lines[0]

+ +

+ +        if proc.returncode != 0:

+ +            self.assertEqual(shebang, new_shebang)

+ +

+ +        return new_shebang

+ +

+ +    def test_recursive(self):

+ +        tmpdir = support.TESTFN + '.d'

+ +        self.addCleanup(support.rmtree, tmpdir)

+ +        os.mkdir(tmpdir)

+ +        expected_stderr = f"recursedown('{os.path.basename(tmpdir)}')\n"

+ +        self.assertEqual(

+ +            self.pathfix(

+ +                '#! /usr/bin/env python',

+ +                ['-i', '/usr/bin/python3'],

+ +                directory=tmpdir,

+ +                stderr=expected_stderr),

+ +            '#! /usr/bin/python3')

+ +

+ +    def test_pathfix(self):

+ +        self.assertEqual(

+ +            self.pathfix(

+ +                '#! /usr/bin/env python',

+ +                ['-i', '/usr/bin/python3']),

+ +            '#! /usr/bin/python3')

+ +        self.assertEqual(

+ +            self.pathfix(

+ +                '#! /usr/bin/env python -R',

+ +                ['-i', '/usr/bin/python3']),

+ +            '#! /usr/bin/python3')

+ +

+ +    def test_pathfix_keeping_flags(self):

+ +        self.assertEqual(

+ +            self.pathfix(

+ +                '#! /usr/bin/env python -R',

+ +                ['-i', '/usr/bin/python3', '-k']),

+ +            '#! /usr/bin/python3 -R')

+ +        self.assertEqual(

+ +            self.pathfix(

+ +                '#! /usr/bin/env python',

+ +                ['-i', '/usr/bin/python3', '-k']),

+ +            '#! /usr/bin/python3')

+ +

+ +    def test_pathfix_adding_flag(self):

+ +        self.assertEqual(

+ +            self.pathfix(

+ +                '#! /usr/bin/env python',

+ +                ['-i', '/usr/bin/python3', '-a', 's']),

+ +            '#! /usr/bin/python3 -s')

+ +        self.assertEqual(

+ +            self.pathfix(

+ +                '#! /usr/bin/env python -S',

+ +                ['-i', '/usr/bin/python3', '-a', 's']),

+ +            '#! /usr/bin/python3 -s')

+ +        self.assertEqual(

+ +            self.pathfix(

+ +                '#! /usr/bin/env python -V',

+ +                ['-i', '/usr/bin/python3', '-a', 'v', '-k']),

+ +            '#! /usr/bin/python3 -vV')

+ +        self.assertEqual(

+ +            self.pathfix(

+ +                '#! /usr/bin/env python',

+ +                ['-i', '/usr/bin/python3', '-a', 'Rs']),

+ +            '#! /usr/bin/python3 -Rs')

+ +        self.assertEqual(

+ +            self.pathfix(

+ +                '#! /usr/bin/env python -W default',

+ +                ['-i', '/usr/bin/python3', '-a', 's', '-k']),

+ +            '#! /usr/bin/python3 -sW default')

+ +

+ +    def test_pathfix_adding_errors(self):

+ +        self.pathfix(

+ +            '#! /usr/bin/env python -E',

+ +            ['-i', '/usr/bin/python3', '-a', 'W default', '-k'],

+ +            exitcode=2,

+ +            stderr="-a option doesn't support whitespaces")

+ +

+ +

+ +if __name__ == '__main__':

+ +    unittest.main()

+ diff --git a/Misc/NEWS.d/next/Tools-Demos/2019-05-27-15-26-12.bpo-37064.k_SPW2.rst b/Misc/NEWS.d/next/Tools-Demos/2019-05-27-15-26-12.bpo-37064.k_SPW2.rst

+ new file mode 100644

+ index 0000000..d1210e2

+ --- /dev/null

+ +++ b/Misc/NEWS.d/next/Tools-Demos/2019-05-27-15-26-12.bpo-37064.k_SPW2.rst

+ @@ -0,0 +1,2 @@

+ +Add option -k to pathscript.py script: preserve shebang flags.

+ +Add option -a to pathscript.py script: add flags.

+ diff --git a/Misc/NEWS.d/next/Tools-Demos/2019-10-02-09-48-42.bpo-38347.2Tq5D1.rst b/Misc/NEWS.d/next/Tools-Demos/2019-10-02-09-48-42.bpo-38347.2Tq5D1.rst

+ new file mode 100644

+ index 0000000..ae64a31

+ --- /dev/null

+ +++ b/Misc/NEWS.d/next/Tools-Demos/2019-10-02-09-48-42.bpo-38347.2Tq5D1.rst

+ @@ -0,0 +1 @@

+ +pathfix.py: Assume all files that end on '.py' are Python scripts when working recursively.

+ diff --git a/Tools/scripts/pathfix.py b/Tools/scripts/pathfix.py

+ index c5bf984..c5ba18b 100755

+ --- a/Tools/scripts/pathfix.py

+ +++ b/Tools/scripts/pathfix.py

+ @@ -1,6 +1,6 @@

+  #!/usr/bin/env python3

+  

+ -# Change the #! line occurring in Python scripts.  The new interpreter

+ +# Change the #! line (shebang) occurring in Python scripts.  The new interpreter

+  # pathname must be given with a -i option.

+  #

+  # Command line arguments are files or directories to be processed.

+ @@ -10,7 +10,13 @@

+  # arguments).

+  # The original file is kept as a back-up (with a "~" attached to its name),

+  # -n flag can be used to disable this.

+ -#

+ +

+ +# Sometimes you may find shebangs with flags such as `#! /usr/bin/env python -si`.

+ +# Normally, pathfix overwrites the entire line, including the flags.

+ +# To change interpreter and keep flags from the original shebang line, use -k.

+ +# If you want to keep flags and add to them one single literal flag, use option -a.

+ +

+ +

+  # Undoubtedly you can do this using find and sed or perl, but this is

+  # a nice example of Python code that recurses down a directory tree

+  # and uses regular expressions.  Also note several subtleties like

+ @@ -33,16 +39,21 @@ rep = sys.stdout.write

+  new_interpreter = None

+  preserve_timestamps = False

+  create_backup = True

+ +keep_flags = False

+ +add_flags = b''

+  

+  

+  def main():

+      global new_interpreter

+      global preserve_timestamps

+      global create_backup

+ -    usage = ('usage: %s -i /interpreter -p -n file-or-directory ...\n' %

+ +    global keep_flags

+ +    global add_flags

+ +

+ +    usage = ('usage: %s -i /interpreter -p -n -k -a file-or-directory ...\n' %

+               sys.argv[0])

+      try:

+ -        opts, args = getopt.getopt(sys.argv[1:], 'i:pn')

+ +        opts, args = getopt.getopt(sys.argv[1:], 'i:a:kpn')

+      except getopt.error as msg:

+          err(str(msg) + '\n')

+          err(usage)

+ @@ -54,6 +65,13 @@ def main():

+              preserve_timestamps = True

+          if o == '-n':

+              create_backup = False

+ +        if o == '-k':

+ +            keep_flags = True

+ +        if o == '-a':

+ +            add_flags = a.encode()

+ +            if b' ' in add_flags:

+ +                err("-a option doesn't support whitespaces")

+ +                sys.exit(2)

+      if not new_interpreter or not new_interpreter.startswith(b'/') or \

+             not args:

+          err('-i option or file-or-directory missing\n')

+ @@ -70,9 +88,10 @@ def main():

+              if fix(arg): bad = 1

+      sys.exit(bad)

+  

+ -ispythonprog = re.compile(r'^[a-zA-Z0-9_]+\.py$')

+ +

+  def ispython(name):

+ -    return bool(ispythonprog.match(name))

+ +    return name.endswith('.py')

+ +

+  

+  def recursedown(dirname):

+      dbg('recursedown(%r)\n' % (dirname,))

+ @@ -96,6 +115,7 @@ def recursedown(dirname):

+          if recursedown(fullname): bad = 1

+      return bad

+  

+ +

+  def fix(filename):

+  ##  dbg('fix(%r)\n' % (filename,))

+      try:

+ @@ -166,12 +186,43 @@ def fix(filename):

+      # Return success

+      return 0

+  

+ +

+ +def parse_shebang(shebangline):

+ +    shebangline = shebangline.rstrip(b'\n')

+ +    start = shebangline.find(b' -')

+ +    if start == -1:

+ +        return b''

+ +    return shebangline[start:]

+ +

+ +

+ +def populate_flags(shebangline):

+ +    old_flags = b''

+ +    if keep_flags:

+ +        old_flags = parse_shebang(shebangline)

+ +        if old_flags:

+ +            old_flags = old_flags[2:]

+ +    if not (old_flags or add_flags):

+ +        return b''

+ +    # On Linux, the entire string following the interpreter name

+ +    # is passed as a single argument to the interpreter.

+ +    # e.g. "#! /usr/bin/python3 -W Error -s" runs "/usr/bin/python3 "-W Error -s"

+ +    # so shebang should have single '-' where flags are given and

+ +    # flag might need argument for that reasons adding new flags is

+ +    # between '-' and original flags

+ +    # e.g. #! /usr/bin/python3 -sW Error

+ +    return b' -' + add_flags + old_flags

+ +

+ +

+  def fixline(line):

+      if not line.startswith(b'#!'):

+          return line

+ +

+      if b"python" not in line:

+          return line

+ -    return b'#! ' + new_interpreter + b'\n'

+ +

+ +    flags = populate_flags(line)

+ +    return b'#! ' + new_interpreter + flags + b'\n'

+ +

+  

+  if __name__ == '__main__':

+      main()

file modified
+15 -1
@@ -17,7 +17,7 @@ 

  #global prerel ...

  %global upstream_version %{general_version}%{?prerel}

  Version: %{general_version}%{?prerel:~%{prerel}}

- Release: 1%{?dist}

+ Release: 2%{?dist}

  License: Python

  

  
@@ -296,6 +296,14 @@ 

  # See https://src.fedoraproject.org/rpms/redhat-rpm-config/pull-request/57#comment-27426

  Patch328: 00328-pyc-timestamp-invalidation-mode.patch

  

+ # 00335 #

+ # Tools/scripts/pathfix.py backports

+ # Add -k and -a command line options to preserve and add shebang flags

+ # In upstream since 3.8: https://bugs.python.org/issue37064

+ # Assume all .py files are Python scripts when working recursively:

+ # In upstream since 3.8: https://bugs.python.org/issue38347

+ Patch335: 00335-backport-pathfix-change.patch

+ 

  # (New patches go here ^^^)

  #

  # When adding new patches to "python" and "python3" in Fedora, EL, etc.,
@@ -633,6 +641,7 @@ 

  %patch274 -p1

  %patch316 -p1

  %patch328 -p1

+ %patch335 -p1

  

  

  # Remove files that should be generated by the build
@@ -1541,6 +1550,11 @@ 

  # ======================================================

  

  %changelog

+ * Thu Dec 05 2019 Patrik Kopkan <pkopkan@redhat.com> - 3.7.5-2

+ - pathfix.py: add -k (keep) and -a (add) command line options,

+               assume all .py files are Python scripts when working recursively

+ - This changes will lead to having macros that will fix (#1335203)

+ 

  * Tue Oct 15 2019 Miro Hrončok <mhroncok@redhat.com> - 3.7.5-1

  - Update to 3.7.5

  

no initial comment

rebased onto bf3fe8fffe8562f5ade63d84434ba65018f6592f

4 years ago

The patches in python3 are applied manually, there is no %autosetup. Don't forget to apply the patch and bump the release.

rebased onto aa92cdd098cfa47db67f1bfc7b7a8bd40fa9fb5c

4 years ago

rebased onto 99ce8a6f6cc1fa702b89c37acfe569b68a081536

4 years ago

rebased onto b1f8cdcdc97ce7949fdb670189b566c07d80b4d2

4 years ago

now it should apply patch.

What do you think of also backporting the second pathfix change?
https://github.com/python/cpython/commit/2b7dc40b2af6578181808ba73c1533fc114e55df

Why is this PR done written for f31 branch? Why not working first in the master branch?

Usually, we push new features in master and then backport to other branches.

This is a typo and it would be better to explain what is the change rather than mentioning a bpo number. I don't think that Fedora users known what "bpo" stands for. By the way, is this change related to a bugzilla issue? If yes, please mention it.

Why is this PR done written for f31 branch? Why not working first in the master branch?
Usually, we push new features in master and then backport to other branches.

in rawhide should be python 3.8 so the change should be there right?

What do you think of also backporting the second pathfix change?
https://github.com/python/cpython/commit/2b7dc40b2af6578181808ba73c1533fc114e55df

Thank you, I forgot that which points me to why test didn't fail.

rebased onto 62ee38d87c78c12d0523a283b58998afdaa5df85

4 years ago

rebased onto 4aa62270529d6acc36603ce11b22e9a4857a2b31

4 years ago

Thank you, I forgot that which points me to why test didn't fail.

So are you ok to also backport the second pathfix change as part of the same PR? I suggest to have a single PR for the two pathfix changes.

rebased onto 9b1eceb255911e2fd9df75bcd58d7581a017dfc4

4 years ago

Now patch should contain three commits

rebased onto a5ef79b4434468c5155e2aefdc0eb9a25e5d53a7

4 years ago

Note that on Python 3.8+ all of our patches are stored in https://github.com/fedora-python/cpython

I recommend first opening a PR with the actual backport there.

Oh, this is a backport into 3.7 only? I'm so sorry. Anyway, we can use that PR to review the patch.

Note for myself: Rawhide doesn't need to be changed, since "python3" package in Rawhide is Python 3.8 which already includes all these pathfix changes.

Please mention also the 4th commit that you backported.

There is a typo: backport.

You may also mention the 4th fix to support "-" in filenames.

Patrick explained me that this change will be backported to Fedora 30. Miro suggests to ignore Fedora 29 will no longer be supported in a few days. Only python3 should be patched for RPM macros (ex: python36 doesn't need to be patched).

I asked if we could copy pathfix.py in the RPM macros package. Yes, it's possible. But Miro prefers that the pathfix provided by the python3 package is fixed as well. I agree with that.

rebased onto 219df6d589d7de527d85fec441047090d1d3011c

4 years ago

rebased onto e3cdcec02e6ca42ae50f3fbcafb53c63ee391ed9

4 years ago

Hum, it should be reformatted and rephrased. First, you need to add an empty line after your new changelog entry. I suggest the text:

* Tue Oct 29 2019 Patrik Kopkan <pkopkan@redhat.com> - 3.7.5-2
  - Backport pathfix changes: add -a and -k command line options,
    and assume all files that end on '.py' are Python scripts when
    working recursively.

I'm sorry, I don't understand what you mean by "have tool to solve (#1335203)".

rebased onto 088f095f28a4673410c3142d239d4414244e73fb

4 years ago

I'm sorry, I don't understand what you mean by "have tool to solve (#1335203)".

I want to say that this change don't fix this bug but leads to fixing it.
Is this rephrase better:
This changes will lead to having macros that will fix (#1335203)

Metadata Update from @churchyard:
- Pull-request tagged with: backport, feature
- Request assigned

4 years ago

rebased onto 7f00fb9acc766945728945947c13638938425753

4 years ago

rebased onto 9cf4138

4 years ago

I've done some changes to the comments, changelog entry and commit message.

Waiting for the build to finish, so I can sanity check if it actually works.

$ echo '#!/usr/bin/python -u' > ha.py 
$ pathfix.py -pni "/usr/bin/python3 -s" ha.py 
ha.py: updating
$ cat ha.py 
#! /usr/bin/python3 -s
$ echo '#!/usr/bin/python -u' > ha.py 
$ pathfix.py -pni /usr/bin/python3 -k ha.py 
ha.py: updating
$ cat ha.py 
#! /usr/bin/python3 -u
$ echo '#!/usr/bin/python -u' > ha.py 
$ pathfix.py -pni /usr/bin/python3 -k -as ha.py 
ha.py: updating
$ cat ha.py 
#! /usr/bin/python3 -su
$ echo '#!/usr/bin/python -u' > ha.py 
$ pathfix.py -pni /usr/bin/python3 -k -au ha.py 
ha.py: updating
$ cat ha.py 
#! /usr/bin/python3 -uu
$ pathfix.py -pni /usr/bin/python3 -as ha.py 
ha.py: updating
$ cat ha.py 
#! /usr/bin/python3 -s

It duplicates a flag. That is to be expected? if so, we cannot really use this.

Metadata Update from @churchyard:
- Request assignee reset

4 years ago

Metadata Update from @churchyard:
- Request assigned

4 years ago

It duplicates a flag. That is to be expected? if so, we cannot really use this.

Currently, pathfix doesn't remove duplicated files. For -v flag, one -v or two -v change sys.flags.verbose value. Some flags behave differently when their value is larger than 1 rather just equal to 1.

If it's an issue, pathfix should be modified upstream. But it may be tricky to get it right, since the flag can have a value.

Our motivation for this change is that we can add the -s flag (stored in %{py3_shbang_opts}) to existing shebangs, while keeping all other flags intact.

While we can certainly add a second -s flag (-ss appears to work), I don't consider it good design.

So will need some flag to specify max count of this flag, something like:
pathfix.py -pni /usr/bin/python3 -as -l "s" 1 ha.py

No. IMHO That is horrible API. When a flag without value and count is added and already present, nothing shall be added.

or pathfix.py -pni /usr/bin/python3 -as -l "{'s': 1}" ha.py

IMHO it's safe to specify -s flag more than once.

To me, It sounds like too much work to try to not duplicate some flags when using pathfix -k -a (...), since the behavior depend on the flag. Some flags have a value, others don't. Almost all flags without value behave the same when specified more than once.

As far as I know, only three Python command line flags behave differently if they are specified more than once:

  • -v (verbose)
  • -W (warning filters)
  • -O (optimization level)

The documentation says that -d (debug) can be specified more than once, but I checked the C code: it has no effect. It behaves as a boolean (disabled if equal to zero, enabled if greater than or equal to zero).
https://docs.python.org/3.8/using/cmdline.html

I checked the specific -s flag: it really behaves as a boolean. Extract of the site module code:

def check_enableusersite():
    """Check if user site directory is safe for inclusion (...)"""
    if sys.flags.no_user_site:
        return False
    ...

Even if technically, the flag value is different if -s is specified more than once:

$ python3 -c 'import sys; print(sys.flags.no_user_site)'
0
$ python3 -s -c 'import sys; print(sys.flags.no_user_site)'
1
$ python3 -s -s -c 'import sys; print(sys.flags.no_user_site)'
2
$ python3 -s -s -s -c 'import sys; print(sys.flags.no_user_site)'
3

For tests, Python uses a private subprocess _args_from_interpreter_flags() function which recreates command line options. This function also uses -s flag as a boolean (it only adds -s once even if the flag value is greater than 1):

        if sys.flags.no_user_site:
            args.append('-s')

Pull-Request has been merged by churchyard

4 years ago