Edgewall Software
Modify

Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#11600 closed task (fixed)

Remove Python 2.5 compatibility

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.

Internal Changes:

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

Attachments (0)

Change History (36)

comment:1 by Ryan J Ollos, 10 years ago

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

Committed to trunk in [12751].

comment:2 by Jun Omae, 10 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, 10 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 10 years ago by Ryan J Ollos (previous) (diff)

comment:4 by Ryan J Ollos, 10 years ago

API Changes: modified (diff)

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

comment:5 by Ryan J Ollos, 10 years ago

Description: modified (diff)

comment:6 by Ryan J Ollos, 10 years ago

Description: modified (diff)

comment:7 by Jun Omae, 10 years ago

Cc: Jun Omae added

Other things:

in reply to:  7 ; comment:8 by Ryan J Ollos, 10 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, 10 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, 10 years ago

Resolution: fixed
Status: closedreopened

in reply to:  3 ; comment:11 by Ryan J Ollos, 10 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, 10 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, 10 years ago

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

comment:14 by anonymous, 10 years ago

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

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

comment:15 by Jun Omae, 10 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, 10 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 10 years ago by Ryan J Ollos (previous) (diff)

in reply to:  16 ; comment:17 by Jun Omae, 10 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, 10 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, 10 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, 10 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, 10 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, 10 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, 10 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, 10 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, 10 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 10 years ago by Jun Omae (previous) (diff)

in reply to:  25 ; comment:26 by Ryan J Ollos, 10 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, 10 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, 10 years ago

API Changes: modified (diff)

comment:29 by Ryan J Ollos, 10 years ago

Here is a FIXME 2.6: tags/trac-1.0.1/trac/versioncontrol/web_ui/browser.py@:43#L24.

The mix of absolute and relative imports in the codebase is inconsistent. Maybe we should just use absolute imports since they are already used so extensively in the codebase, and might be considered more clear. I haven't done much reading on relative imports to understand what the advantages might be, but so far I can't see much advantage to using relative imports in the Trac codebase.

comment:30 by Jun Omae, 10 years ago

I'm wondering that the comment says 2.6. Relative imports are already used in tags/trac-1.0.1/trac/util/__init__.py@:36-37#L20, the statements work on Python 2.5.

Agreed. I think we should use only absolute imports.

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

Sure. I've worked a little porting to Python 3 and I learned supporting both Python 2 and 3 isn't a easy job. https://github.com/jun66j5/trac/commits/python3.

comment:31 by Ryan J Ollos, 10 years ago

I find only a few cases of relative imports with grep, fewer than I imagined:

$ grep -r "from \." --exclude-dir=.svn --include=*.py
trac/db/api.py:from .pool import ConnectionPool
trac/db/api.py:from .util import ConnectionWrapper
trac/util/__init__.py:from .compat import any, md5, sha1, sorted
trac/util/__init__.py:from .datefmt import to_datetime, to_timestamp, utc
trac/util/__init__.py:from .text import exception_to_unicode, to_unicode, getpreferredencoding
trac/wiki/api.py:from .parser import WikiParser
trac/versioncontrol/web_ui/browser.py:from ..api import NoSuchChangeset, RepositoryManager
trac/versioncontrol/web_ui/browser.py:from trac.versioncontrol.web_ui.util import * # `from .util import *` FIXME 2.6
trac/cache.py:from .core import Component
trac/cache.py:from .util import arity
trac/cache.py:from .util.concurrency import ThreadLocal, threading

Proposed changes for trunk in log:rjollos.git:t11600.5.

comment:32 by Ryan J Ollos, 10 years ago

The pattern in the use of relative imports I see is that relative imports are used to import from modules within the same packages. For example, importing from a module in trac.wiki to another module of trac.wiki, relative imports would be used. So we could go the other direction and make sure this relative import pattern is implemented everywhere. I wonder though, what the advantages might be.

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

in reply to:  30 comment:33 by Ryan J Ollos, 10 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.

Sure. I've worked a little porting to Python 3 and I learned supporting both Python 2 and 3 isn't a easy job. https://github.com/jun66j5/trac/commits/python3.

I guess there are a number of paths we could consider, and many factors that could influence the decision. For instance, it might be easier to drop 2.6 support in Trac 1.4 and only support Python 2.7 / 3.3, or we could just move straight to Python 3.3. One of the biggest factors I can think of is to ease the transition for plugins.

comment:34 by Ryan J Ollos, 10 years ago

Changes from comment:31 committed to trunk in [12835].

in reply to:  25 ; comment:35 by Ryan J Ollos, 9 years ago

Replying to jomae:

I'm seeing an error,

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

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

Simple functional test for Syntax Highlighting preferences page added in [14044].

in reply to:  35 comment:36 by Ryan J Ollos, 9 years ago

Replying to rjollos:

Simple functional test for Syntax Highlighting preferences page added in [14044].

Modified test to only run when Pygments is present in [14274].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.