Opened 4 months ago

Closed 3 months ago

#33192 closed defect (fixed)

Stop assuming that /usr/bin/python exists

Reported by: alwayslivid Owned by:
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 044-should, BugSmashFund
Cc: nickm Actual Points: 0.1
Parent ID: #33193 Points:
Reviewer: teor Sponsor:

Description

This issue is relevant to the following ticket: #29913

In short, the problem in #29913 was that Python 3 scripts cannot depend on a hardcoded path in order to find the Python 3 executable, because the path itself is not immutable. (For instance, as seen in that ticket itself.

Unfortunately, the same exact issue also applies to Python 2 scripts and their relevant executables.

Child Tickets

Change History (15)

comment:1 Changed 4 months ago by alwayslivid

Please check my proposed change here: https://github.com/torproject/tor/pull/1712

This change will be particularly useful for people that are using FreeBSD, have compiled Python on a Linux machine, or use non-traditional setups of Python.

comment:2 Changed 4 months ago by teor

Cc: nickm added
Keywords: extra-review added
Milestone: Tor: 0.4.3.x-final
Status: newneeds_review
Version: Tor: 0.4.2.5

Looks good to me, but I think the bugfix version in the changes file should be much earlier.

We use the earliest version that the bug was introduced in, so you want the first commit that used the path "/usr/bin/python". Here are some instructions for finding it:
https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n121

I'd also like to check with Nick (another maintainer), to see if this change go in 0.4.3, or 0.4.4,

Here are the tradeoffs:

  • It's a long-standing bug on BSD, so it's not urgent
  • We still haven't checked if all of our scripts work on python 3 (see #33196), and this change could make more scripts use python 3 on more machines

comment:3 Changed 4 months ago by teor

Also, when we merge, we should cherry-pick this change, because merging from a user branch called "maint-0.4.2" will confuse developers.

Usually, we name branches after the ticket number, or after the feature or bug they fix.

Our maint-0.4.2 branch is for maintenance changes only, and I don't think this change qualifies. It's too risky.

comment:4 Changed 4 months ago by alwayslivid

I probably misinterpreted something in the documentation, excuse me for that one.

I could close the pull request and make the change again on top of the master or another desired branch.

comment:5 Changed 4 months ago by alwayslivid

I checked some of the scripts and changed "python" to "python2" wherever the syntax wouldn't allow using Python 3. I also imported print_function from future in some cases.

comment:6 in reply to:  5 ; Changed 4 months ago by teor

Status: needs_reviewneeds_revision

Replying to alwayslivid:

I probably misinterpreted something in the documentation, excuse me for that one.

Maybe our documentation isn't clear?

I could close the pull request and make the change again on top of the master or another desired branch.

That would be great!

Replying to alwayslivid:

I checked some of the scripts and changed "python" to "python2" wherever the syntax wouldn't allow using Python 3.

We'd like to transition to Python 3, since python 2 is not supported. So let's leave those scripts with "python" and fix the Python 3 errors?

There's similar work going on in #33193, you might want to check out the latest PR there?

I also imported print_function from __future__ in some cases.

I think we already added all the Python 2.7 to Python 3 future statements in master in #32732.

comment:7 Changed 4 months ago by teor

Parent ID: #33193

We'd like to make these changes in two different releases:

  • /usr/bin/env python change in 0.4.3, and
  • leave the other syntax fixes for 0.4.4 (in #33193).

The master branch is currently 0.4.3, but we will split off maint-0.4.3 soon, and then the master branch will become maint-0.4.4.

comment:8 Changed 4 months ago by nickm

Keywords: 043-can added

comment:9 in reply to:  6 ; Changed 4 months ago by alwayslivid

Replying to teor:

Replying to alwayslivid:

I could close the pull request and make the change again on top of the master or another desired branch.

That would be great!

Just for the sake of clarification, I ought to use the master branch for my own changes, right?

Replying to alwayslivid:

I checked some of the scripts and changed "python" to "python2" wherever the syntax wouldn't allow using Python 3.

We'd like to transition to Python 3, since python 2 is not supported. So let's leave those scripts with "python" and fix the Python 3

Transitioning from Python 2 to 3 would be pretty great, especially when you take how critical Tor and its surrounding software is into account. For the time being, however, this change would potentially break some of the scripts, considering that python can also refer to python3 and some scripts simply aren't suited for python3 right now. Maybe this explicit mention of python2 would also mobilize more people to work on that. (I'd like to do some work on that myself later on.)

I'm not sure whether I misinterpreted something, but the changes of the aforementioned tree (#33193) don't seem to be conflicting with my own changes in any capacity.

comment:10 in reply to:  9 Changed 4 months ago by teor

Replying to alwayslivid:

Replying to teor:

Replying to alwayslivid:

I could close the pull request and make the change again on top of the master or another desired branch.

That would be great!

Just for the sake of clarification, I ought to use the master branch for my own changes, right?

Yes please!

Replying to alwayslivid:

I checked some of the scripts and changed "python" to "python2" wherever the syntax wouldn't allow using Python 3.

We'd like to transition to Python 3, since python 2 is not supported. So let's leave those scripts with "python" and fix the Python 3

Transitioning from Python 2 to 3 would be pretty great, especially when you take how critical Tor and its surrounding software is into account. For the time being, however, this change would potentially break some of the scripts, considering that python can also refer to python3 and some scripts simply aren't suited for python3 right now. Maybe this explicit mention of python2 would also mobilize more people to work on that. (I'd like to do some work on that myself later on.)

Some OSes don't install Python 2 as python2, so we can't change to /usr/bin/env python2 in the current tor 0.4.3 alpha series.

Maybe it would be best to leave all these changes until 0.4.4, when we have time to fix any Python 3 issues?

I'm not sure whether I misinterpreted something, but the changes of the aforementioned tree (#33193) don't seem to be conflicting with my own changes in any capacity.

I wasn't sure if there were conflicts.

But I thought if you were trying to fix issues with Python 3, you might want to re-use some of those fixes.

comment:11 Changed 4 months ago by teor

Keywords: 044-should added; 043-can removed
Milestone: Tor: 0.4.3.x-finalTor: 0.4.4.x-final

We've opened merges to 0.4.4 in our master branch.

Let's make all these changes in master, except for scripts that are:

  • supposed to run on Python 2,
  • but are broken in maint-0.4.3.

comment:12 Changed 3 months ago by alwayslivid

Will work on it.

comment:14 Changed 3 months ago by teor

Actual Points: 0.1
Keywords: BugSmashFund added; extra-review removed
Reviewer: teor
Status: needs_revisionmerge_ready

Thanks! These changes look good.

Feel free to open other trac tickets and pull requests for Python 3 compatibility changes. Now we're in 0.4.4, we can merge Python 3 fixes.

comment:15 Changed 3 months ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged

Note: See TracTickets for help on using tickets.