bpo-37064: adds option to keep/add flags to pathfix. - #13591

PatrikKopkan · 2019-05-27T13:13:58Z

https://bugs.python.org/issue37064

- option enables adding flag

the-knights-who-say-ni · 2019-05-27T13:14:02Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Our records indicate we have not received your CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

eamanu · 2019-05-27T23:47:46Z

@@ -0,0 +1,3 @@
+adds option -f to pathscript.py. The option takes argument and is for adding flags. If argument is empty, it just keeps flags.


What about:

Add option -f to pathscript.py. This option takes argument to adding flags ...

eamanu · 2019-05-27T23:49:59Z

@@ -0,0 +1,3 @@
+adds option -f to pathscript.py. The option takes argument and is for adding flags. If argument is empty, it just keeps flags.
+Let's say you have script with '#! /usr/bin/env -R' and you want to just change interpreter. You can run now


I don't know if necessary this example

PatrikKopkan · 2019-05-29T13:41:53Z

Thanks @eamanu for review

auvipy

looks good

hroncok

The usage is not updated:

$ python3 Tools/scripts/pathfix.py 
-i option or file-or-directory missing
usage: Tools/scripts/pathfix.py -i /interpreter -p -n file-or-directory ...

Also, not sure how to use -f without a value:

$ python3 Tools/scripts/pathfix.py -i /usr/bin/python3 -f script 
-i option or file-or-directory missing
usage: Tools/scripts/pathfix.py -i /interpreter -p -n file-or-directory ...

$ python3 Tools/scripts/pathfix.py -i /usr/bin/python3 script -f
script: updating
-f: cannot open: FileNotFoundError(2, 'No such file or directory')

$ cat script 
#!python -xyz
[cpython (master %)]$ python3 Tools/scripts/pathfix.py -i /usr/bin/python3.7 -f "" script 
script: updating
[cpython (master %)]$ cat script
#! /usr/bin/python3.7

hroncok

This should fix the -f "" use case.

hroncok · 2019-06-19T12:25:54Z

 new_interpreter = None
 preserve_timestamps = False
 create_backup = True
+add_flag = ''


Suggested change

add_flag = ''

add_flag = None

hroncok · 2019-06-19T12:26:08Z

-    return b'#! ' + new_interpreter + b'\n'
+
+    fixedline = b'#! ' + new_interpreter
+    if not add_flag:


Suggested change

if not add_flag:

if add_flag in None:

encukou · 2019-06-26T09:31:52Z

+        subprocess.call([sys.executable, self.script, '-i', '/usr/bin/python', '-n', self.temp_file])
+        with open(self.temp_file) as f:
+            output = f.read()
+        self.assertEqual(output, '#! /usr/bin/python\n')


Shouldn't this test that the backup is not created?

encukou · 2019-06-26T09:31:57Z

+                testing_output = self.pathfix.fixline(line)
+                self.assertEqual(testing_output, b'#! ' + self.pathfix.new_interpreter + b'\n')
+
+    input_file = \


Please keep to PEP 8:

The preferred way of wrapping long lines is by using Python's implied line continuation inside parentheses, brackets and braces. Long lines can be broken over multiple lines by wrapping expressions in parentheses. These should be used in preference to using a backslash for line continuation.

PatrikKopkan · 2019-07-01T10:50:14Z

Here is failed test_os. I believe it is not related to this PR change.

vstinner

pathfix.py seems to be old and can be modernized. I suggest to work on a first PR only to replace optparse with argparse and add a few basic tests, before adding new features.

vstinner · 2019-07-08T14:07:52Z

+
+# Sometimes you may find shebangs with flags such as `#! /usr/bin/env python -si`.
+# Normally, pathfix overwrites the entire line, including the flags.
+# To keep flags from the original shebang line, use -f "".


I would prefer to have one option to preserve existing flags and another to add more flags. Like:

pathfix.py --preserve-flags --add-flags "..." script

where --add-flags "..." would accept an arbitrary string like: --add-flags "-X dev --check-hash-based-pycs=never".

I suggest to replace legacy optparse with newer argparse, it's easier to use "long flags" (like --preserve-flags) in argparse.

If you really want to short flags (single letter), I suggest:

--preserve-flags/-p

--add-flags/-a FLAGS

The Linux kernel treats everything following the first “word” in the shebang line as a single argument. Such flags as -X dev --check-hash-based-pycs=never are impossible here.

vstinner · 2019-07-08T14:08:47Z

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


I dislike global variables :-( Maybe all "options" and be stored in a "options" variable (namespace) when updating the code to argparse.

vstinner · 2019-07-08T14:13:49Z

+    """
+    end = len(shebangline)
+    start = shebangline.find(b' -', 0, end) + 2  # .find() returns index at space
+    if chr(shebangline[start]).isalpha():


This code looks fragile. It doesn't support --check-hash-based-pycs=MODE option.

IMHO it could be simpler: cut the string at " -" without checking what is next.

The Linux kernel simply uses a space to separate the end of the program name and the start of command line arguments. Maybe " -" is maybe safer than just " ", since I'm not sure how the Windows launcher py.exe handles spaces.

vstinner · 2019-07-08T14:15:10Z

+    if add_flag in flags:
+        return fixedline + b' -' + flags + args + b'\n'
+
+    return fixedline + b' -' + add_flag + flags + args + b'\n'


I would expect a line like: b'#! ' + new_interpreter + flags + b'\n'. Then arrange your code to build flags as you want, flags can an empty string.

I also suggest to create a sub-function fix_shebang() to clarify the purpose of the function.

encukou · 2019-07-08T14:34:37Z

I suggest to work on a first PR only to replace optparse with argparse and add a few basic tests, before adding new features.

Are you saying an enhancement can't be accepted without refactoring the whole tool?

vstinner · 2019-07-09T11:49:58Z

Are you saying an enhancement can't be accepted without refactoring the whole tool?

It's not are requirement, just a suggestion.

Patrik Kopkan added 9 commits May 27, 2019 14:51

added option -f for pathfix

accd417

- option enables adding flag

detecting same flag

0aa1237

cleared leftovers

33045ef

changed slashes

a0b5079

added comments

43ac6f9

add test case

2d18119

moved tests

d108b0b

reworked tests to unittests

98fac52

fixing typos

fa6d07d

the-knights-who-say-ni added the CLA not signed label May 27, 2019

bedevere-bot added the awaiting review label May 27, 2019

blurb-it Bot and others added 2 commits May 27, 2019 15:26

📜🤖 Added by blurb_it.

882c744

fixes whitespaces

4fca2df

eamanu reviewed May 27, 2019

View reviewed changes

edited test setup, edited news entry

d11689c

the-knights-who-say-ni added CLA signed and removed CLA not signed labels May 29, 2019

Patrik Kopkan added 2 commits May 29, 2019 16:57

added test_main

3802896

adds deletion of left file

3dcee91

auvipy approved these changes May 31, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels May 31, 2019

brettcannon added the type-feature A feature request or enhancement label Jun 3, 2019

hroncok suggested changes Jun 19, 2019

View reviewed changes

hroncok reviewed Jun 19, 2019

View reviewed changes

Patrik Kopkan added 3 commits June 19, 2019 15:16

updated usage, comments, default content of variable

3b37555

add tests

08fa4dd

make test work for windows

6c4071d

encukou reviewed Jun 26, 2019

View reviewed changes

Patrik Kopkan added 3 commits June 26, 2019 17:06

litle edit to tests

03de591

updated doc

d9dcbf3

added test

1544c81

vstinner reviewed Jul 8, 2019

View reviewed changes

Patrik Kopkan added 2 commits July 14, 2019 09:15

refractor

900fb6d

rewrite to have option -k for keeping flags

693f733

PatrikKopkan closed this Sep 17, 2019

		@@ -0,0 +1,3 @@
		adds option -f to pathscript.py. The option takes argument and is for adding flags. If argument is empty, it just keeps flags.

		@@ -0,0 +1,3 @@
		adds option -f to pathscript.py. The option takes argument and is for adding flags. If argument is empty, it just keeps flags.
		Let's say you have script with '#! /usr/bin/env -R' and you want to just change interpreter. You can run now

Uh oh!

Uh oh!

Conversation

PatrikKopkan commented May 27, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented May 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PatrikKopkan commented May 29, 2019

Uh oh!

auvipy left a comment

Choose a reason for hiding this comment

Uh oh!

hroncok left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hroncok left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PatrikKopkan commented Jul 1, 2019

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

encukou commented Jul 8, 2019

Uh oh!

vstinner commented Jul 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

PatrikKopkan commented May 27, 2019 •

edited by bedevere-bot

Loading

hroncok left a comment •

edited

Loading