Edgewall Software

Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#11600 closed task (fixed)

Remove Python 2.5 compatibility — at Version 21

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.2
Component: general Version:
Severity: normal Keywords: python26
Cc: Jun Omae Branch:
Release Notes:

Minimum required Python version is 2.6.

API Changes:

Remove Python 2.5 compatibility:

  • statements from __future__ import with_statement.
  • trac.util.presentation.to_json for the case that json can't be imported.
  • cleandoc definition in trac.util.compat.
  • PEP 3110 style exception catching, except X as e.

The function terminate in trac.util will accept a subprocess.Popen object or integer process id as an input.

Description (last modified by Ryan J Ollos)

If the changes proposed in #11581 are committed, we will start enforcing the Python 2.6 requirement on the trunk (see TracDev/ApiChanges/1.1.1). Therefore the from __future__ import with_statement lines can be removed from the codebase.

See https://docs.python.org/2/whatsnew/2.6.html

Change History (21)

comment:1 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: newclosed

Committed to trunk in [12751].

comment:2 by Jun Omae, 5 years ago

Description: modified (diff)
Summary: Remove "from __future__ import with_statement"Remove Python 2.5 compatibility

json library has been added as a standard module since 2.6. We could remove except ImportError block for from json ... in tags/trac-1.1.1/trac/util/presentation.py@:306#L293.

comment:3 by Ryan J Ollos, 5 years ago

Okay, I'll go ahead and make that change as well.

There's also a comment in pygments, which implies the import might be handled differently in Python 2.5 and later.

I don't seem to be receiving email notifications for this ticket.

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:4 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)

Change suggested in comment:2 committed in [12758].

comment:5 by Ryan J Ollos, 5 years ago

Description: modified (diff)

comment:6 by Ryan J Ollos, 5 years ago

Description: modified (diff)

comment:7 by Jun Omae, 5 years ago

Cc: Jun Omae added

Other things:

in reply to:  7 ; comment:8 by Ryan J Ollos, 5 years ago

Replying to jomae:

Other things:

The comment mentions some possible issues on Windows: branches/1.0-stable/trac/util/__init__.py@12083:370#L366.

There's also a use of terminate where the pid is attached to an object that's not a subprocess.Popen object: branches/1.0-stable/trac/tests/functional/testenv.py@12528:286#L280.

I've prepared some changes in log:rjollos.git:t11600.2.

in reply to:  8 comment:9 by Jun Omae, 5 years ago

Replying to rjollos:

There's also a use of terminate where the pid is attached to an object that's not a subprocess.Popen object: branches/1.0-stable/trac/tests/functional/testenv.py@12528:286#L280.

Oh, you're right. I didn't intend non-Popen instance is passed to terminate(). We leave it.

comment:10 by Ryan J Ollos, 5 years ago

Resolution: fixed
Status: closedreopened

in reply to:  3 ; comment:11 by Ryan J Ollos, 5 years ago

Replying to rjollos:

There's also a comment in pygments, which implies the import might be handled differently in Python 2.5 and later.

It looks like absolute imports can be enforced in Python 2.5 and later using a module from __future__. This is described in the Python 2.5 release notes.

I wonder if we could use this feature to avoid problems like the one in #11248, and the test failure in comment:4:ticket:11284. I'll save that for another day though.

comment:12 by Ryan J Ollos, 5 years ago

I added a few more changes to log:rjollos.git:t11600.2, to adjust to the additional use case of terminate. Please let me know if you have any comments.

comment:13 by Jun Omae, 5 years ago

In [78c15189/rjollos.git], we could remove process argument from terminate_win(process) and terminate_nix(process).

comment:14 by anonymous, 5 years ago

API Changes: modified (diff)
Resolution: fixed
Status: reopenedclosed

Thanks, committed to trunk in [12764:12765].

comment:15 by Jun Omae, 5 years ago

I noticed two things.

We could removed checking sys.version_info with (2, 6), see [fe905968/jomae.git] and [3d54bf056/jomae.git].

And I'm Sorry. We should revert [12765]. The terminate function avoids #10607 and th:#9646 in [11000] and #10958 in [11710]. If Popen.terminate is called for terminated process, it raises OSError: [Errno 3] No such process on Unix and WindowsError: (5, 'Access is denied') on Windows.

comment:16 by Ryan J Ollos, 5 years ago

Would you mind pushing those changes? I'm a bit busy for the next several days.

If possible, it would be nice to keep the change in [12765#file0] and the pid = process if isinstance(process, int) else process.pid in [12765#file1]

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

in reply to:  16 ; comment:17 by Jun Omae, 5 years ago

Would you mind pushing those changes? I'm a bit busy for the next several days.

Okay. I'll push later.

If possible, it would be nice to keep the change in [12765#file0] and the pid = process if isinstance(process, int) else process.pid in [12765#file1]

Yeah, I've updated jomae.git@t11600.3 and keep the changes, see [1641e1aa/jomae.git].

in reply to:  11 comment:18 by Jun Omae, 5 years ago

I wonder if we could use this feature to avoid problems like the one in #11248, and the test failure in comment:4:ticket:11284. I'll save that for another day though.

It's no wonder.

By the following reason, the import pygments will use pygments.py in trac/mimeview/tests directory even if absolute_import is used.

If the script name refers directly to a Python file, the directory containing that file is added to the start of sys.path, and the file is executed as the __main__ module.

From https://docs.python.org/2/using/cmdline.html.

But I think it is simple to use absolute_import, see [21547258/jomae.git].

in reply to:  17 ; comment:19 by Jun Omae, 5 years ago

Replying to jomae:

Would you mind pushing those changes? I'm a bit busy for the next several days.

Okay. I'll push later.

Committed in [12778-12780].

PEP 3110 style exception catching in [5e31e276/jomae.git], e.g. except X as e. If we have a plan to support Python 3, we need to change the style. Should we apply the changes?

in reply to:  19 ; comment:20 by Ryan J Ollos, 5 years ago

Replying to jomae:

PEP 3110 style exception catching in [5e31e276/jomae.git], e.g. except X as e. If we have a plan to support Python 3, we need to change the style. Should we apply the changes?

That looks good to me. It looks like the change was planned: TracDev/ApiChanges/1.1.1.

As far as a tentative plan for switching to Python 3.2 or 3.3, shall we consider that for Trac 1.4?

in reply to:  20 comment:21 by Jun Omae, 5 years ago

API Changes: modified (diff)

Replying to rjollos:

Replying to jomae:

PEP 3110 style exception catching in [5e31e276/jomae.git], e.g. except X as e. If we have a plan to support Python 3, we need to change the style. Should we apply the changes?

That looks good to me. It looks like the change was planned: TracDev/ApiChanges/1.1.1.

Thanks, committed in [12785]. Also absolute_import for importing pygments is committed in [12784].

As far as a tentative plan for switching to Python 3.2 or 3.3, …

#10083 has been filed about porting to Python 3. I think it might be good to support both 2.6+ and 3.3+, not switching.

…, shall we consider that for Trac 1.4?

Agreed.

Note: See TracTickets for help on using tickets.