abort() doubles the output message #1318

Closed
4hr1m4n opened this Issue Apr 18, 2015 · 1 comment

Projects

None yet

2 participants

@4hr1m4n
4hr1m4n commented Apr 18, 2015

If abort() is called with a message, like in the code below, and output.aborts is True (default), then the message is displayed twice.

from fabric.api import abort, task

@task()
def test():
    abort("Mxyzptlk")

This is the output of fab test:

Fatal error: Mxyzptlk

Aborting.
Mxyzptlk

This behavior was introduced by 96043d6. Not sure why though.

I propose either to roll it back or, if there is a good reason for that change, make it to something like this:

        sys.exit(1 if output.aborts else msg)

PS: I've also reported this issue on fab-user mailing list back in March 13th

@4hr1m4n 4hr1m4n referenced this issue Apr 20, 2015
@bitprophet Yannis Panousis + bitprophet Include error message in task abort exception
Change-Id: I22856d54b3d55663d8f11c8ad7efa9871a744c14
96043d6
@bitprophet
Member

I've been noticing this myself too, it is pretty annoying. Thanks for bringing it up again.

Dug up its source, which is #1213, wherein my feedback focused on whether it would change the exit code, and totally missed the printing angle. My mistake.

I think a different update than the one you suggest is required. The use case for #1213 seemed to be "I am catching SystemExit above Fabric-using code & want to see the abort message", and my guess is that's irrespective of whether one has aborts active (eg perhaps they're running it headless & want some metadata, such as "what happened with this particular job", stored separately from the raw stdout/stderr).

In other words, the conflation of additional metadata in the exception, with the print behavior, is our problem.


Dug into exactly how sys.exit and SystemException work; under Python 2.6.9 the following appears true:

  • From outside the process, sys.exit(arg) and raise SystemExit(arg) behave identically, including exit code and automatically printed-to-stderr values if it is a string, etc. This was expected from the docs.

  • Introspection shows that SystemExit objects created by either of those methods, have distinct code and message attributes, and that they always contain identical values when generated in the above manner (i.e., arg).

  • However, one can do the following to obtain a SystemExit whose code and message are distinct, and furthermore the message is not printed to stderr (implying that the printing is driven by code, not message), which is exactly what I think we want here:

    e = SystemExit()
    e.code = 1
    e.message = "oh dear"
    raise e

So I think updating abort to use the above lets us have our cake and eat it too - @ipanousis would still be able to inspect e.message in his except clause, but we would bypass sys.exit's printing behavior.

I will implement this now. Should users identify problems with this approach, I am leaning towards reverting the original patch (or going for your meets-it-halfway suggestion), the benefits don't outweigh the ugly.

@bitprophet bitprophet added a commit that referenced this issue May 4, 2015
@bitprophet bitprophet Test proving #1318 e3786d5
@bitprophet bitprophet added a commit that referenced this issue May 4, 2015
@bitprophet bitprophet Add another test re: #1318, re #1213.
Also tweak previous test. Not sure why newline was different this time.
2d7a1b6
@bitprophet bitprophet added a commit that closed this issue May 4, 2015
@bitprophet bitprophet Fix #1318 4baea51
@bitprophet bitprophet closed this in 4baea51 May 4, 2015
@bitprophet bitprophet added a commit that referenced this issue May 4, 2015
@bitprophet bitprophet Changelog re #1318 055b141
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment