New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decorator stacking problems and @task #410

Closed
bitprophet opened this Issue Aug 19, 2011 · 11 comments

Comments

Projects
None yet
2 participants
@bitprophet
Member

bitprophet commented Aug 19, 2011

Description

This is at least a documentation problem, if not a code one -- using @task "inside" other decorators such as @hosts doesn't work well, and @task must thus come at the top of the decorator 'list'. See this ML thread for details.

See also Erich Heine's post in that thread suggesting a way of normalizing the various decorators so this becomes less of a problem. It sounds a lot like Travis' idea of preemtively wrapping classic-style tasks in class tasks so that we can start assuming Task objects in the rest of the system.

EDIT: I think it makes more sense to defer this to 1.4/1.5 and correctly normalize things, instead of adding a doc/FAQ change that will become obsolete soon after.


Originally submitted by Jeff Forcier (bitprophet) on 2011-08-11 at 02:32pm EDT

@dcolish

This comment has been minimized.

Show comment
Hide comment
@dcolish

dcolish Feb 2, 2012

I'm not seeing issues with stacking order when using the hosts and task decorators. Is there a clear failure case? Its not obvious from the ML thread. Could this have been already fixed inadvertently?

dcolish commented Feb 2, 2012

I'm not seeing issues with stacking order when using the hosts and task decorators. Is there a clear failure case? Its not obvious from the ML thread. Could this have been already fixed inadvertently?

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Feb 2, 2012

Member

It seems to make the tasks not show up as valid tasks, i.e. they're not in --list nor can they be specified on the CLI or called with execute('name').

Member

bitprophet commented Feb 2, 2012

It seems to make the tasks not show up as valid tasks, i.e. they're not in --list nor can they be specified on the CLI or called with execute('name').

@dcolish

This comment has been minimized.

Show comment
Hide comment
@dcolish

dcolish Feb 2, 2012

Hmm, i didn't have any issues in this trivial example regardless of the order of the decorators. I should not this runs on my branch with dotted path support, but i dont think that is effecting anything here.

@task
@hosts('host1')
@runs_once
def some_task():
    print env['host']

execute(some_task)

dcolish commented Feb 2, 2012

Hmm, i didn't have any issues in this trivial example regardless of the order of the decorators. I should not this runs on my branch with dotted path support, but i dont think that is effecting anything here.

@task
@hosts('host1')
@runs_once
def some_task():
    print env['host']

execute(some_task)
@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Feb 3, 2012

Member

A) note that I said execute('name') not execute(task_object) ;)
B) If you're calling execute() at top level at all, you're not using fab the tool, which is where the problem comes up.

With the following fabfile.py:

from fabric.api import *

@hosts('localhost')
@task
def foo():
    run("uname -a")

@task
def bar():
    execute('foo')

invoking it as fab --list shows this output:

Available commands:

    bar

Strike one: it's not showing foo. Reversing the order of decorators on foo makes it show up in --list correctly.

Invoking fab bar, which should simply execute 'foo' (via name lookup), results in Fatal error: None is not callable or a valid task name. Strike two. Reversing the order of decorators, again, fixes this. Switching from execute('foo') to execute(foo) (if the decorators are in the "wrong" order) also makes it work fine.

Member

bitprophet commented Feb 3, 2012

A) note that I said execute('name') not execute(task_object) ;)
B) If you're calling execute() at top level at all, you're not using fab the tool, which is where the problem comes up.

With the following fabfile.py:

from fabric.api import *

@hosts('localhost')
@task
def foo():
    run("uname -a")

@task
def bar():
    execute('foo')

invoking it as fab --list shows this output:

Available commands:

    bar

Strike one: it's not showing foo. Reversing the order of decorators on foo makes it show up in --list correctly.

Invoking fab bar, which should simply execute 'foo' (via name lookup), results in Fatal error: None is not callable or a valid task name. Strike two. Reversing the order of decorators, again, fixes this. Switching from execute('foo') to execute(foo) (if the decorators are in the "wrong" order) also makes it work fine.

@dcolish

This comment has been minimized.

Show comment
Hide comment
@dcolish

dcolish Feb 3, 2012

Ah, I see whats going on here. My example was producing this behavior when I ran from the command line as well. I've traced through what is used to append tasks for listing from the cli. It looks like the application of decorators after task is destroying the type information needed, ie, isinstance(a, Task). I don't have a solution yet, but that's the cause afaik.

dcolish commented Feb 3, 2012

Ah, I see whats going on here. My example was producing this behavior when I ran from the command line as well. I've traced through what is used to append tasks for listing from the cli. It looks like the application of decorators after task is destroying the type information needed, ie, isinstance(a, Task). I don't have a solution yet, but that's the cause afaik.

@dcolish

This comment has been minimized.

Show comment
Hide comment
@dcolish

dcolish Feb 3, 2012

Sorry, github seems to have some way of linking issues I don't understand. I sent pull request #548 to start work on a fix for this.

dcolish commented Feb 3, 2012

Sorry, github seems to have some way of linking issues I don't understand. I sent pull request #548 to start work on a fix for this.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Feb 3, 2012

Member

@dcolish I think it just doesn't parse the title/header for "#XXX", I edited your description field to include the reference and it should now show up in here as an autolink commenty looking thingamabob. EDIT: yup.

Member

bitprophet commented Feb 3, 2012

@dcolish I think it just doesn't parse the title/header for "#XXX", I edited your description field to include the reference and it should now show up in here as an autolink commenty looking thingamabob. EDIT: yup.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Feb 8, 2012

Member

Just realized: the "make all our decorators return Task objects" approach would solve it for the use case of "already buying into new-style tasks" -- but it breaks for anybody using old-style tasks :(

EDIT: Except for the if statements in @dcolish's diff, though. That should handle things OK, unless I was thinking of something else before.

Member

bitprophet commented Feb 8, 2012

Just realized: the "make all our decorators return Task objects" approach would solve it for the use case of "already buying into new-style tasks" -- but it breaks for anybody using old-style tasks :(

EDIT: Except for the if statements in @dcolish's diff, though. That should handle things OK, unless I was thinking of something else before.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Feb 14, 2012

Member

As it turns out, this can be implemented in a relatively sane way, and I don't think it does need to be part of 1.4, so I'm going to backport my implementation of it to 1.3.x. That should be pushed soon, then I can do a flight of releases.

Member

bitprophet commented Feb 14, 2012

As it turns out, this can be implemented in a relatively sane way, and I don't think it does need to be part of 1.4, so I'm going to backport my implementation of it to 1.3.x. That should be pushed soon, then I can do a flight of releases.

bitprophet added a commit that referenced this issue Feb 14, 2012

bitprophet added a commit that referenced this issue Feb 14, 2012

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Feb 14, 2012

Member

Ugh, neglected to account for the non-@hosts/@roles decorators. The problem with not pulling down pull requests verbatim.

Member

bitprophet commented Feb 14, 2012

Ugh, neglected to account for the non-@hosts/@roles decorators. The problem with not pulling down pull requests verbatim.

@bitprophet bitprophet reopened this Feb 14, 2012

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Feb 14, 2012

Member

Better now.

Member

bitprophet commented Feb 14, 2012

Better now.

@bitprophet bitprophet closed this Feb 14, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment