Edgewall Software
Modify

Opened 5 years ago

Closed 5 years ago

Last modified 3 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
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

Attachments (0)

Change History (36)

comment:1 Changed 5 years ago by Ryan J Ollos

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

Committed to trunk in [12751].

comment:2 Changed 5 years ago by Jun Omae

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 Changed 5 years ago by Ryan J Ollos

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 Changed 5 years ago by Ryan J Ollos

API Changes: modified (diff)

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

comment:5 Changed 5 years ago by Ryan J Ollos

Description: modified (diff)

comment:6 Changed 5 years ago by Ryan J Ollos

Description: modified (diff)

comment:7 Changed 5 years ago by Jun Omae

Cc: Jun Omae added

Other things:

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

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.

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

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 Changed 5 years ago by Ryan J Ollos

Resolution: fixed
Status: closedreopened

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

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 Changed 5 years ago by Ryan J Ollos

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 Changed 5 years ago by Jun Omae

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

comment:14 Changed 5 years ago by anonymous

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

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

comment:15 Changed 5 years ago by Jun Omae

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 Changed 5 years ago by Ryan J Ollos

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)

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

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

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

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

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

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?

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

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?

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

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.

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

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 Changed 5 years ago by Ryan J Ollos

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 Changed 5 years ago by Ryan J Ollos

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

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

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)

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

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

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

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 Changed 5 years ago by Ryan J Ollos

API Changes: modified (diff)

comment:29 Changed 5 years ago by Ryan J Ollos

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 Changed 5 years ago by Jun Omae

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 Changed 5 years ago by Ryan J Ollos

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 Changed 5 years ago by Ryan J Ollos

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 5 years ago by Ryan J Ollos (previous) (diff)

comment:33 in reply to:  30 Changed 5 years ago by Ryan J Ollos

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 Changed 5 years ago by Ryan J Ollos

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

comment:35 in reply to:  25 ; Changed 4 years ago by Ryan J Ollos

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

comment:36 in reply to:  35 Changed 3 years ago by Ryan J Ollos

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