802265 - versionbump.py should have a --tag-only option

Status: RESOLVED WORKSFORME

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

currently, versionbump.py is fairly monolithic.  One consequence of
this is that if one step fails, like for instance if i leave the
interactive password prompt open too long and a git pull request times
out, then it is hard to resume the step.  We should have an
e.g. --tag-only option to deal with this (and possibly an
--upload-only switch for uploading to pypi).

Comment 1

[Jeff Hammel]

As a long term plan, we should separate out the various components in actions with dependencies.  In fact, a general purpose python implementation would be nice.  For the immediate term, this could be done or we could just have if-else blocks based on --tag-only (much quicker)

Comment 2

[Jeff Hammel]

So for the general purpose implementation we should have something like a list of actions that get run that correspond to various code blocks.  These could be e.g. 'bump_setup_py_versions', 'git_tag', 'upload_to_pypi' etc.  Each of these should be separated to their own functions.  The main section can then call each of these in order.  doing e.g. '--tag-only' or what not should remove the unneeded actions.  I can provide example code if this makes sense or talk via IRC or other

Comment 3

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: refactor as requested

Here's a prototype of what was requested.

I kept the globals()['var'] trick because it seems the easiest way in which we can have the actions as functions that get looked up by name and executed. Otherwise, we'd have to do something with the return values and the "do_action" function would get more complicated.

Since the diff may be misleading, you can see the whole file here:
http://www.pastebin.mozilla.org/1995889


Basically new parameters and new actions would be trivial to add this way.

Comment 4

[Jeff Hammel]

Comment on attachment 692443 [details] [diff] [review]
refactor as requested

If we're going to do a drastic rewrite, we probably shouldn't use
global variables.  Instead, we should probably use a class for this.

+    if options.dry_run:

Should be an instance variable

+    # export the data
+    globals()['info'] = info
+    globals()['dependencies'] = dependencies
+    globals()['directories'] = directories

Yeah...let's not do this anymore

++methods = {
+    'pkg_info': pkg_info,
+    'print_pkg_info': print_pkg_info,
+    'print_pypi_versions': print_pypi_versions,
+    'check_for_pypirc': check_for_pypirc,
+    'check_for_master_branch': check_for_master_branch,
+    'check_for_clean_dir': check_for_clean_dir,
+    'parse_pkg_versions': parse_pkg_versions,
+    'check_bumping_dependencies': check_bumping_dependencies,
+    'check_for_latest_revision': check_for_latest_revision,
+    'bump_desired_packages': bump_desired_packages,
+    'bump_dependent_packages': bump_dependent_packages,
+    'write_diff': write_diff,
+    'check_for_msg': check_for_msg,
+    'push_changes': push_changes,
+    'tag_changes': tag_changes,
+    'upload_to_pypi': upload_to_pypi,
+    }

Again, let's not do this.  A decorator would be more appropriate.

+method_deps = {}
+method_deps['info'] = ['pkg_info', 'print_pkg_info']
+method_deps['pypi_versions'] = ['pkg_info', 'print_pypi_versions']
+method_deps['bump'] = ['pkg_info', 'check_for_master_branch',
'check_for_clean_dir',
+                       'parse_pkg_versions',
'check_bumping_dependencies',
+                       'check_for_latest_revision',
'bump_desired_packages',
+                       'bump_dependent_packages']
+method_deps['diff'] = method_deps['bump'] + ['write_diff']
+method_deps['push'] = method_deps['bump'] + ['push_changes']
+method_deps['tag'] = method_deps['push'] + ['tag_changes']
+method_deps['tag_only'] = ['pkg_info', 'check_for_master_branch',
+                           'check_for_clean_dir',
'check_for_latest_revision',
+                           'parse_pkg_versions']
+method_deps['upload'] = method_deps['tag'] + ['check_for_pypirc',
'upload_to_pypi']

Again, let's not do this ;)  Since we have e.g.

+method_deps['diff'] = method_deps['bump'] + ['write_diff']

its probably better to build a system that will recursively descend
through dependencies instead of trying to keep a flat list.

+    globals()['args'] = args
+    globals()['options'] = options
+    globals()['parser'] = parser

Let's not do this

+    if options.info:
+        do_action('info')
+        parser.exit()
+
+    if options.pypi_versions:
+        do_action('pypi_versions')
+        parser.exit()
+
+    if options.diff and not options.dry_run:
+        do_action('diff')
+        sys.exit()
+
+    if options.tag_only:
+        do_action('tag_only')
+        sys.exit()
+
+    if options.tag:
+        do_action('tag')
+        sys.exit()
+
+    do_action('upload')

A few things here:

* +    if options.diff and not options.dry_run:
  Probably not the best pattern.  Instead, it'd be easier to make the
  functions respond to options.

* Similarly, if you do this, you can make a list of actions an iterate
  over them (or something similar, potentially relying on recursive
  dependencies). I generally avoid if-else trees unless they're really
  necessary and in this case they aren't.  In this case, its not
  necessary except for the above bullet point: you have a table of
  things to do which are functions which take no arguments

* Again, I'd rather off load most of this logic to a class

----

Backing off a bit, what problems are we solving here?

* It would be nice to have a simple class that encapsulates the
  dependencies that actions need

* This class should take configuration from the command line and
  "other"

* [Recommended] there should be a decorator to label what "actions"
  are required for other actions.

As I've often done, I've seriously underestimated the amount of work
this bug requires ;)  I'm happy to stub out an outline when I get a
chance, but this will actually be a, ahem, "interesting"
architecture.  In short....

class Dependencies(object):
      """dependency decorator"""

      def __call__(self, *dependencies):
          """return a function that decorates a function and notes the
          dependencies"""

      def do(self, action):
          """perform the function `action`"""

action = Dependencies()

class VersionBump(self):
      defaults = {}


      def __init__(self, various, options, and, args):
          """setup VersionBump environment, configuration, etc"""

      @action('info', ...)
      def tag(self):
          """"something about tagging; it depends on info being run
      here"""

      def info(...):

def main(args=sys.argv[1:])

    parser = OptionParser()
    # TODO: add options, probably from the VersionBump class
    options, args = parser.parse_args(args)

    arguments = dict([(i, j) for i, j in options.__dict__.items()
                      if i in VersionBump.defaults])
    versionbump = VersionBump(**arguments) # and something about the
                      versions

    actions = [] # some list of actions from the command line
    for a in actions:
        getattr(versionbump, action)()


This is basically just a sketch, but similar to what I would do.

Comment 5

[Mihnea Dobrescu-Balaur (:mihneadb)]

As discussed on IRC, work in progress: http://pastebin.mozilla.org/2009121

Comment 6

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: rewrote vb.py

The file looks like this: http://pastebin.mozilla.org/2035641

So I'm not sure about the build_actions & run() part, suggestions are welcome. Otherwise, the core ideas are as discussed.

Comment 7

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: rewrote vb.py

Forgot to add --tag-only :).

http://pastebin.mozilla.org/2035648

Comment 8

[Jeff Hammel]

Sorry this has taken me so long to get to.  :mihneadb, could you upload the rewritten version of the file?  So much has changed that the diff is pretty hard to parse

Comment 9

[Mihnea Dobrescu-Balaur (:mihneadb)]

I have, there's the URL in #c7.

http://pastebin.mozilla.org/2035648

Comment 10

[Jeff Hammel]

Attachment: f

Comment 11

[Jeff Hammel]

Comment on attachment 697422 [details] [diff] [review]
rewrote vb.py

So....

This is really complicated and hard to read.  I imagine it would also be very hard to debug.  While I'm not going to take the time to go through this verbosely, a few things:

- build_actions_list seems pretty unnecessary.  Really, 'run' should take an action (or list of actions) to run directly from the command line.

- ... this also implies that casting the CLI options directly to a dict to use in the ctor doesn't really work.  It implies that the class is usable other than via CLI invocation, which isn't effectively true

- quit() should never be an action (or a function, for that matter)

- actions should allow recursive dependencies.  ABICT, they don't

In any case, I somewhat regret calling for an action-dependency system now.  While I do think it is, in the long term, the right path, it will be very hard to get right.  I will try to take a stab at it sometime in the future, but for now it seems a lot of work for a tiny bit of benefit.

Comment 12

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Jeff Hammel [:jhammel] from comment #11)
> Comment on attachment 697422 [details] [diff] [review]
> rewrote vb.py
> 
> So....
> 
> This is really complicated and hard to read.  I imagine it would also be
> very hard to debug.

Except for the class/dependencies part, it is still the existing code from vb.py. Do you mean the dependency system is complicated?

>  While I'm not going to take the time to go through this
> verbosely, a few things:
> 
> - build_actions_list seems pretty unnecessary.  Really, 'run' should take an
> action (or list of actions) to run directly from the command line.
> 
> - ... this also implies that casting the CLI options directly to a dict to
> use in the ctor doesn't really work.  It implies that the class is usable
> other than via CLI invocation, which isn't effectively true

Right, as I said, I don't know how to "correctly" do that part. Suggestions are welcome! :)

> 
> - quit() should never be an action (or a function, for that matter)
> 
> - actions should allow recursive dependencies.  ABICT, they don't

I'm not sure what you mean. if F depends on G and H depends on F, when you call H both G and F will get called.

> 
> In any case, I somewhat regret calling for an action-dependency system now. 
> While I do think it is, in the long term, the right path, it will be very
> hard to get right.  I will try to take a stab at it sometime in the future,
> but for now it seems a lot of work for a tiny bit of benefit.

Sure, let me know if I can help. :)

Comment 13

[Ed Morley [:emorley]]

versionbump.py no longer exists after bug 949600.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I don't think we should get this bug closed. Instead get it ported via bug 974184 for hg.m.o.

Comment 15

[Geoff Brown [:gbrown] (pto Mar 24)]

There's only versioninfo.py now (via bug 974184); I don't think this bug is relevant any more.