Opened 12 years ago

Closed 9 years ago

#3526 defect closed fixed (fixed)

The running-from-SVN header is buggy

Reported by: Thijs Triemstra Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: core Keywords:
Cc: Jean-Paul Calderone, Thijs Triemstra, jesstess, ivank Branch: branches/running-from-svn-3526-2
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

The running-from-SVN header is buggy because it assumes a sub-project bin directory. Scripts for twisted core (twistd, trial) need one fewer reference to os.pardir.

Attachments (1)

normalize.preamble.patch (15.9 KB) - added by anatoly techtonik 9 years ago.
alt.patch

Download all attachments as: .zip

Change History (51)

comment:1 Changed 12 years ago by Jean-Paul Calderone

I'm pretty sure the ticket description is inaccurate. However, for subproject scripts, it needs one more os.pardir.

comment:2 Changed 12 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/scripts-3526

(In [25396]) Branching to 'scripts-3526'

comment:3 Changed 12 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added
Keywords: review added
Owner: Glyph deleted

I put an idea into the branch. Some things to consider:

  1. Do we actually care about this functionality, or can we throw it away?
  2. Can we make this work properly with packaging? How?
  3. How much testing is required?

comment:4 in reply to:  3 ; Changed 12 years ago by Glyph

Keywords: review removed
Owner: set to Jean-Paul Calderone

Replying to exarkun:

I put an idea into the branch. Some things to consider:

  1. Do we actually care about this functionality, or can we throw it away?

The functionality that I care about here is a new user being able to get started with Twisted, having nothing but the tarball, and not installing it; not setting any environment variables or hacking sys.path.

The preamble, as such, may not be sufficient. Nowadays, Twisted depends on zope.interface, and neither the checkout nor the downloadable tarball includes an importable version of zi, so the user must download and install that before anything would work. I'd prefer that this work, but making this work right in the general case is basically making easy_install work correctly, then making Twisted work with it, and there's a lot of ground between here and there.

Unless we get rid of subproject subdirectories, there's no particular reason to keep this code there.

The other functionality bin/ provides, in kind of a backwards way (since it predates combinator) is to allow combinator to recognize scripts that are part of Twisted and make it work when I type twistd or trial at the command line. We could easily remove the preamble without breaking that.

  1. Can we make this work properly with packaging? How?

I'm not really sure what you mean by "properly". Certainly you can't put a _preamble.py into /usr/bin.

My inclination would be to make trunk/bin be a convenience only for development from a checkout, and generate the actual scripts/ or bin/ from separate metadata when the releases are generated. The shebang lines, for example, should be shebang lines for whatever interpreter is used to do the installation, not hard-coded to /usr/bin/python. And on Windows there shouldn't even be a bin/, there should be a scripts/.

  1. How much testing is required?

I think this depends on what the actual requirements for the purpose of these files is. I have made some tools that work based on their presence, but I frankly have no idea how they relate to the release toolchain. Someone else who has an idea of why they are as they are now, rather than the reason that the bin/ directory was added in the first place, should probably comment on that before we decide.

Tests should verify that the scripts serve whatever purpose that they are supposed to serve; it wouldn't be terribly meaningful to test these without a tightly specified environment, so I think some spawnProcess-ing might be called for.

comment:5 in reply to:  4 Changed 12 years ago by Jean-Paul Calderone

Replying to glyph:

Replying to exarkun:

I put an idea into the branch. Some things to consider:

  1. Do we actually care about this functionality, or can we throw it away?

The functionality that I care about here is a new user being able to get started with Twisted, having nothing but the tarball, and not installing it; not setting any environment variables or hacking sys.path.

The preamble, as such, may not be sufficient. Nowadays, Twisted depends on zope.interface, and neither the checkout nor the downloadable tarball includes an importable version of zi, so the user must download and install that before anything would work. I'd prefer that this work, but making this work right in the general case is basically making easy_install work correctly, then making Twisted work with it, and there's a lot of ground between here and there.

Unless we get rid of subproject subdirectories, there's no particular reason to keep this code there.

The other functionality bin/ provides, in kind of a backwards way (since it predates combinator) is to allow combinator to recognize scripts that are part of Twisted and make it work when I type twistd or trial at the command line. We could easily remove the preamble without breaking that.

It sounds like we care, but the functionality isn't actually provided now, as the running-from-SVN header is not sufficient to provide it. So we should get rid of the preamble everywhere.

One other thing, though. The preamble includes code to add the working directory to sys.path - sometimes. That sounds like a different feature than the one you described (and one I've disliked for some time, due to its inconsistent availability, normal user vs root, posix vs windows).

  1. Can we make this work properly with packaging? How?

I'm not really sure what you mean by "properly". Certainly you can't put a _preamble.py into /usr/bin.

My inclination would be to make trunk/bin be a convenience only for development from a checkout, and generate the actual scripts/ or bin/ from separate metadata when the releases are generated. The shebang lines, for example, should be shebang lines for whatever interpreter is used to do the installation, not hard-coded to /usr/bin/python. And on Windows there shouldn't even be a bin/, there should be a scripts/.

This basically sounds good to me, and it's really getting into the details of what I was asking. Having trunk/bin be a developer-only convenience with some other mechanism to provide scripts for installations sounds great to me. Can we overcome the technical obstacles to accomplish that? I feel like the answer is no (at least with the current primitive packaging tools we're using).

I'm interested in finding a way to resolve this ticket without re-implementing distutils, setuptools, and aap from scratch.

comment:6 Changed 10 years ago by Jean-Paul Calderone

Branch: branches/scripts-3526branches/running-from-svn-3526

(In [29724]) Branching to 'running-from-svn-3526'

comment:7 Changed 10 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

I kinda forgot about the original branch here. That was stupid. But at least it only took about 5 minutes to re-implement everything in it. I also added tests this time.

To sum up a few things:

  1. Do we actually care about this functionality, or can we throw it away?

Yes, we care. So now it's unit (well, sort of) tested

  1. Can we make this work properly with packaging? How?

We can't because we don't make the packages. But _preamble is now imported such that if it isn't found, the script will just carry on. This should be the correct behavior for the installed case. Twisted is already in the path (that's the point of installation).

So, in particular:

Certainly you can't put a _preamble.py into /usr/bin.

doesn't matter, because we don't want to do that, and now we don't need to.

Perhaps eventually we can generate the scripts at sdist (or something?) time and leave this out entirely. That would be nice.

  1. How much testing is required?

I tested enough to demonstrate that nearby vcs checkout of Twisted is used. I didn't add any tests for the installed case, which means there's no tests to verify that the ImportError is being discarded.

It's probably possible to come up with some tests that exercise the installed case. I'll think about it a bit, in case anyone wants such tests before this is merged.

One thing that's presently not in the branch at all anymore is the special non-root handling on POSIX. I should probably put that back. But... tests... yea.

comment:8 Changed 10 years ago by Jean-Paul Calderone

(In [29843]) Update the running-from-SVN header documentation

refs #3526

comment:9 Changed 10 years ago by Jean-Paul Calderone

(In [29844]) get rid of the extra list item

refs #3526

comment:10 Changed 10 years ago by Thijs Triemstra

Cc: Thijs Triemstra radix added
Keywords: review removed
Owner: set to Jean-Paul Calderone
  • in the description it mentions 'UNIX like', this should be 'UNIX-like'
  • maybe rewrite 'operating systems we use the' to 'operating systems, we use the', so adding a comma.
  • the word 'env' in should be wrapped in <code></code>

comment:11 Changed 10 years ago by Thijs Triemstra

Cc: radix removed

comment:12 Changed 10 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

Those changes don't appear to be related to this ticket.

comment:13 in reply to:  12 ; Changed 10 years ago by Thijs Triemstra

Replying to exarkun:

Those changes don't appear to be related to this ticket.

Why not? That section describes the running-from-svn-header, I'm confused. If you don't want the docs to be improved than let me know.

comment:14 Changed 10 years ago by Screwtape

Owner: set to Screwtape

comment:15 in reply to:  13 Changed 10 years ago by Screwtape

Keywords: review removed
Owner: changed from Screwtape to Jean-Paul Calderone

Replying to thijs:

Why not? That section describes the running-from-svn-header, I'm confused. If you don't want the docs to be improved than let me know.

Looking closely, it seems that section describes module boilerplate in general; the paragraph that exarkun changed was the paragraph about running-from-SVN-header, the bit you're talking about was the previous paragraph about the hash-bang line.

As for my review, it generally looks good, except:

  1. Running tests on CentOS 5, I get crazy "error in processing external entity reference at line 239, column 2" exceptions from the Lore tests, but I rather suspect that's some misconfiguration on my end. Still, build results from py2.4 builders would be good to see.
  2. Running tests on Fedora 13, all the tests in twisted.conch.test.test_scripts fail because I don't have PyASN.1 installed. Running "bin/conch/conch --version" gives me a traceback rather than the "Twisted version: 10.1.0" that the tests are looking for. I guess you need to apply whatever .skip attribute is used to skip the rest of the Conch tests to these tests too.
  3. Needs a news file.

comment:16 Changed 10 years ago by Jean-Paul Calderone

(In [30129]) Skip these tests if a dependency (PyCrypto or pyasn1 or Tkinter) is missing

refs #3526

comment:17 Changed 10 years ago by Jean-Paul Calderone

(In [30130]) Really skip tkconch if necessary

refs #3526

comment:18 Changed 10 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

comment:19 Changed 10 years ago by jesstess

Cc: jesstess added
Keywords: review removed
Owner: set to Jean-Paul Calderone

Thanks for sticking with this, exarkun.

  • buildbot has an exciting mélange of issues.
  • bin/tapconvert didn't get a test.
  • Does _preamble.py need a copyright?

comment:20 Changed 9 years ago by <automation>

Owner: Jean-Paul Calderone deleted

Changed 9 years ago by anatoly techtonik

Attachment: normalize.preamble.patch added

alt.patch

comment:21 Changed 9 years ago by anatoly techtonik

Keywords: review added

I need this patch to run scripts on Windows from checkout of /twisted/ for further development. #5041 was marked as duplicate of this issue. Seems like this issue #3526 got overcomplicated - tl;dr, but the patch fixes original user story mentioned in description.

Please, review.

comment:22 Changed 9 years ago by Jean-Paul Calderone

Branch: branches/running-from-svn-3526branches/running-from-svn-3526-2

(In [31575]) Branching to 'running-from-svn-3526-2'

comment:23 Changed 9 years ago by anatoly techtonik

Thanks for working on this.

comment:24 Changed 9 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jean-Paul Calderone
Status: newassigned

not really in review right now, branch has an issue with setuptools interaction, the cause of which isn't quite clear

Thanks for your patch, techtonik, but I'd rather continue working on the solution in the branch than abandon it and start from scratch in a different direction ignoring all of the issues that came up earlier during development of the branch.

comment:25 Changed 9 years ago by anatoly techtonik

What's wrong with setuptools?

comment:26 Changed 9 years ago by Jean-Paul Calderone

Two tests fail in this branch when run using the setuptools console entrypoint for trial, <http://buildbot.twistedmatrix.com/builders/debian-easy-py2.5-epoll/builds/1170>. Something about how the trial script is launched causes a difference in the result of sys.exc_info() so that there is an exception returned from it when one is not expected.

comment:27 Changed 9 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted
Status: assignednew

This wasn't really a setuptools issue. It was an installed case issue. Too accustomed to finding fault with setuptools. ;)

I fixed this by adding sys.exc_clear() to the use-preamble boilerplate. Latest build results.

I also added a test for the mailmail script change, along the lines of the other script tests already added.

comment:28 Changed 9 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone
  1. Some of the test files (twisted/lore/test/test_scripts.py, twisted/mail/test/test_scripts.py, twisted/conch/test/test_scripts.py) are missing blank lines between the imports and the test class.

Looks good, please merge.

comment:29 Changed 9 years ago by Jean-Paul Calderone

(In [31709]) vertical whitespace

refs #3526

comment:30 Changed 9 years ago by Jean-Paul Calderone

on IRC ivan pointed out that this removes the sys.path.insert(0, CWD) behavior from trial and twistd.

comment:31 Changed 9 years ago by therve

Keywords: review added
Owner: Jean-Paul Calderone deleted

I did something.

comment:32 Changed 9 years ago by anatoly techtonik

This makes me wonder if trial and twisted (and maybe other scripts as well) should explicitly tell user where are they called from?

comment:33 Changed 9 years ago by ivank

Cc: ivank added
Keywords: review removed
  1. On IRC, I mentioned that this needs a NEWS entry:

twisted/topfiles/3526.bugfix: "trial and twistd now add the current directory to sys.path even when running as root or on Windows. mktap, tapconvert, and pyhtmlizer no longer add the current directory to sys.path."

  1. Please remove twisted/topfiles/3526.feature, because a refactoring of some code is not something that users care about. (It's not a new feature either.)
  1. twisted/python/dist.py's getScripts lists .pyc in specialExclusion. This neglects .pyo and Jython's $py.class. Please add those, or make it exclude everything that starts with '_preamble' (note no trailing dot). test_excludedPreamble needs to be updated too.
  1. twisted/conch/test/test_scripts.py is still missing some whitespace after the imports.

Otherwise, looks good; merge if those are fixed.

comment:34 Changed 9 years ago by ivank

techtonik: Do you mean you want every script like trial to announce "I'm in /usr/bin/trial"? The which command is handy for finding this information. Or do you mean you want them to announce sys.path? (This is pretty deterministic and probably doesn't need to be announced. Users with a lot of .egg directories from setuptools will see a lot of line noise.)

comment:35 Changed 9 years ago by anatoly techtonik

ivank: I want to know which trial is running when I execute the script. Like --version can show that trial tail is imported from \Python25\lib\something or from local checkout or from current directory.

comment:36 Changed 9 years ago by Jean-Paul Calderone

This neglects .pyo and Jython's $py.class.

This really only needs to work with the documented release process. We don't do releases with python -O or Jython.

comment:37 in reply to:  36 Changed 9 years ago by Glyph

Replying to exarkun:

This neglects .pyo and Jython's $py.class.

This really only needs to work with the documented release process. We don't do releases with python -O or Jython.

True enough, is it really a good idea to have another ad-hoc list which repeats the static '.svn' string again? It seems like we could just re-use _filterNames and get the same effect with less code.

This wouldn't fix the problem with $py.class, but it would at least give us just one list to update when we want to fix it in the future.

comment:38 Changed 9 years ago by ivank

I invite you to ignore my #3 entirely, if you wish (and maybe open a new ticket for refactorings).

comment:39 Changed 9 years ago by therve

Owner: set to therve

comment:40 Changed 9 years ago by Jean-Paul Calderone

Owner: changed from therve to Jean-Paul Calderone

There are some buildbot failures because the sys.path tests think that a Python process can reliably produce no output, but that's totally false. :)

eg, on the OS X builder, the child process produces:

/System/Library/Frameworks/Python.framework/Versions/2.6/Extras/lib/python/zope/__init__.py:1: 
    UserWarning: Module twisted was already imported from /Users/exarkun/Run/twisted-slave/osx10.6-py2.6-select/Twisted/twisted/__init__.pyc,
        but /System/Library/Frameworks/Python.framework/Versions/2.6/Extras/lib/python is being added to sys.path
  __import__('pkg_resources').declare_namespace(__name__)

comment:41 Changed 9 years ago by Jean-Paul Calderone

(In [31913]) Check stdout for some known-good output instead of verifying that there is no stderr output

refs #3526

comment:42 Changed 9 years ago by Jean-Paul Calderone

Keywords: review added

I twiddled the test a little bit. Build results.

comment:43 in reply to:  40 Changed 9 years ago by Glyph

Replying to exarkun:

There are some buildbot failures because the sys.path tests think that a Python process can reliably produce no output, but that's totally false. :)

eg, on the OS X builder, the child process produces:

... Module twisted was already imported from ... but ... is being added to sys.path ...

In case some poor soul comes across this ticket while searching for this problem (and as a potential solution for similar tests) this can be fixed, on multiple platforms, by python setup.py egg_info in the Twisted directory under test.

comment:44 Changed 9 years ago by Jean-Paul Calderone

Owner: Jean-Paul Calderone deleted

comment:45 Changed 9 years ago by Glyph

Owner: set to Glyph
Status: newassigned

Reviewing.

comment:46 Changed 9 years ago by Glyph

Keywords: review removed
Owner: changed from Glyph to Jean-Paul Calderone
Status: assignednew

Although the buildbots pass the tests for this branch, on my machine, I see this traceback (only when running the full suite):

Traceback (most recent call last):
  File "twisted/trial/runner.py", line 570, in loadPackage
    module = modinfo.load()
  File "twisted/python/modules.py", line 383, in load
    return self.pathEntry.pythonPath.moduleLoader(self.name)
  File "twisted/python/reflect.py", line 464, in namedAny
    topLevelPackage = _importAndCheckStack(trialname)
  File "twisted/python/reflect.py", line 400, in _importAndCheckStack
    return __import__(importName)
  File "twisted/mail/test/test_imap.py", line 3375, in <module>
    class NewFetchTestCase(unittest.TestCase, IMAP4HelperMixin):
  File "twisted/mail/test/test_imap.py", line 3486, in NewFetchTestCase
    locale.setlocale(locale.LC_ALL, currentLocale)
  File ".../lib/python2.6/locale.py", line 494, in setlocale
    return _setlocale(category, locale)
locale.Error: unsupported locale setting

twisted.mail.test.test_imap

This occurs because, for some reason I can't divine, importing Tkinter, on a mac, only when you are running under a GUI, causes the locale to be set to an invalid value ("en_US.UTF8", to be precise, as opposed to the correct "en_US.UTF-8"). Actually many of our uses of the locale module look to be at odds with the observed behavior on a couple of systems I've looked at. Most importantly, the value returned from getlocale can't necessarily safely be passed to setlocale, even if it's actually a supported locale. So this needs to be addressed somehow, albeit perhaps a separate ticket would be good for attacking this, since it's mostly an accident that this doesn't occur in trunk.

More relevant to the actual content of the branch: it would be nice to get rid of the subdirectories in bin/. For one thing, one reason for having a 'bin' directory is to have an entry to put on $PATH during development, and subdirectories break it. For another, we now have a preamble-preamble, which is a whole separate maintenance headache. Of course, then we'd need an addition to our release management tools to declare the separation of scripts into different projects. I think the thing to do here is to file a new ticket.

Please address these two issues to your satisfaction and land, unless the solution to the first one is complex enough to warrant a second review.

comment:47 Changed 9 years ago by Jean-Paul Calderone

The locale issue is fixed in r32199

comment:48 Changed 9 years ago by Jean-Paul Calderone

Filed #5168 for the bin/ subdirectories. I want to avoid touching it for now because it means teaching the release tools a new trick. It would definitely be a nice simplification though.

comment:49 Changed 9 years ago by Jean-Paul Calderone

(In [32201]) assertEquals -> assertEqual

refs #3526

comment:50 Changed 9 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [32202]) Merge running-from-svn-3526-2

Author: exarkun Reviewer: glyph, Screwtape, jesstess, therve, ivank Fixes: #3526

Factor the duplicate sys.path manipulation done by the command line scripts to share a single implementation.

Also change trial and twistd to always add the current working directory to sys.path (previously it would be added unless running as root or on Windows).

Note: See TracTickets for help on using tickets.