Ticket #9580 (closed defect: fixed)

Opened 1 year ago

Last modified 7 months ago

Recursion after installing in Trac 0.13 (dev)

Reported by: gary.martin@wandisco.com Assigned to: olemis
Priority: normal Component: ThemeEnginePlugin
Severity: normal Keywords:
Cc: rjollos Trac Release: 0.12

Description

A change in the component system for 0.13 appears to have broken the ThemeEnginePlugin such that ThemeEngineSystem.__init__() is called recursively. The attached patch stops the ThemeEngineSystem class from implementing IThemeProvider and separates out the code relating to the default theme into its own class.

Attachments

theme-engine-system-recursion.diff (4.3 kB) - added by gary.martin@wandisco.com on 12/05/11 16:29:01.
patch for recursion error

Change History

12/05/11 16:29:01 changed by gary.martin@wandisco.com

  • attachment theme-engine-system-recursion.diff added.

patch for recursion error

01/18/12 23:05:40 changed by olemis

Recently I tried to install this plugin in 0.13dev and got similar results i.e. it fails at load time . Fact is that this happens due to the changes committed in 0.12 (more precisely trac:changeset:10298 and trac:changeset:10300 . The whole story is explained in trac:ticket:9418 ;)

I'll be reporting this in t.e.o. issue tracker . I suggest to close this ticket as wontfix .

01/19/12 15:32:46 changed by anonymous

The comment above is not accurate. Patch attached to trac:ticket:9418 was committed in trac:changeset:10295 . Revisions mentioned above add further changes on top of it .

01/20/12 16:32:49 changed by olemis

  • status changed from new to closed.
  • resolution set to wontfix.

As long as there's no definitive solution provided by trac-dev , users experiencing this issue can try to apply this patch to Trac distribution .

https://bitbucket.org/olemis/trac-mq/raw/tho_themeengine/tho/themeengine/t-9580-infinite-recursion.diff

This bug is definitely caused by Trac core rather than the plugin , hence I'm closing the ticket with wontfix (... at least for the moment ;)

01/20/12 21:50:41 changed by anonymous

  • status changed from closed to reopened.
  • resolution deleted.

Reopening ticket as this won't be fixed neither in 0.13 nor even later :-/

(follow-up: ↓ 7 ) 10/09/12 19:53:20 changed by anonymous

    def __init__(self):
        self._info = None

    @property
    def info(self):
        # This can safely be cached because the data can only change on a restart anyway
        if self._info is None:
            self._info = {}
            for provider in self.providers:
                for name in provider.get_theme_names():
                    theme = provider.get_theme_info(name)
                    theme['provider'] = provider
                    theme['module'] = provider.__class__.__module__
                    theme['name'] = name
                    self._info[name.lower()] = theme
        return self._info

10/10/12 01:37:42 changed by rjollos

  • cc set to rjollos.

(in reply to: ↑ 5 ) 10/10/12 11:08:50 changed by gary.martin@wandisco.com

Replying to anonymous

That looks like a really nice solution to me. The idea of deferring calculation of ThemeEngineSystem.info seems sound and a few quick tests suggest to me that it should work.

10/11/12 21:09:52 changed by anonymous

As suggested by cboos you could use @lazy since Trac 1.0 (or by copying it):

from trac.util import lazy
...
    @lazy 
    def info(self):
        result = {}
        for provider in self.providers:
            for name in provider.get_theme_names():
                theme = provider.get_theme_info(name)
                theme['provider'] = provider
                theme['module'] = provider.__class__.__module__
                theme['name'] = name
                result[name.lower()] = theme
        return result

(follow-up: ↓ 15 ) 10/14/12 06:23:08 changed by olemis

  • status changed from reopened to new.
  • owner changed from coderanger to olemis.

I'm proposing the following patch against trunk (see r12162 ) to make it work with Trac >= 0.11 . I'm also raising minor version number (i.e. 2.0.1 => 2.1.0 ) to delimit the exact point before and after Trac >= 0.12 compatibility .

Index: trunk/setup.py
===================================================================
--- trunk/setup.py	(revision 12163)
+++ trunk/setup.py	(working copy)
@@ -14,7 +14,7 @@
 
 setup(
     name = 'TracThemeEngine',
-    version = '2.0.1',
+    version = '2.1.0',
     packages = ['themeengine'],
     package_data = { 'themeengine': ['templates/*.html', 'htdocs/*.js', 'htdocs/*.css', 'htdocs/img/*.gif',
                                      'htdocs/farbtastic/*.png', 'htdocs/farbtastic/*.js', 'htdocs/farbtastic/*.css' ] },
Index: trunk/themeengine/api.py
===================================================================
--- trunk/themeengine/api.py	(revision 12163)
+++ trunk/themeengine/api.py	(working copy)
@@ -13,6 +13,11 @@
 from trac.core import *
 from trac.config import Option
 
+try:
+    from trac.util import lazy
+except ImportError:
+    lazy = None
+
 class ThemeNotFound(TracError):
     """The requested theme isn't found."""
     
@@ -67,16 +72,27 @@
     providers = ExtensionPoint(IThemeProvider)
     
     def __init__(self):
-        # This can safely go in here because the data can only change on a restart anyway
-        self.info = {}
+        if lazy is None:
+            # Trac < 0.12 : this can safely go in here because the data can 
+            # only change on a restart anyway
+            self.info = self.info()
+
+    def info(self):
+        # Trac >= 0.12 : Hack needed to deal with infinite recusrion error
+        #    Details : http://trac-hacks.org/ticket/9580#comment:1
+        info = {}
         for provider in self.providers:
             for name in provider.get_theme_names():
                 theme = provider.get_theme_info(name)
                 theme['provider'] = provider
                 theme['module'] = provider.__class__.__module__
                 theme['name'] = name
-                self.info[name.lower()] = theme
-                
+                info[name.lower()] = theme
+        return info
+
+    if lazy is not None:
+        info = lazy(info)
+
     # IThemeProvider methods
     def get_theme_names(self):
         yield 'default'

I'm using lazy instead of property because computation needs to be preformed just once . I'll commit this solution immediately after I confirm that everything will work with aforementioned versions . I've only tested modifications against Trac 0.13dev-r11046 so far . Anyway feedback is welcome .

10/14/12 06:30:26 changed by olemis

  • status changed from new to assigned.

(in reply to: ↑ 9 ; follow-up: ↓ 16 ) 10/15/12 02:04:14 changed by rjollos

Replying to olemis:

I'm proposing the following patch against trunk (see r12162 ) to make it work with Trac >= 0.11 . I'm also raising minor version number (i.e. 2.0.1 => 2.1.0 ) to delimit the exact point before and after Trac >= 0.12 compatibility .

Isn't the patch actually fixing Trac >= 1.0 compatibility? The lazy class wasn't available until 1.0 anyway, so on Trac < 1.0, lazy will evaluate to None in __init__.py, and it looks like you'll be falling back to the old way of initializing the attribute. I think this part of the patch is fine, just the comments need to be updated to change reference to 0.12 to 1.0.

I'm using lazy instead of property because computation needs to be preformed just once.

This is not an aspect of Python that I've used a lot, but can't you use lazy as a decorator, as suggest in comment:8?

(in reply to: ↑ 15 ; follow-up: ↓ 17 ) 10/15/12 05:39:31 changed by olemis

Replying to rjollos:

Replying to olemis:

I'm proposing the following patch against trunk (see r12162 ) to make it work with Trac >= 0.11 . I'm also raising minor version number (i.e. 2.0.1 => 2.1.0 ) to delimit the exact point before and after Trac >= 0.12 compatibility .

Isn't the patch actually fixing Trac >= 1.0 compatibility?

It seems plugin should fail also with Trac=0.12 , so that's something to consider as well .

The lazy class wasn't available until 1.0 anyway, so on Trac < 1.0, lazy will evaluate to None in __init__.py, and it looks like you'll be falling back to the old way of initializing the attribute.

Yes I just checked few minutes ago and it seems lazy is not available in 0.12 . So I'll wait for a while before commiting, at least until somebody actually confirms whether this bug can still be reproduced with Trac=0.12 or not .

I think this part of the patch is fine, just the comments need to be updated to change reference to 0.12 to 1.0.

The solution should cover them all 0.11 , 0.12 and 1.0 . Besides I don't want to include copies of lazy nor any other function / class / ... defined in Trac .

I'm using lazy instead of property because computation needs to be preformed just once.

This is not an aspect of Python that I've used a lot, but can't you use lazy as a decorator, as suggest in comment:8?

No , because that will fail for Trac<=0.11 .

(in reply to: ↑ 16 ) 10/15/12 23:13:55 changed by rjollos

Replying to olemis:

This is not an aspect of Python that I've used a lot, but can't you use lazy as a decorator, as suggest in comment:8?

No , because that will fail for Trac<=0.11 .

Okay, yes, I should have noticed that. But you must mean Trac < 1.0, because as you mentioned earlier, the lazy class was introduced in 1.0.

10/16/12 00:12:19 changed by rjollos

I've tested out the ThemeEnginePlugin with PyTppThemePlugin, and ThemeEnginePlugin 2.0.1 seems to work fine on Trac 0.12. I've also tested after the patch for ThemeEnginePlugin 2.1.0 has been applied and that works fine with Trac 0.12. t:#9418 has the milestone of 1.0 and none of the changes were merged to 0.12-stable, so I think this all makes sense. The issue does not seem to exist in Trac 0.12, and was introduced with Trac 1.0.

Finally, I've tested with Trac 1.0 just to confirm that I can reproduce the issue and wasn't overlooking something when testing with Trac 0.12. I don't see how I could be overlooking the presence of this issue ;)

My only suggestion is to change the two comments in the patch to make reference to Trac 1.0 rather than Trac 0.12.

  • trunk/themeengine/api.py

    old new  
    1313from trac.core import * 
    1414from trac.config import Option 
    1515 
     16try: 
     17    from trac.util import lazy 
     18except ImportError: 
     19    lazy = None 
     20 
    1621class ThemeNotFound(TracError): 
    1722    """The requested theme isn't found.""" 
    1823     
     
    6772    providers = ExtensionPoint(IThemeProvider) 
    6873     
    6974    def __init__(self): 
    70         # This can safely go in here because the data can only change on a restart anyway 
    71         self.info = {} 
     75        if lazy is None: 
     76            # Trac < 1.0: this can safely go in here because the data can  
     77            # only change on a restart anyway 
     78            self.info = self.info() 
     79 
     80    def info(self): 
     81        # Trac >= 1.0 : Hack needed to deal with infinite recusrion error 
     82        #    Details : http://trac-hacks.org/ticket/9580#comment:1 
     83        info = {} 
    7284        for provider in self.providers: 
    7385            for name in provider.get_theme_names(): 
    7486                theme = provider.get_theme_info(name) 
    7587                theme['provider'] = provider 
    7688                theme['module'] = provider.__class__.__module__ 
    7789                theme['name'] = name 
    78                 self.info[name.lower()] = theme 
    79                  
     90                info[name.lower()] = theme 
     91        return info 
     92 
     93    if lazy is not None: 
     94        info = lazy(info) 
     95 
    8096    # IThemeProvider methods 
    8197    def get_theme_names(self): 
    8298        yield 'default' 

(follow-up: ↓ 16 ) 10/16/12 04:37:04 changed by olemis

  • status changed from assigned to closed.
  • resolution set to fixed.

(In [12173]) ThemeEnginePlugin: Fixes #9580 infinite recursion issue in Trac=1.0


Add/Change #9580 (Recursion after installing in Trac 0.13 (dev))




Change Properties
Action