[Twisted-Python] "twistd" in Twisted 16.4.0 can't import modules/packages from current working directory

[Yuri]

Hi all

I couldn't find Twisted-specific group, so posting here.

Recently Twisted 16.4.0 got released. Yesterday I've tried to upgrade it
for my apps and got an error.

I've created simple example, which demonstrates it.

File service.tac:
=======================================
import os
from twisted.application import service, internet

import mymodule

application = service.Application("Demo application")
----------------------------------------


File mymodule.py:
========================================
def myfunction(asd):
""" Stub function """
----------------------------------------


If you try to run it with twistd -y service.tac you'll get an error:
== output ==============================
Unhandled Error
Traceback (most recent call last):
File
"/usr/local/lib/python2.7/site-packages/twisted/application/app.py",
line 648, in run
runApp(config)
File
"/usr/local/lib/python2.7/site-packages/twisted/scripts/twistd.py", line
25, in runApp
_SomeApplicationRunner(config).run()
File
"/usr/local/lib/python2.7/site-packages/twisted/application/app.py",
line 379, in run
self.application = self.createOrGetApplication()
File
"/usr/local/lib/python2.7/site-packages/twisted/application/app.py",
line 444, in createOrGetApplication
application = getApplication(self.config, passphrase)
--- <exception caught here> ---
File
"/usr/local/lib/python2.7/site-packages/twisted/application/app.py",
line 455, in getApplication
application = service.loadApplication(filename, style, passphrase)
File
"/usr/local/lib/python2.7/site-packages/twisted/application/service.py",
line 411, in loadApplication
passphrase)
File
"/usr/local/lib/python2.7/site-packages/twisted/persisted/sob.py", line
223, in loadValueFromFile
eval(codeObj, d, d)
File "service.tac", line 7, in <module>
import mymodule
exceptions.ImportError: No module named mymodule


Failed to load application: No module named mymodule
----------------------------------------


The errors comes down to this: twistd script does not add current
working directory to python path (or removes it, I don't know what
exactly happens), so it fails to import any packages/modules from it.
The issue does not appear in previous version (Twisted 16.3.2).

Any ideas what caused it?

[Yuri]

Post by Yuri
I couldn't find Twisted-specific group, so posting here.

Sorry, obviously this IS Twisted-specific group. I've copied the message
from python list, where I initially posted it and was redirected to this
group.

[Amber "Hawkie" Brown]

Post by Yuri
Hi all
I couldn't find Twisted-specific group, so posting here.
Recently Twisted 16.4.0 got released. Yesterday I've tried to upgrade it for my apps and got an error.
...
The errors comes down to this: twistd script does not add current working directory to python path (or removes it, I don't know what exactly happens), so it fails to import any packages/modules from it. The issue does not appear in previous version (Twisted 16.3.2).
Any ideas what caused it?

Yes -- we moved to using setuptools console scripts, and these console scripts don't add "." to the PYTHONPATH. We realised this in prerelease but decided against fixing it, as it adding the current working dir to the PATH has lead to a lot of subtle bugs in the past and this is a good chance to make a break from them.

So, in short, this is expected behaviour -- we generally want people to be running twistd, trial, etc on *installed* Python packages -- testing or running from checkouts often hides many bugs about what is or isn't included in the installed package by accident. If you rely on this behaviour, though, set the PYTHONPATH environment variable to "." -- e.g. `env PYTHONPATH=. twistd -n myplugin`.

- Amber

[Jean-Paul Calderone]

On Thu, Sep 1, 2016 at 11:36 AM, Amber "Hawkie" Brown <

Post by Yuri

Post by Yuri
Hi all
I couldn't find Twisted-specific group, so posting here.
Recently Twisted 16.4.0 got released. Yesterday I've tried to upgrade it

for my apps and got an error.

Post by Yuri
...
The errors comes down to this: twistd script does not add current

working directory to python path (or removes it, I don't know what exactly
happens), so it fails to import any packages/modules from it. The issue
does not appear in previous version (Twisted 16.3.2).

Post by Yuri
Any ideas what caused it?

Yes -- we moved to using setuptools console scripts, and these console
scripts don't add "." to the PYTHONPATH. We realised this in prerelease but
decided against fixing it, as it adding the current working dir to the PATH
has lead to a lot of subtle bugs in the past and this is a good chance to
make a break from them.
So, in short, this is expected behaviour -- we generally want people to be
running twistd, trial, etc on *installed* Python packages -- testing or
running from checkouts often hides many bugs about what is or isn't
included in the installed package by accident. If you rely on this
behaviour, though, set the PYTHONPATH environment variable to "." -- e.g.
`env PYTHONPATH=. twistd -n myplugin`.

FWIW, as a user, it would have been nice to have a NEWS entry for this. It
would have been easier to discover there than searching through the mail
archives.

Thanks,
Jean-Paul

[Glyph Lefkowitz]

Post by Amber "Hawkie" Brown

Post by Yuri
Hi all
I couldn't find Twisted-specific group, so posting here.
Recently Twisted 16.4.0 got released. Yesterday I've tried to upgrade it for my apps and got an error.
...
The errors comes down to this: twistd script does not add current working directory to python path (or removes it, I don't know what exactly happens), so it fails to import any packages/modules from it. The issue does not appear in previous version (Twisted 16.3.2).
Any ideas what caused it?

Yes -- we moved to using setuptools console scripts, and these console scripts don't add "." to the PYTHONPATH. We realised this in prerelease but decided against fixing it, as it adding the current working dir to the PATH has lead to a lot of subtle bugs in the past and this is a good chance to make a break from them.
So, in short, this is expected behaviour -- we generally want people to be running twistd, trial, etc on *installed* Python packages -- testing or running from checkouts often hides many bugs about what is or isn't included in the installed package by accident. If you rely on this behaviour, though, set the PYTHONPATH environment variable to "." -- e.g. `env PYTHONPATH=. twistd -n myplugin`.
FWIW, as a user, it would have been nice to have a NEWS entry for this. It would have been easier to discover there than searching through the mail archives.

Yes, there definitely should have been. We only realized the implications of the change after the fact, and only after some discussion decided that it was in fact desirable and should not be rolled back in a patch release.

We don't have a process for fixing NEWS files right now, and given that releases are immutable, I wonder if there's any way we could. So, in the rare case that we mess up this way in the future, what (aside from the mailing list) would be a good communication mechanism to users?

-glyph

[Amber "Hawkie" Brown]

Post by Glyph Lefkowitz

Post by Amber "Hawkie" Brown

Post by Yuri
Hi all
I couldn't find Twisted-specific group, so posting here.
Recently Twisted 16.4.0 got released. Yesterday I've tried to upgrade it for my apps and got an error.
...
The errors comes down to this: twistd script does not add current working directory to python path (or removes it, I don't know what exactly happens), so it fails to import any packages/modules from it. The issue does not appear in previous version (Twisted 16.3.2).
Any ideas what caused it?

Yes -- we moved to using setuptools console scripts, and these console scripts don't add "." to the PYTHONPATH. We realised this in prerelease but decided against fixing it, as it adding the current working dir to the PATH has lead to a lot of subtle bugs in the past and this is a good chance to make a break from them.
So, in short, this is expected behaviour -- we generally want people to be running twistd, trial, etc on *installed* Python packages -- testing or running from checkouts often hides many bugs about what is or isn't included in the installed package by accident. If you rely on this behaviour, though, set the PYTHONPATH environment variable to "." -- e.g. `env PYTHONPATH=. twistd -n myplugin`.
FWIW, as a user, it would have been nice to have a NEWS entry for this. It would have been easier to discover there than searching through the mail archives.

Yes, there definitely should have been. We only realized the implications of the change after the fact, and only after some discussion decided that it was in fact desirable and should not be rolled back in a patch release.
We don't have a process for fixing NEWS files right now, and given that releases are immutable, I wonder if there's any way we could. So, in the rare case that we mess up this way in the future, what (aside from the mailing list) would be a good communication mechanism to users?
-glyph

We have, in the past, fixed up historic NEWS files in later releases (e.g. the one which removed 2.6 support). We could always roll these changes into a post1, but, that seems like a lot of effort. Maybe we could put errata on the blog?

- Amber

[Jean-Paul Calderone]

On Tue, Jan 3, 2017 at 3:22 PM, Amber "Hawkie" Brown <

Post by Amber "Hawkie" Brown
We have, in the past, fixed up historic NEWS files in later releases (e.g.
the one which removed 2.6 support). We could always roll these changes into
a post1, but, that seems like a lot of effort. Maybe we could put errata on
the blog?

With a link to the blog post in the NEWS file? Cold checking blog was
below "read git history and try to correlate with the issue tracker" on my
list of how to track down this change.

Jean-Paul

[Jean-Paul Calderone]

Note there is still some confusion over this matter. See <
http://twistedmatrix.com/trac/ticket/8972>, <
http://twistedmatrix.com/trac/ticket/8978>, and <
https://github.com/twisted/twisted/pull/672>.

Jean-Paul

On Tue, Jan 3, 2017 at 3:24 PM, Jean-Paul Calderone <

Post by Jean-Paul Calderone
On Tue, Jan 3, 2017 at 3:22 PM, Amber "Hawkie" Brown <

Post by Amber "Hawkie" Brown
We have, in the past, fixed up historic NEWS files in later releases
(e.g. the one which removed 2.6 support). We could always roll these
changes into a post1, but, that seems like a lot of effort. Maybe we could
put errata on the blog?

With a link to the blog post in the NEWS file? Cold checking blog was
below "read git history and try to correlate with the issue tracker" on my
list of how to track down this change.
Jean-Paul

[Glyph Lefkowitz]

Thanks for highlighting those. I've put the link in the other direction as well.

Note there is still some confusion over this matter. See <http://twistedmatrix.com/trac/ticket/8972 <http://twistedmatrix.com/trac/ticket/8972>>, <http://twistedmatrix.com/trac/ticket/8978 <http://twistedmatrix.com/trac/ticket/8978>>, and <https://github.com/twisted/twisted/pull/672 <https://github.com/twisted/twisted/pull/672>>.
Jean-Paul
We have, in the past, fixed up historic NEWS files in later releases (e.g. the one which removed 2.6 support). We could always roll these changes into a post1, but, that seems like a lot of effort. Maybe we could put errata on the blog?
With a link to the blog post in the NEWS file? Cold checking blog was below "read git history and try to correlate with the issue tracker" on my list of how to track down this change.
Jean-Paul
_______________________________________________
Twisted-Python mailing list
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

[Jean-Paul Calderone]

Post by Glyph Lefkowitz
Thanks for highlighting those. I've put the link in the other direction as well.

Craig seems eager to go ahead with reverting this change in behavior.

https://github.com/twisted/twisted/pull/672#issuecomment-275956265

As far as I can tell, no one has weighed in on the other side. So I'm
inclined to go along with the reversion.

Jean-Paul

[Glyph Lefkowitz]

Post by Glyph Lefkowitz
Thanks for highlighting those. I've put the link in the other direction as well.
Craig seems eager to go ahead with reverting this change in behavior.
https://github.com/twisted/twisted/pull/672#issuecomment-275956265 <https://github.com/twisted/twisted/pull/672#issuecomment-275956265>
As far as I can tell, no one has weighed in on the other side. So I'm inclined to go along with the reversion.

My 2¢ for the other side is: if trial does this, but twist and twistd don't, then it will be possible to get a passing test run for a plugin that doesn't get loaded. I think it would be simpler and easier to debug to leave these consistent.

(I really don't think we should put this behavior back into twist or twistd, because "cd malware; twist web --path ." should not pwn your machine.)

-glyph

[Craig Rodrigues]

Post by Jean-Paul Calderone

Post by Glyph Lefkowitz
Thanks for highlighting those. I've put the link in the other direction as well.

Craig seems eager to go ahead with reverting this change in behavior.
https://github.com/twisted/twisted/pull/672#issuecomment-275956265
As far as I can tell, no one has weighed in on the other side. So I'm
inclined to go along with the reversion.
My 2¢ for the other side is: if trial does this, but twist and twistd
don't, then it will be possible to get a passing test run for a plugin that
doesn't get loaded. I think it would be simpler and easier to debug to
leave these consistent.

This is an interesting corner case, but I think the twistd and twist issues
should be pursued in
separate discussions and tickets.

For trial, I would like to proceed with
https://github.com/twisted/twisted/pull/672/ . With my conversion
of trial to a console script, the new behavior was unintentional on my
part. Since all the unit tests
passed, I did not notice. In this ticket:
https://twistedmatrix.com/trac/ticket/8978
Job-Evers-Meltzer provided a use-case where the new behavior of trial
broke existing usage.

trial can be used as a general-purpose tool for running unittest-style
tests,
much like pytest or nose. The new behavior is disruptive, for minimal
benefit,
so I would like to restore the old behavior.

I added a unit test for this, so in future, if this breaks again, we will
catch it.

--
Craig

[Glyph Lefkowitz]

Post by Glyph Lefkowitz

Post by Glyph Lefkowitz
Thanks for highlighting those. I've put the link in the other direction as well.
Craig seems eager to go ahead with reverting this change in behavior.
https://github.com/twisted/twisted/pull/672#issuecomment-275956265 <https://github.com/twisted/twisted/pull/672#issuecomment-275956265>
As far as I can tell, no one has weighed in on the other side. So I'm inclined to go along with the reversion.

My 2¢ for the other side is: if trial does this, but twist and twistd don't, then it will be possible to get a passing test run for a plugin that doesn't get loaded. I think it would be simpler and easier to debug to leave these consistent.
This is an interesting corner case, but I think the twistd and twist issues should be pursued in
separate discussions and tickets.
For trial, I would like to proceed with https://github.com/twisted/twisted/pull/672/ <https://github.com/twisted/twisted/pull/672/> . With my conversion
of trial to a console script, the new behavior was unintentional on my part. Since all the unit tests
passed, I did not notice. In this ticket: https://twistedmatrix.com/trac/ticket/8978 <https://twistedmatrix.com/trac/ticket/8978>
Job-Evers-Meltzer provided a use-case where the new behavior of trial
broke existing usage.
trial can be used as a general-purpose tool for running unittest-style tests,
much like pytest or nose. The new behavior is disruptive, for minimal benefit,
so I would like to restore the old behavior.
I added a unit test for this, so in future, if this breaks again, we will catch it.

There's a lot of controversy around this type of organization of tests; personally, I believe it is a horrible antipattern, others, notably Donald Stufft, (wrongly) regard it as a best practice.

Apropos of the comment I put on 9035, https://twistedmatrix.com/trac/ticket/9035#comment:4 <https://twistedmatrix.com/trac/ticket/9035#comment:4>, would it be acceptable for Job Evers‐Meltzer if the syntax were simply 'trial ./tests/' instead?

-glyph

[Mark Williams]

Post by Yuri
The errors comes down to this: twistd script does not add current working
directory to python path (or removes it, I don't know what exactly happens),
so it fails to import any packages/modules from it. The issue does not
appear in previous version (Twisted 16.3.2).
Any ideas what caused it?

The commits that closed this ticket:
http://twistedmatrix.com/trac/ticket/8491

Generally:
https://github.com/twisted/twisted/commit/c5a4c635de259cd1b92a555c930aa426164f9cce
Specifically:
https://github.com/twisted/twisted/commit/c5a4c635de259cd1b92a555c930aa426164f9cce#diff-a7270bb5043e420fd7a98b81e48ac082

The bin/twistd script would add the current working directory to
sys.path prior to running the actual twistd logic. This was not
ported over.

I'm going to say that was a conscious decision, as it coincided with
moving twisted to a src/ layout and a fair bit of discussion (visible
in that ticket) about how these changes have made trial only discover
installed code and not whatever's in the current working directory.

To a developer of Twisted it's clear that the project's trying to
replace magical code discovery with boring code importing. To a user,
that's not clear, and we need to clearly document this.

Being a Twisted developer, I do think in general that it's better to
install your code into an existing sys.path entry instead of adding a
sys.path entry that contains your code.

Would this be difficult for your application, and if so, why? If it's
possible to make it easier for your application I'd like to help.

Thanks for using Twisted!

-Mark

[Yuri]

Thanks for swift response!

I do not tend to install my applications. Never did that to be honest,
and don't know what kind of issues to expect. Definitely should try it.

For now workaround will be fine.

Just wanted to confirm that it's an intended behavior and find out the
reasons why that was changed.