Allow the easy creation of executable utilities with invoke. #173

Closed
sophacles opened this Issue Aug 23, 2014 · 28 comments

Projects

None yet

2 participants

@sophacles

Description

Splitting this ticket from #170 at the request of @bitprophet

It would be nice to easily create scripts that I can place in my $PATH that internally use invoke, but are named according to script functionality. Some rational (copied from referenced ticket discussion):

I could probably train myself always to do:

inv -c /path/to/source/tree/utils/$TASK ....

But I kind of like having them:

  1. as a script available in my $PATH
  2. as tools named cognitively, just like when I'm using busybox... i just link (e.g) ls and cp rather than calling busybox ls ... or busybox cp file1 file2
  3. in a consistent place (depending on what I'm doing, the source tree where the task files live can vary. It's in one place on my dev machine, and different places in deployment depending on what OS, nfs details, and so on. We're still working out our "one true deployment strategy" so this changes as stuff comes up).

Also noteworthy, several of my targets are most commonly called from places in the system that are not the directory where invoke lives.

Currently I do this via the code in this gist: https://gist.github.com/sophacles/c17ce33a14d582dfa268 and creating scripts entry points in a setup.py.


To-do list

[Note: this part of the ticket is being maintained by @bitprophet]

  • Parameterization of version number by itself (not name+version as we currently do)
  • Parameterization of tool/package name, defaulting to the name seen in argv[0]
    • Capitalized in --version output, e.g. Invoke 1.2
    • Allowance for arbitrary additional lines of version info, e.g. for Fabric wanting to display its + Paramiko's, and even e.g. other data like Python version?
    • Left as-is in --help output, e.g. inv[oke] --core-opts ...
    • Is there an automated way to determine inv vs invoke vs etc? We don't have access to setuptools' data at runtime, so we'll probably have to provide this manually. (Maybe stored in similar fashion as version - reused in both spots?)
  • Ability to update global --help to include elements of --list, i.e. for tools distributing with built-in tasks as subcommands (which would be...most of them...Fabric is actually unusual in this regard).
    • I.e. probably sticking --list type output above the core flags, which is what most real subcommand based CLI apps tend to do (see git, pip etc)
  • Fix up per-task help to honor the dynamic program name (it's hardcoded right now, as above)
  • Ability to override which flags are presented and/or their default values

Cleanup:

  • Check all the task modules in _support to remove any no longer in use (& consolidate some if possible)
  • Check all methods in _utils.py for whether they've gone out of use
  • Ensure docs build & the docstrings recently added look good rendered
@bitprophet
Member

As I think I mentioned in #170 this basically just translates to "make the existing cli.py even easier to reuse".

@sophacles

Anyone looking do this right now, until it can be properly included in invoke, can do what I do at this gist: https://gist.github.com/sophacles/fc6bf504af1d4e3d4485

It uses some introspection magic to figure out where the dispatch_me() call is coming from, and re-works the command line before dispatching to invoke. It's not perfect, nor pretty, but it gets the job done for now.

@bitprophet
Member

I was kinda wanting this just the other day, in a one-off fashion (i.e. a single task as a 'binary' - for now I just run invoke directly but it'd be nice to distribute as a single binstub) though obviously having a full collection available as per earlier comments here is equally valid.

Suspect the real driver for this will be my ongoing fabric 2 work (providing a fab binary that adds/changes core flags as well as being renamed), so might happen sooner instead of later.

@bitprophet bitprophet added this to the 1.0 milestone Sep 7, 2015
@bitprophet
Member

I don't see a note here about it, but at some point we did do a TINY bit of work towards this by allowing users to override --version output, https://github.com/pyinvoke/invoke/blob/6aeffbde925291c281c700be0f1fac7c50a6fbc4/invoke/cli.py#L268-L271 - I think it was coderanger who requested it.

Whatever we do for this (which will be Soon now because I need it, again) should tickle that too, or replace it.

@bitprophet
Member

Also noticed there's a bug of sorts, doing core help (inv --help) obtains a dynamic program name based on argv[0], which is nice, as it means simply calling invoke.cli.main from another package's CLI entrypoint works correctly. However, it a) ignores the "name + version" bit passed in to dispatch, and b) isn't reflected in per-task help, despite the 'header' of the two help outputs being similar otherwise.

Updated ticket desc with a checklist & more thoughts.

@bitprophet
Member

Realized as I started writing docs for this that the truly common case isn't Fabric, which still cares about exposing the idea of arbitrary tasks to be run, but a program where the distributed task tree are the ONLY tasks ever to be run, because they're effectively subcommands.

In that scenario, the default split between --help and --list is unhelpful at best and confusing at worst. We really want the ability for --help to show both the task tree and core options, think pip / git help output.

Having updates to help output was already required here but this is a more involved version of it. Hopefully not too hard, though.

@bitprophet
Member

One open question, do we refer to "baked-in" tasks as "commands" or "subcommands"? Most programs seem to use the terminology "commands", but for some reason my instinct was to refer to them as "subcommands" (since the program itself is "the command").

@bitprophet
Member

Have been doing a metric shitton of implementation/refactoring towards Program these past few days, it's shaping up pretty nicely and as a side effect has cleaned up the relevant existing tests at least a little bit (sometimes moderately). Most of the way there.

@sophacles

Looking at the branch for this, I'm pretty excited about it landing. Fantastic work as usual @bitprophet!

As for the open question, I'm +1 on subcommands being the terminology, as that is how my documentation refers to them where I use my hack mentioned above.

@bitprophet
Member

Thanks for the feedback @sophacles :) glad you're watching since I was going to ask you your opinion on the API once I had it working well enough.

Currently bedeviled by the old "do I write the class as a series of bundled functional subroutines passing data around (i.e. classes as code reuse only), or do I shovel all that data into self.xyz (i.e. classes as state keepers too)" question. I've gone with the latter because it simplifies a lot of niggly bits in the implementation, but it does make targeted unit-style testing harder & is less "appropriate" given Program's intended API is very short-lived - having one around for more than a few lines is unlikely.

Then again, I waffle a lot on just how unit-y to make the tests for this module, since much of the behavior is only interesting at a high level ("did giving X argv result in triggering Y behavior just prior to exit?") and unit testing specific methods doesn't ensure they're even called by run.

So...having to stick with "call Program().run() and examine result" for some tests is not a huge loss really.

@bitprophet
Member

cli.py is dead, long live cli.py \o/ all tests pass again. Back to the checklist...

@bitprophet
Member

Trying to name a kwarg/attr for "extra versions", i.e. so that Fabric can preserve its "my version + my immediate dependencies' versions" output - I think that's a useful thing to have in some situations.

E.g. something like:

from . import __version__ as fabric_ver
from invoke import __version__ as invoke_ver
from paramiko impot __version__ as paramiko_ver

program = Program(
    name='Fabric',
    version=fabric_ver,
    extra_versions=[
        "Paramiko {0}".format(paramiko_ver),
        "Invoke {0}".format(invoke_ver),
    ],
)

Literally calling it extra_versions feels a little clunky though, so I'm wondering about e.g. subversions or dependency_versions (though, I could see folks using this for not-just-dependency info, e.g. spitting out the current Python and OS/platform versions, if they wanted a whole hog easy-to-paste thing for bug reports).

Also alternate ideas like "just make it a single string that is printed verbatim", maybe even accompanied by "and then do away with 'name' and 'version' as separate fields too". ¯\_(ツ)_/¯

@sophacles any opinions?

@sophacles

Personally, I'm a fan of "short version" and "long version" as separate things. (And name as separate as well.) For example nginx has the -v and -V option, the difference being (on my nearest VM):

vagrant@test:~$ nginx -v
nginx version: nginx/1.4.6 (Ubuntu)
vagrant@test:~$ nginx -V
nginx version: nginx/1.4.6 (Ubuntu)
built by gcc 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04) 
TLS SNI support enabled
configure arguments: --with-cc-opt='-g -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D_FORTIFY_SOURCE=2' --with-ld-opt='-Wl,-Bsymbolic-functions -Wl,-z,relro' --prefix=/usr/share/nginx --conf-path=/etc/nginx/nginx.conf --http-log-path=/var/log/nginx/access.log --error-log-path=/var/log/nginx/error.log --lock-path=/var/lock/nginx.lock --pid-path=/run/nginx.pid --http-client-body-temp-path=/var/lib/nginx/body --http-fastcgi-temp-path=/var/lib/nginx/fastcgi --http-proxy-temp-path=/var/lib/nginx/proxy --http-scgi-temp-path=/var/lib/nginx/scgi --http-uwsgi-temp-path=/var/lib/nginx/uwsgi --with-debug --with-pcre-jit --with-ipv6 --with-http_ssl_module --with-http_stub_status_module --with-http_realip_module --with-http_addition_module --with-http_dav_module --with-http_geoip_module --with-http_gzip_static_module --with-http_image_filter_module --with-http_spdy_module --with-http_sub_module --with-http_xslt_module --with-mail --with-mail_ssl_module

Since invoke is a framework, I think supporting this notion is a good idea. A couple of options that come to mind are:

  • -v is short version output and --version is long version output (because the form and the function are conceptually the same: short arg = short version, long arg = long version)
  • mirror the nginx way (little v = little output, big V = big output). (the long-form of these would be --version and --long-version respectively. This works nicest with the current argument parsing)

As for the mechanism of making this happen I think something like the following is both simple and flexible enough for most cases:

  • In Program keep the version attribute as is.

  • In Program add a property long_version that behaves something like this:

    @property
    def long_version(self):
        if self._long_version is None: return self.version
        if isinstance(self._long_version_val, basestring):
           return self._long_version_val
        else:
            return self._long_version_val()
    @long_version.setter
    def long_version(self, val):
        if isinstance(val, basestring) or callable(val):
            self._long_version_val = val
        else:
            raise SomethingIsBroken("bad long version, must be callable or string")
  • in Program change to: def print_version(long=False) (and follow the obvious implications of that as appropriate).

  • in the constructor for Program, name the argument long_version, and defaults to None (note above handles this case as long version == short version)

  • in constructor for Program, have a way to set short version = long version (the inverse of long_version = None). Further thoughts below....

By doing this your above example becomes:

from . import __version__ as fabric_ver
from invoke import __version__ as invoke_ver
from paramiko impot __version__ as paramiko_ver

    extra_versions=[
    ],

def _fab_long_version():
    return "\n".join(["Dependency info:",
                      "\tParamiko {0}".format(paramiko_ver),
                      "\tInvoke {0}".format(invoke_ver),
                      "\tand so on..."])

program = Program(
    name='Fabric',
    version=fabric_ver,
    long_version = _fab_long_version
)

Playing devil's advocate against myself:

  • There's a few weirdnesses with this mechanism - specifically defaulting to long-version always is a bit strange. There would also be the case where long-version = short-version having 2 different version arguments (although the '-v' and '--version' CLI convention mentioned above would hide this). This edge case seems to me as unusual, and could be handled simply by subclassing the Program class in the instances where it is needed.
  • It could also be argued that the long version stuff I just described could be the result of subclassing when needed for it. The problem I have with making the user do that, is it requires the user to muck with argument parsing almost right away if they want this sort of behavior (although one of the things I really like about the way Program is implemented is that mucking with core arguments is easy now!).
@bitprophet
Member

Good point re: having 2x version args, I considered it briefly but either forgot or discarded it because I thought the other -v was already taken (it's actually not tho...but maybe it should be, idk. it's so commonly used for verbosity control...).

Also good point re: subclassing, I'm torn on whether that "should" be the right way to go about this, but it definitely works and pretty easily (& most importantly, it's entirely flexible from the user's perspective - for all I know they might want all sorts of crazy shit in here besides a simple list of version numbers.)

E.g. this just-works right now with no other changes to Program:

from invoke import Program, __version__ as invoke
from paramiko import __version__  as paramiko

from . import __version__ as fabric


class Fab(Program):
    def print_version(self):
        super(Fab, self).print_version()
        print("Paramiko {0}".format(paramiko))
        print("Invoke {0}".format(invoke))

program = Fab(name="Fabric", version=fabric)

Given the above & that I've spent about a week longer on this than I intended (sigh) I'm pretty tempted to just leave it as-is for now, actually. We can always build more functionality into Program later if we find common patterns.

@bitprophet
Member

Also, yea, even now it should be way easier to muck with core flags, and I haven't gotten around to that part specifically! Which is one of the last things before I can close this as "works well enough for fabric 2 and @sophacles" ;) (hopefully)

@sophacles

Hah! You did ask for my opinion, I figured I should offer one... but overall I'm +1 on good enough[1]. This already lets me cut out a bunch of hacky code I have in production right now, and the 2x versions thing is not actually that important. I'll be interested to see how the arg mucking ends up though!

[1] I say this as a person who qualifies as 'might want all sorts of crazy shit in here' -- subclassing covers most forms of crazy I can come up with in actual, practical usage.

@bitprophet
Member

Yea I'm poking at arg stuff now so we'll see what shakes up. The driver is basically "how to get fab -H foo mytask working" (insofar as def mytask(ctx, host): doesn't scale very far once you have N tasks) so that requires ability for Fabric to expand core args to include stuff like -H. Then probably also some subclassing of Executor to tweak the context it hands in, and maybe that's it for now.

Which would rock & also be the point of the entire, broad exercise of "rewrite shit using classes instead of a-bunch-of-not-really-functionally-written-functions" :D so we'll see.

@bitprophet
Member

And yea, thanks a LOT for your detailed opinion, I'm sorry I wasn't up to chasing it further, but I'm super prone to that sort of unintentional nerd sniping / bikeshedding / yak shaving; and am trying to learn how to manage my time better! (Also working on a deadline, I want to get these Invoke changes & Fab 2 alpha out prior to "the last day before I go on a 3 week vacation" which is October 2nd....)

@sophacles

Right on - in that case I'll wait a couple months I start pushing on #186 and #170 again :)

@bitprophet
Member

I imagine public feedback on, and/or my own use of as I try porting my own stuff to, Fabric 2 will bring at least #170 close to the front of the queue anyways :)

@bitprophet
Member

Basically waiting on merging this until the fab CLI tool "mostly works". Today, got things tweaked such that it's easier for client code of Invoke to change up the default collection name away from tasks.

Also found some slightly related holes in the test suite (eg after moving help junk around to support bundled namespaces [a feature Fabric doesn't actually use, funnily] I accidentally broke core --help in situations where no tasks collection was present.

Next up is aforementioned parser tweakin' stuff - adding a -H to Fabric (I just nuked it from Invoke proper, didn't realized I'd accidentally poached it. Meh. Relying on a long-flag for global output hiding isn't that bad for now) which will probably tie into Executor subclassing & class selection.

@bitprophet
Member

@sophacles, I just pushed what might be the last main change for this feature - the other tweaks I need to make for fab to work aren't as strictly tied to "run some invokey things as separate binaries" (e.g. Executor updates).

Anyway, that change was to make core_args a method instead of a class variable, that seems to be the easiest way for subclasses to add on more, (ugly because Github uses raw RST and not Sphinx) example here: https://github.com/pyinvoke/invoke/blob/def2042fa8c109aa0f4046d3b315378d15ff0abf/sites/docs/concepts/library.rst#modifying-core-parser-arguments

Let me know your thoughts on that though :)

@sophacles

I took the morning to dive deep on the branch. Everything looks great so far, using the core_args() as a method approach makes sense and feels consistent with the rest of invoke customization so I'm +1 on it.

A thought on structure: Currently accessing new core args is not obvious. If I'm creating a Program subclass with a few extra args, accessing those args in methods of Program itself is easy, however, there is nothing to access them after the executor is intantiated/run. I think a simple way to do this, is
to have the config property include a dict core_args or cliargs (etc) in the overrides dict of the Config object it returns. This allows program writers to access the core args from within their tasks, without having to also do method overrides on config or run or parse or _parse or... (actually, the correct method to override is also unclear anyway).

Another thought (slightly tangential) - env_prefix being hardcoded as INVOKE_ stops making sense if users are making cli programs. It can introduce conflicts, and also be very confusing for the ultimate end users (the users' users) - as in "why do I have to configure fromulator with env var INVOKE_FOO?"

@bitprophet
Member

Yea, I think I had the same thought about exposing core args in Context/Config, but never followed up on it. I will actually run into that today, my "the rest of the stuff isn't applicable" was probably wrong.

+1 on the env_prefix, that's one of those bits where I ported it straight from the old implementation and haven't gotten around to pushing it "up" farther for easier override. Should be pretty easy to fix.

@bitprophet
Member

OK, env_prefix is controllable in Program constructor now. Took a lot longer than I expected but only because the testing of it was tricky & I was being really, really stupid in my initial stab at the test. So I got to deep dive for a while before realizing my mistake. Oh, programming.

@bitprophet
Member

Want to avoid any further "living in a branch forever" shenanigans; merging this to master for now, then branching out again as needed for the Executor related stuff or anything else that pops up.

Will try to release again in the very near future as well, especially if @sophacles has time to actually try applying the now current state of master to his work & give a "yup, nothing broke" +1 :)

@bitprophet bitprophet closed this Sep 21, 2015
@sophacles

@bitprophet have a task scheduled for later this week to give it a shot. There's actually a bit of work on my end for that since the new Program class means futzing with taking out my hack code and translating the semantics right. But I'll ping in irc when I get to that!

@bitprophet
Member

Awesome, thanks :)

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