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 28

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:

Removed 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.

Utilized Python 2.6 features:

  • PEP 3110 style exception catching, except X as e.
  • Print function (from __future__ import print_function) (PEP 3105).
  • Absolute imports (from __future__ import absolute_import).

The function terminate in trac.util will accept either 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 (28)

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.

in reply to:  21 comment:22 by Ryan J Ollos, 5 years ago

Replying to jomae:

#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.

Supporting multiple versions takes up a lot of extra time, especially supporting Python 2 and 3 on the same codebase. If it can be avoided, I'd rather spend my time developing features.

comment:23 by Ryan J Ollos, 5 years ago

I'm seeing an error,

08:23:28 AM Trac[loader] ERROR: Skipping "trac.mimeview.pygments = trac.mimeview.pygments [pygments]": 
Traceback (most recent call last):
  File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/loader.py", line 68, in _load_eggs
    entry.load(require=True)
  File "/home/user/Workspace/tracdev/local/lib/python2.7/site-packages/distribute-0.6.34-py2.7.egg/pkg_resources.py", line 2013, in load
    entry = __import__(self.module_name, globals(),globals(), ['__name__'])
  File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/mimeview/pygments.py", line 37, in <module>
    get_all_lexers = pygments.lexers.get_all_lexers
AttributeError: 'module' object has no attribute 'lexers'
$ python
Python 2.7.4 (default, Sep 26 2013, 03:20:26) 
[GCC 4.7.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import pygments
>>> pygments.__version__
'1.6'

It seems to be fixed with:

  • trac/mimeview/pygments.py

    diff --git a/trac/mimeview/pygments.py b/trac/mimeview/pygments.py
    index 9b3858f..7d010ee 100644
    a b from datetime import datetime  
    1616import os
    1717from pkg_resources import resource_filename
    1818import pygments
     19import pygments.formatters
     20import pygments.lexers
    1921import re
    2022
    2123from trac.core import *

It appears that pygments.styles is imported with pygments.formatters.

comment:24 by Ryan J Ollos, 5 years ago

We could use the print_function from __future__ PEP-3105: log:rjollos.git:t11600.4.

in reply to:  23 ; comment:25 by Jun Omae, 5 years ago

I'm seeing an error,

...
AttributeError: 'module' object has no attribute 'lexers'

Oops. I didn't notice. Unit and functional tests passed.

I've installed Pygments 1.4 and got the same error.

$ PYTHONPATH=. ~/venv/py26/bin/python -c 'import pygments; print pygments.__version__; import trac.mimeview.pygments'
1.4
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "trac/mimeview/pygments.py", line 35, in <module>
    get_all_lexers = pygments.lexers.get_all_lexers
AttributeError: 'module' object has no attribute 'lexers'

We could use import statement for the symbols. It would be simple. [5cda7782/jomae.git].

We could use the print_function from __future__ PEP-3105: log:rjollos.git:t11600.4.

Looks good to me.

Last edited 5 years ago by Jun Omae (previous) (diff)

in reply to:  25 ; comment:26 by Ryan J Ollos, 5 years ago

Replying to jomae:

Oops. I didn't notice. Unit and functional tests passed.

I would have expected at least test_extra_mimetypes to fail, but get_all_lexers seems to be imported correctly when the unit tests are executed.

We could use import statement for the symbols. It would be simple. [5cda7782/jomae.git].

That refactoring looks good and the change works well.

We could use the print_function from __future__ PEP-3105: log:rjollos.git:t11600.4.

Looks good to me.

Thanks. Committed to trunk in [12788].

in reply to:  26 comment:27 by Jun Omae, 5 years ago

We could use import statement for the symbols. It would be simple. [5cda7782/jomae.git].

That refactoring looks good and the change works well.

Thanks, committed in [12789].

comment:28 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)
Note: See TracTickets for help on using tickets.