#147 Fedora 31: pathfix.py: add -k (keep) and -a (add) CLI options, fix all *.py files
Opened a month ago by pkopkan. Modified 4 days ago
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 bf3fe8f

a month 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 aa92cdd

a month ago

rebased onto 99ce8a6

a month ago

rebased onto b1f8cdc

a month 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 62ee38d

a month ago

rebased onto 4aa6227

a month 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 9b1eceb

a month ago

Now patch should contain three commits

rebased onto a5ef79b

a month 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 219df6d

24 days ago

rebased onto e3cdcec

24 days 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 088f095

20 days 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 days ago

rebased onto 7f00fb9

4 days ago

rebased onto 9cf4138

4 days 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 days ago

Metadata Update from @churchyard:
- Request assigned

4 days 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.