twisted.python.versions uses implementation detail of SVN, broken on 1.4

[jknight]

The file format of the entries file has changed in svn 1.4, breaking t.p.versions, thus making twisted not even start up when run from a svn 1.4 directory.

It should use a public API, instead, to get this info. I suggest the svnversion command. And now I can say "I told you so", since, well, I did. :)

Alternatives if you don't like svnversion for some reason are the svn python bindings or svn status -v -N --xml.

[jknight]

Oh, sorry, this looks like it was radix's fault. Reassigning to him.

[exarkun]

Haha radix you jerk.

[exarkun]

Wait what I meant to type there was:

svnversion? svn status? So importing Twisted would launch a subprocess?

[radix]

exarkun seems to think that running a subprocess when twisted is imported is a terrible idea and will never ever ever happen.

[warner]

Why is it important that intermediate versions know precisely which SVN revision they are from?

My habit with buildbot is to set buildbot/init.py's version= string equal to something like "0.7.4+" while I'm in-between releases, so people know they aren't dealing with a real release, and then letting them look at the ChangeLog or something if they really care which intermediate release they're using. (also, buildbot lives in Darcs, so it's not like there's a notion of "revision number" anyways).

Would this be insufficient? I'm finding it a bit of a nuisance to not be able to run Twisted from SVN at all right now, on my debian/unstable system which moved to svn-1.4.0 recently.

[therve]

One way to achieve the same functionality would be to make a post-commit hook in the subversion server that set a __revision__ value in twisted.__init__.py. Not trivial to do, but it could solve the problem.

[titty]

why is running an xml parser when twisted is imported not a terrible idea? the svnversion will differ for people who mantain their own copy of twisted in their own repositories (or will even not be available when people install their twisted with python setup.py install). in prior days it was possible to change the twisted version for people mantaining their own copy (i.e twisted-1.3.0-titty-7). this is not possible anymore with twisted.python.versions. too me it seems like this file is a bit overengineered.

[titty]

and why did no one care for this bug for 2 months? in case none of you guys have subversion 1.4 installed:

$ python -m twisted
Traceback (most recent call last):
  File "/exp/lib/python2.5/runpy.py", line 95, in run_module
    filename, loader, alter_sys)
  File "/exp/lib/python2.5/runpy.py", line 52, in _run_module_code
    mod_name, mod_fname, mod_loader)
  File "/exp/lib/python2.5/runpy.py", line 32, in _run_code
    exec code in run_globals
  File "/home/ralf/tst/bbot/extern/Twisted/twisted/__init__.py", line 26, in <module>
    from twisted.python import compat
  File "twisted/__init__.py", line 31, in <module>
    __version__ = version.short()
  File "twisted/python/versions.py", line 37, in short
    svnver = self._getSVNVersion()
  File "twisted/python/versions.py", line 90, in _getSVNVersion
    doc = parse(file(ent)).documentElement
  File "/exp/lib/python2.5/site-packages/_xmlplus/dom/minidom.py", line 1915, in parse
    return expatbuilder.parse(file)
  File "/exp/lib/python2.5/site-packages/_xmlplus/dom/expatbuilder.py", line 930, in parse
    result = builder.parseFile(file)
  File "/exp/lib/python2.5/site-packages/_xmlplus/dom/expatbuilder.py", line 207, in parseFile
    parser.Parse(buffer, 0)
xml.parsers.expat.ExpatError: syntax error: line 1, column 0

but i guess I should have written a unit test instead... this just plain sucks.

[jknight]

There's still svn python bindings...nobody has said they don't like that yet.

But, I think it's probably best to just remove the svn version finding stuff if nobody can agree on a way to do this with public svn APIs. It's not a critical feature, but it is pretty critical that twisted doesn't even work.

[jerub]

While this bug is being fixed, it would be good if it were fixed in such a way that if the platform doesn't have a working xml parser, this doesn't blow up either. (pypy problem, see #2076)

[jknight]

#2106 closed as duplicate.

[wsanchez]

One downside to the bindings is that they aren't built and installed by default. So you might have svn without the bindings on your system.

In any case, not knowing the svn version shouldn't light the house on fire.

[wsanchez]

See  http://divmod.org/trac/ticket/1536 for a related fix to Combinator.

[wsanchez]

Use svnversion command

[wsanchez]

For those of us that need to continue working while we debate the merits of various options, I've attached svnversion.diff which is a patch to use the svnversion command, which should work fine for those of us with svn in our PATH.

At least, I can launch my server again.

[jerub]

Yay for having a patch, boo for having a patch that will cause this to blow up if svnversion isn't in the path.

I propose a similar patch that will silence any errors (such as op.popen being an AttributeError under pypy) would be a suitable fix for this issue.

[wsanchez]

Yep.  As I said, this is for simply getting work done which we argue about the merits of forking vs. bindings, etc.  If we were to use that patch as a starting point, I'd wrap the code in a try/except clause that returned None on error and simply live without the information.  I'd rather launch without version information than crash.

[titty]

If we've already reached the point where it is ok to start a subprocess in order to get the svn version when importing twisted, I propose the following:

Let the versions.Version's methods return deferreds instead of the real values and use twisted.internet.process to start the svnversion command lazily. This has some advantages:

1. "import twisted" will not block on os.popen("svnversion ..."). The same applies to using the version object for other packages than twisted.

2. the version number isn't computed everytime twisted is imported. most of the time no one needs it anyway.

The current version of svnversion.diff doesn't include unit tests. It also does not cache the values returned by _getSVNVersion. Is this a design choice? It would start to execute svnversion everytime I want to get a string representation of the version??

[wsanchez]

Sorry, to be clear, there were no design choices in that patch.  The goal of that patch was to enable me to continue doing work; not being able to launch twisted at all any more wasn't really working for me.  It's a hack, posted in case anyone else needs to, you know, run the software also.  I personally don't care whether this feature works at all; I don't need to know the version from svn, and never thought about it.

The patch could easily become a non-hack by handling errors and caching the result, yes, but it sounded like forking a process was going to raise objections, so I didn't want to spend time on a design that wasn't going to be acceptable long-term.  Forking won't work well for Windows users, for example, who don't have the command line client installed.

Unfortunately, I don't know of any great options here; using the bindings has its problems as well.  Now this is a developer-only issue, since we're talking about versions from svn, and not releases, so maybe a solution that only works if you have the bindings installed is fine.

[titty]

Replying to wsanchez:

Sorry, to be clear, there were no design choices in that patch. The goal of that patch was to enable me to continue doing work; not being able to launch twisted at all any more wasn't really working for me. It's a hack, posted in case anyone else needs to, you know, run the software also. I personally don't care whether this feature works at all; I don't need to know the version from svn, and never thought about it.

I also don't need this version, and if someone needs it, they should just run the svnversion command. This particular functionality should just be removed. Instead I'd like to be able to do basic things like comparing the twisted version number:

In [12]: twisted.version        
Out[12]: Version('twisted', 2, 4, 0)  # (SVN r25018)

In [13]: twisted.version>=(2,3,0)
Out[13]: False

In [14]: from twisted.python.versions import Version

In [15]: needed = Version("twisted", 2,3,0)

In [16]: twisted.version>needed
Out[16]: True

In [17]: needed
Out[17]: Version('twisted', 2, 3, 0)  # (SVN r25018)

Comparing with a simple tuple does not work, instead I have to import twisted.python.versions, create a Versions object (which somehow magically is svn revision 25018) and compare that against twisted.version. I just do not understand why someone cares to implement that svn version number parsing, and then is not able or willing to fix this bug for 2 months. But then I guess this is just how the "Ultimate Quality Development System" works.

The patch could easily become a non-hack by handling errors and caching the result, yes, but it sounded like forking a process was going to raise objections, so I didn't want to spend time on a design that wasn't going to be acceptable long-term. Forking won't work well for Windows users, for example, who don't have the command line client installed.

Running svnversion implies returning a deferred from Version's methods.

Unfortunately, I don't know of any great options here; using the bindings has its problems as well. Now this is a developer-only issue, since we're talking about versions from svn, and not releases, so maybe a solution that only works if you have the bindings installed is fine.

I know a great option: remove _getSVNVersion

[exarkun]

This is a feature for Twisted developers. It's to let us determine what version of Twisted is being used, so we can more easily address bug reports.

Running svnversion separately is a degredation in functionality, since most people won't do this in their initial bug report, and many people never make even a single followup comments on a bug reporter.

As a feature for developers, this isn't as high priority to fix as issues which affect released versions of Twisted. If you cannot deal with this, then you should be using a released' version of Twisted.

If you need to use a development version of Twisted, you should be prepared to address development issues it has. Personally, I have not fixed this bug because I'm not using svn 1.4. As soon as I need to use svn 1.4, I imagine I will fix this.

What I don't understand is why you haven't fixed this yet, since apparently it is absolutely critical for you to use svn 1.4 alongside Twisted trunk@HEAD. Instead, you waste time posting annoying comments to this issue tracker.

Your issues with Version comparison against tuples are unrelated to this issue. If you would like to request a feature, open a separate ticket.

Your issues with UQDS are irrelevant. Please restrict yourself to productive commentary here. If you want to engage in Twisted bashing, there are plenty of other forums to do so.

[spiv]

Out of interest, have we ever had a bug report that used this information? I can't recall seeing any. I'm personally in favour of just removing the feature, or at least making it fail silently. I don't think I'd miss it. I didn't even know it existed.

[exarkun]

The feature is only a few months old, which is probably why you didn't know about it.

Failing silently in the face of an unrecognizable configuration, however, is perfectly acceptable to me. It is an aid, not a critical piece of information.

[titty]

Replying to exarkun:

This is a feature for Twisted developers. It's to let us determine what version of Twisted is being used, so we can more easily address bug reports.

Version('twisted', 2, 4, 0) # (SVN r25018)

Which version am I running?

Running svnversion separately is a degredation in functionality, since most people won't do this in their initial bug report, and many people never make even a single followup comments on a bug reporter. As a feature for developers, this isn't as high priority to fix as issues which affect released versions of Twisted. If you cannot deal with this, then you should be using a released' version of Twisted.

which also fails miserably when put into a svn repo.

If you need to use a development version of Twisted, you should be prepared to address development issues it has. Personally, I have not fixed this bug because I'm not using svn 1.4. As soon as I need to use svn 1.4, I imagine I will fix this. What I don't understand is why you haven't fixed this yet, since apparently it is absolutely critical for you to use svn 1.4 alongside Twisted trunk@HEAD. Instead, you waste time posting annoying comments to this issue tracker.

That's because this particular bug annoys me as well...everyone using svn 1.4 has to fix in in their own copy of twisted, and later resolve conflicts when you decide to switch to svn 1.4 and this bug is being fixed. this sucks.

Your issues with Version comparison against tuples are unrelated to this issue. If you would like to request a feature, open a separate ticket. Your issues with UQDS are irrelevant. Please restrict yourself to productive commentary here. If you want to engage in Twisted bashing, there are plenty of other forums to do so.

I'm not practicing UQDS so I have no issues with it. I have issues with easy to fix bugs not being fixed. My guess is that this particular bug not being fixed, is a symptom of UQDS (lack of reviewers? lack of unit tests?).

[exarkun]

Replying to titty:

I'm not practicing UQDS so I have no issues with it. I have issues with easy to fix bugs not being fixed. My guess is that this particular bug not being fixed, is a symptom of UQDS (lack of reviewers? lack of unit tests?).

No one has even tried to fix this in a reasonable way. This is an issue of lack of developer resources, nothing else.

[titty]

$ svn diff
Index: twisted/python/versions.py
===================================================================
--- twisted/python/versions.py	(revision 18209)
+++ twisted/python/versions.py	(working copy)
@@ -88,6 +88,8 @@
 
         @return: None or string containing SVN Revision number.
         """
+        return None
+    
         mod = sys.modules.get(self.package)
         if mod:
             ent = os.path.join(os.path.dirname(mod.__file__),

bye.

[radix]

handle parse errors gracefully

[radix]

PATCH is a patch for your perusal. Please review.

[jerub]

Okay! good. No subprocess stuff going on, a 'catch all' exception will make this nice and neat. Won't work with svn 1.4, but I think I am at peace with that.

'screw you' is inappropriate in the test. Please change this to. "A String That Is Not XML" or anything else that is in no way profane.

Change the test, make sure it passes and please merge. I'll be glad to have this bug closed.

[radix]

(In [18214]) Apply patch from #1968 (with less cussing)

Author: radix Reviewer: jerub Fixes #1968

This makes it so twisted can be imported when used in an svn 1.4-generated checkout. It does this by ignoring parse errors and reporting the SVN revision as "unknown" when such an error occurs.