760722 - automatic version bumping for mozbase

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

Similar to the versionbump.py script for mozmill,
https://github.com/mozautomation/mozmill/blob/master/versionbump.py ,
a script should be written for mozbase that handles bumping versions
and the dependency requirements.

Given the formal release process of mozbase,
https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Versioning , this
script should also (at least optionally) git tag and push the updated
packages to pypi.

Comment 1

[Jeff Hammel]

Attachment: implementation

The git tagging and bumping to pypi has not been tested live, for obvious reasons.  That said, the commands look fine.  The rest of the patch has been tested pretty thoroughly.

Comment 2

[Jeff Hammel]

If you do test, be careful you don't *actually* upload any new versions :)

The only remaining TODO comment i left is for ensuring you're bumping versions forward.  This requires pkg_resources which I'm not sure is always available with setuptools/distribute.  So I say we just go ahead and don't type in any crazy version numbers (there is also nothing preventing you, nor can there be, really, anything that prevents you from going from, say version 0.1.1 to 1110 if you typo.  Just don't do it and use --diff and --dry-run to ensure you're not being crazy before updating git and pypi.  These mistakes can be undone, just annoying.)

Comment 3

[Jeff Hammel]

:whimboo, please let me know if there's anything you want me to do before reviewing this patch.  I would like to land this soon before it rots and before any further manual version bumping

Comment 4

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

Comment on attachment 635950 [details] [diff] [review]
implementation

> PACKAGE_NAME = "ManifestDestiny"
>-PACKAGE_VERSION = "0.5.4"
>+version = "0.5.4"

Why this change? Was it by accident? Constants should always be built of capital letters. This applies to all setup.py files.

>+++ b/setup_development.py
>-def dependencies(directory):
>+def info(directory):
>     """
>-    get the dependencies of a package directory containing a setup.py
>-    returns the package name and the list of dependencies
>+    get the package setup.py information
>     """
>+

If you only want to add a summary description for the method please do it in a single line. Also there is no empty line afterward.

>@@ -74,20 +67,47 @@ def dependencies(directory):
>+def dependencies(directory):

Such a declaration I would expect for a property but not a method. For the latter I miss a verb for most of the cases in this file.

>+    """
>+    get the dependencies of a package directory containing a setup.py
>+    returns the package name and the list of dependencies
>+    """

Same as mentioned above for the docstring.

>+    # get the .egg-info directory
>+    egg_info = [i for i in os.listdir(directory)
>+                if i.endswith('.egg-info')][0]
>+

So 'i' is referring to directory entries. Can we make this more understandable? 'i' I would expect for loops when we have to iterate via the index. The same applies to the info() method.

Given that we make use of code in getting the .egg-info folder twice, shall we move this into a new method?

>+    if os.path.exists(requires):
>+        dependencies = [i.strip() for i in file(requires).readlines() if i.strip()]

Same for 'i' here.

>+def dependency_info(dep):
>     """
>-    remove version numbers from deps
>+    return dictionary of dependency information from a string like
>+    'mozprocess == 0.2'
>     """

A summary has to fit in a single line. Otherwise use detailed information as a separate block.

>+    retval = dict(Name=None, Type=None, Version=None)
>     for joiner in ('==', '<=', '>='):
>         if joiner in dep:

I would first check if joiner is in dep, that would save us 3 cycles in case it's not.

>+++ b/versionbump.py
>+"""
>+bump mozbase versions
>+https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Versioning
>+"""
>+
>+import optparse

Same as above for docstring. 

>+here = setup_development.here
>+mozilla = 'https://github.com/mozilla/mozbase.git'
>+version_format = '%(Name)s %(Type)s %(Version)s'

Those are constants and capital letters have to be used. Also I would rename 'mozilla' to a proper repository name for mozbase.

>+class CalledProcessError(Exception):
>+    """error for bad calls"""
>+
>+def format_version(**dep):

Two empty lines for top level functions and classes please.

>+def call(cmd, **kwargs):
>+
>+    print "Running %s, %s" % (cmd, kwargs)

Just a nit, but I don't really see a need for this empty line. 

>+def revert(git):
>+    """revert the repository on error"""
>+    call([git, 'reset', '--hard', 'HEAD'])

Mind changing the parameter to 'git_path'?

>+def main(args=sys.argv[1:]):
>+    usage = '%prog [options] packageA=0.1.2 <packageB=1.2> <...>'

So how would someone know about all the dependencies involved here by a complex upgrade? Would you manually have to figure out all the versions or can we have a version file in the root folder which could we use to bump specific versions and which then gets read in? This would definitely make bumping version numbers easier.

>+    parser.add_option('--info', dest='info',
>+                      action='store_true', default=False,
>+                      help="display package version information and dependencies and exit")

There is no rule but we probably should put each entry in its own line. We are doing that for Mozmill and already some CLI commands here in this repository.

>+    parser.add_option('-m', '--message', dest='message',
>+                      help="message to commit to %s; without this, changes will not be committed to git and packages will not be uploaded to pypi" % mozilla)

Would this message apply to any mozbase package which gets upgraded with this single step?

>+    globals()['dry_run'] = options.dry_run # adding to globals to avoid passing to call()

If you make use of inline comments please separate by 2 blanks. Otherwise just put it above the actual code.

>+        if not os.path.exists(pypirc):
>+            parser.error("""%s not found.
>+Make sure you are registered on http://pypi.python.org/pypi ,
>+have permission to update the packages you've specified,
>+and have a file at %s of the form:
>+
>+[server-login]
>+username:<pypiusername>
>+password:<pypipassword>
>+""" % (pypirc, pypirc))

wow, could we define that at the top level? That makes it really hard to read.

>+    # - ensure you are on the master branch (TODO)

This can be removed or?

>+    stdout, stderr = process.communicate()
>+    if stderr or process.returncode:
>+        raise CalledProcessError("Error running %s: %d" % (cmd, process.returncode))

Can we also include the error message please? It's more helpful as the returncode.

>+    # find desired versions
>+    if not args:
>+        parser.print_help()
>+        parser.exit()

I'm sure we want to return with a value != 0.

>+    unrecognized = [i for i in versions if i not in dependencies]

What is i? Please use a more descriptive name.

>+    # TODO: ensure that the new versions are greater than the old versions

Do we have a bug for it already?

>+    if missing:
>+        missing = dict([(i,  [format_version(**k) for k in j])
>+                        for i, j in dependent_versions.items()
>+                        if i in missing])
>+        parser.error("Bumping packages %s, but you also need to bump packages %s" % (versions, missing))

I haven't tested this yet, but we also print out the current version of the dep, right?

>+        if not options.dry_run:
>+            f = file(setup_py, 'w')
>+            f.write(contents)
>+            f.close()

So if something goes wrong in writing content to the files how do we safely revert all the changes?

>+        revert(options.git) # get back to your old state
>+        parser.exit()       # you're done

I think at this level parser.exit() is not appropriate to use anymore. 

>+    # push the changes
>+    if not options.message:
>+        print "No commit --message given; not updating git or pushing to pypi"
>+        parser.exit()

Shouldn't we check this at the beginning and bail out earlier before modifying all the files? Also right now we do not revert the changes which have been made already.

>+    print "Commit changes to %s: %s" % (mozilla, options.message)
>+    call([options.git, 'commit', '-a', '-m', options.message], cwd=here)
>+    call([options.git, 'push', mozilla, 'master'], stdout=None, stderr=None, cwd=here)

I would propose we push this to a specific integration branch and  merge/push it manually. It's kinda risky to mess up with the repository this way. If we don't want to do that, I would at least print the diff to stdout and ask the user again if that's ok to push to master.

Also please take care of line lengths. I would advise you run pep8 (installable via pypi) over this file to get those fixed.

I haven't had time yet to actually test this patch. If you can answer my questions and could come up fix updates I will make sure to test it as early as possible tomorrow.

Comment 5

[Jeff Hammel]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Comment on attachment 635950 [details] [diff] [review]
> implementation
> 
> > PACKAGE_NAME = "ManifestDestiny"
> >-PACKAGE_VERSION = "0.5.4"
> >+version = "0.5.4"
> 
> Why this change? Was it by accident? Constants should always be built of
> capital letters. This applies to all setup.py files.

The version bump program depends on the string `version = "x"` in all setup.py files in order to replace the strings in the setup.py.  So they can all be `version`, which most of them were, or they can all be `PACKAGE_VERSION`, etc.

> >+++ b/setup_development.py
> >-def dependencies(directory):
> >+def info(directory):
> >     """
> >-    get the dependencies of a package directory containing a setup.py
> >-    returns the package name and the list of dependencies
> >+    get the package setup.py information
> >     """
> >+
> 
> If you only want to add a summary description for the method please do it in
> a single line. Also there is no empty line afterward.

I will change all docstrings to be a single line. IMHO this detracts from people trying to understand the code, but que sara sara.

> >@@ -74,20 +67,47 @@ def dependencies(directory):
> >+def dependencies(directory):
> 
> Such a declaration I would expect for a property but not a method. For the
> latter I miss a verb for most of the cases in this file.

What would you suggest for a method? get_dependencies?  Personally, I don't like throwing get in for no real reason, but again will do so against my better judgement if this is what you want.

> >+    """
> >+    get the dependencies of a package directory containing a setup.py
> >+    returns the package name and the list of dependencies
> >+    """
> 
> Same as mentioned above for the docstring.

Will do

> >+    # get the .egg-info directory
> >+    egg_info = [i for i in os.listdir(directory)
> >+                if i.endswith('.egg-info')][0]
> >+
> 
> So 'i' is referring to directory entries. Can we make this more
> understandable? 'i' I would expect for loops when we have to iterate via the
> index. The same applies to the info() method.

Will change to directory.

> Given that we make use of code in getting the .egg-info folder twice, shall
> we move this into a new method?

Would you like this?  IMHO this isn't particularly important.

> >+    if os.path.exists(requires):
> >+        dependencies = [i.strip() for i in file(requires).readlines() if i.strip()]
> 
> Same for 'i' here.

Will do.

> >+def dependency_info(dep):
> >     """
> >-    remove version numbers from deps
> >+    return dictionary of dependency information from a string like
> >+    'mozprocess == 0.2'
> >     """
> 
> A summary has to fit in a single line. Otherwise use detailed information as
> a separate block.

Will do

> >+    retval = dict(Name=None, Type=None, Version=None)
> >     for joiner in ('==', '<=', '>='):
> >         if joiner in dep:
> 
> I would first check if joiner is in dep, that would save us 3 cycles in case
> it's not.

Not sure what you're saying here.  I do first check if each joiner is in the dep: `if joiner in dep`

> >+++ b/versionbump.py
> >+"""
> >+bump mozbase versions
> >+https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Versioning
> >+"""
> >+
> >+import optparse
> 
> Same as above for docstring. 
> 
> >+here = setup_development.here
> >+mozilla = 'https://github.com/mozilla/mozbase.git'
> >+version_format = '%(Name)s %(Type)s %(Version)s'
> 
> Those are constants and capital letters have to be used. Also I would rename
> 'mozilla' to a proper repository name for mozbase.

Name suggestion?

> >+class CalledProcessError(Exception):
> >+    """error for bad calls"""
> >+
> >+def format_version(**dep):
> 
> Two empty lines for top level functions and classes please.

Will change

> >+def call(cmd, **kwargs):
> >+
> >+    print "Running %s, %s" % (cmd, kwargs)
> 
> Just a nit, but I don't really see a need for this empty line. 
> 
> >+def revert(git):
> >+    """revert the repository on error"""
> >+    call([git, 'reset', '--hard', 'HEAD'])
> 
> Mind changing the parameter to 'git_path'?

Its more typing but sure.

> >+def main(args=sys.argv[1:]):
> >+    usage = '%prog [options] packageA=0.1.2 <packageB=1.2> <...>'
> 
> So how would someone know about all the dependencies involved here by a
> complex upgrade? Would you manually have to figure out all the versions or
> can we have a version file in the root folder which could we use to bump
> specific versions and which then gets read in? This would definitely make
> bumping version numbers easier.

So if you run `versionbump.py --info` or `setup_development.py --dependencies` you can see what dependencies are requirements for each packages.  I'm a big -1 on having a file in the root directory that records the versions separately from the setup.py files.  For one, DRY.  For another, if you mess up this file, failures will be difficult to diagnose and could possibly result in bad an accidental releases.  The main point of versionbump.py is to mitigate the possibility of typos and otherwise manually screwing up a version bump by automating the very large number of things that have to be remembered for each bump.  

> >+    parser.add_option('--info', dest='info',
> >+                      action='store_true', default=False,
> >+                      help="display package version information and dependencies and exit")
> 
> There is no rule but we probably should put each entry in its own line. We
> are doing that for Mozmill and already some CLI commands here in this
> repository.

If you want to mail the list and bring this up as a practice that we agree on we can.  Personally, I'm against overformalizing such things.

> >+    parser.add_option('-m', '--message', dest='message',
> >+                      help="message to commit to %s; without this, changes will not be committed to git and packages will not be uploaded to pypi" % mozilla)
> 
> Would this message apply to any mozbase package which gets upgraded with
> this single step?

Yep.  Generally it will be of the form:

"Bug 123456 - Bump mozrunner and mozprofile and release to pypi; r=ctalbert'

> >+    globals()['dry_run'] = options.dry_run # adding to globals to avoid passing to call()
> 
> If you make use of inline comments please separate by 2 blanks. Otherwise
> just put it above the actual code.

Will kill the comment.

> >+        if not os.path.exists(pypirc):
> >+            parser.error("""%s not found.
> >+Make sure you are registered on http://pypi.python.org/pypi ,
> >+have permission to update the packages you've specified,
> >+and have a file at %s of the form:
> >+
> >+[server-login]
> >+username:<pypiusername>
> >+password:<pypipassword>
> >+""" % (pypirc, pypirc))
> 
> wow, could we define that at the top level? That makes it really hard to
> read.

I'll just make a shorter message

> >+    # - ensure you are on the master branch (TODO)
> 
> This can be removed or?

Yep, sorry forgot to remove it.

> >+    stdout, stderr = process.communicate()
> >+    if stderr or process.returncode:
> >+        raise CalledProcessError("Error running %s: %d" % (cmd, process.returncode))
> 
> Can we also include the error message please? It's more helpful as the
> returncode.

Sure.

> >+    # find desired versions
> >+    if not args:
> >+        parser.print_help()
> >+        parser.exit()
> 
> I'm sure we want to return with a value != 0.

Why?  Is it an error to run `versionbump.py`? I say no, but I'm sure there is not consenses on the subject.

> >+    unrecognized = [i for i in versions if i not in dependencies]
> 
> What is i? Please use a more descriptive name.

Will do

> >+    # TODO: ensure that the new versions are greater than the old versions
> 
> Do we have a bug for it already?

No.  We can't very well file bugs for code that doesn't exist yet.  I will file a bug once this is pushed.  Until then, there is no bug as there is no software

> >+    if missing:
> >+        missing = dict([(i,  [format_version(**k) for k in j])
> >+                        for i, j in dependent_versions.items()
> >+                        if i in missing])
> >+        parser.error("Bumping packages %s, but you also need to bump packages %s" % (versions, missing))
> 
> I haven't tested this yet, but we also print out the current version of the
> dep, right?

Yes: format_version(**k) for k in j]

> >+        if not options.dry_run:
> >+            f = file(setup_py, 'w')
> >+            f.write(contents)
> >+            f.close()
> 
> So if something goes wrong in writing content to the files how do we safely
> revert all the changes?

We don't.  The user may run `git reset` his/her self, though in this case the user *might* want to have the repository state handy if they want to diagnose the failure.

> >+        revert(options.git) # get back to your old state
> >+        parser.exit()       # you're done
> 
> I think at this level parser.exit() is not appropriate to use anymore. 

What do you mean at this level?  It is in scope.  Or should I call sys.exit(0) ? (And why?)

> >+    # push the changes
> >+    if not options.message:
> >+        print "No commit --message given; not updating git or pushing to pypi"
> >+        parser.exit()
> 
> Shouldn't we check this at the beginning and bail out earlier before
> modifying all the files? Also right now we do not revert the changes which
> have been made already.

This is by design.

> >+    print "Commit changes to %s: %s" % (mozilla, options.message)
> >+    call([options.git, 'commit', '-a', '-m', options.message], cwd=here)
> >+    call([options.git, 'push', mozilla, 'master'], stdout=None, stderr=None, cwd=here)
> 
> I would propose we push this to a specific integration branch and 
> merge/push it manually. It's kinda risky to mess up with the repository this
> way. If we don't want to do that, I would at least print the diff to stdout
> and ask the user again if that's ok to push to master.

So there are several things you can do with `versionbump.py`:

1. Use --dry-run: this will tell you what it would do if you ran with another command but nothing is changed and nothing is uploaded

2. Run with --diff: this will generate a diff which can be inspected but the changes are reverted in the repository.  This is useful to ensure that the diff you're actually committing == the diff desired.

3. Run without -m, --message:  This will actually modify the repository but will not tag the repository or upload to pypi.

4. Run with --message:  This will *actually* bump the version.

The numerous safety features above should allow for intimate inspection of what is being done with a good fail-safe on the -m switch.  I don't see what another branch buys us.  Since tagging and uploading to pypi are part of the process, I would advise having them in the script.

> Also please take care of line lengths. I would advise you run pep8
> (installable via pypi) over this file to get those fixed.
> 
> I haven't had time yet to actually test this patch. If you can answer my
> questions and could come up fix updates I will make sure to test it as early
> as possible tomorrow.

Comment 6

[Jeff Hammel]

> >+here = setup_development.here
> >+mozilla = 'https://github.com/mozilla/mozbase.git'
> >+version_format = '%(Name)s %(Type)s %(Version)s'
> 
> Those are constants and capital letters have to be used. 

'here' is a common global (ad hoc standard) for the directory containing a python file. We have 'here' several different places in our code....not just mozbase but all over.  'here' is also the setup_development.py name for the variable.  IMHO it is a mistake to change it now

Comment 7

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

(In reply to Jeff Hammel [:jhammel] from comment #5)
> > > PACKAGE_NAME = "ManifestDestiny"
> > >-PACKAGE_VERSION = "0.5.4"
> > >+version = "0.5.4"
> > 
> > Why this change? Was it by accident? Constants should always be built of
> > capital letters. This applies to all setup.py files.
> 
> The version bump program depends on the string `version = "x"` in all
> setup.py files in order to replace the strings in the setup.py.  So they can
> all be `version`, which most of them were, or they can all be
> `PACKAGE_VERSION`, etc.

I would like that we stay with the capital letters here. It's a constant and we shouldn't diverge from coding styles because an external tool wants to have something different. We can update the version bump code easily to support 'PACKAGE_VERSION'.

> > If you only want to add a summary description for the method please do it in
> > a single line. Also there is no empty line afterward.
> 
> I will change all docstrings to be a single line. IMHO this detracts from
> people trying to understand the code, but que sara sara.

As mentioned on IRC yesterday it's not necessary. The only thing you should obey is that the first line should self-explain what this function does and should not extend into the next line. There can still be a full description. See the following patch which I have landed for Mozmill:

https://github.com/mozautomation/mozmill/commit/7e987187e55ab6d52620d6965430784740907c77

> > >@@ -74,20 +67,47 @@ def dependencies(directory):
> > >+def dependencies(directory):
> > 
> > Such a declaration I would expect for a property but not a method. For the
> > latter I miss a verb for most of the cases in this file.
> 
> What would you suggest for a method? get_dependencies?  Personally, I don't
> like throwing get in for no real reason, but again will do so against my
> better judgement if this is what you want.

get, fetch, retrieve - there is more than one verb to describe that. Otherwise make it a getter and leave as is.

> > Given that we make use of code in getting the .egg-info folder twice, shall
> > we move this into a new method?
> 
> Would you like this?  IMHO this isn't particularly important.

Just a nit, if you think it's not that important just skip it. There is not much code which will make use of it.

> > >+    retval = dict(Name=None, Type=None, Version=None)
> > >     for joiner in ('==', '<=', '>='):
> > >         if joiner in dep:
> > 
> > I would first check if joiner is in dep, that would save us 3 cycles in case
> > it's not.
> 
> Not sure what you're saying here.  I do first check if each joiner is in the
> dep: `if joiner in dep`

Drop this. My fault when reading the code. joiner gets defined by the for loop.

> > 'mozilla' to a proper repository name for mozbase.
> 
> Name suggestion?

'REPOSITORY_URL' which is what we use here?

> > So how would someone know about all the dependencies involved here by a
> > complex upgrade? Would you manually have to figure out all the versions or
> > can we have a version file in the root folder which could we use to bump
> > specific versions and which then gets read in? This would definitely make
> > bumping version numbers easier.
> 
> So if you run `versionbump.py --info` or `setup_development.py
> --dependencies` you can see what dependencies are requirements for each
> packages.  I'm a big -1 on having a file in the root directory that records
> the versions separately from the setup.py files.  For one, DRY.  For
> another, if you mess up this file, failures will be difficult to diagnose
> and could possibly result in bad an accidental releases.  The main point of
> versionbump.py is to mitigate the possibility of typos and otherwise
> manually screwing up a version bump by automating the very large number of
> things that have to be remembered for each bump.  

So does --info also show all the dependencies? If that's the case we are totally fine here. Thanks for the explanation and I agree with you.

> > >+    parser.add_option('--info', dest='info',
> > >+                      action='store_true', default=False,
> > >+                      help="display package version information and dependencies and exit")
> > 
> > There is no rule but we probably should put each entry in its own line. We
> > are doing that for Mozmill and already some CLI commands here in this
> > repository.
> 
> If you want to mail the list and bring this up as a practice that we agree
> on we can.  Personally, I'm against overformalizing such things.

So leave as it is. I don't think it's worth starting a discussion on this right now.

> > >+    parser.add_option('-m', '--message', dest='message',
> > >+                      help="message to commit to %s; without this, changes will not be committed to git and packages will not be uploaded to pypi" % mozilla)
> > 
> > Would this message apply to any mozbase package which gets upgraded with
> > this single step?
> 
> Yep.  Generally it will be of the form:
> 
> "Bug 123456 - Bump mozrunner and mozprofile and release to pypi; r=ctalbert'

I assume we also insert the versions beside the package name by default or has to be specified by the user? If that's the case why not hard-code the message to make it consistent. Otherwise people will use different messages for each release. 

> > >+    # find desired versions
> > >+    if not args:
> > >+        parser.print_help()
> > >+        parser.exit()
> > 
> > I'm sure we want to return with a value != 0.
> 
> Why?  Is it an error to run `versionbump.py`? I say no, but I'm sure there
> is not consenses on the subject.

If you don't run with any arguments nothing can be done. I would call this an user failure. Also you are doing the check here and exit right away. So doing an exit with 1 is probably the best solution.

> > >+    # TODO: ensure that the new versions are greater than the old versions
> > 
> > Do we have a bug for it already?
> 
> No.  We can't very well file bugs for code that doesn't exist yet.  I will
> file a bug once this is pushed.  Until then, there is no bug as there is no
> software

Well, how complicated would that be? Could this become a serious problem for our users if we are doing something wrong? Why not implementing it right away if this is just a simple check and we can prove that we do not break something by downgrading a package.

> > >+        if not options.dry_run:
> > >+            f = file(setup_py, 'w')
> > >+            f.write(contents)
> > >+            f.close()
> > 
> > So if something goes wrong in writing content to the files how do we safely
> > revert all the changes?
> 
> We don't.  The user may run `git reset` his/her self, though in this case
> the user *might* want to have the repository state handy if they want to
> diagnose the failure.

But in some cases we run revert(). So why not automatically roll back in all cases something goes wrong? That would give a clean state of the repository without additional user interaction.

> > >+        revert(options.git) # get back to your old state
> > >+        parser.exit()       # you're done
> > 
> > I think at this level parser.exit() is not appropriate to use anymore. 
> 
> What do you mean at this level?  It is in scope.  Or should I call
> sys.exit(0) ? (And why?)

IMHO parser.exit() should only be used when checking options and args for existence and validating their values. Here we are far away from this code. So yes, I would propose sys.exit(0).

> > >+    # push the changes
> > >+    if not options.message:
> > >+        print "No commit --message given; not updating git or pushing to pypi"
> > >+        parser.exit()
> > 
> > Shouldn't we check this at the beginning and bail out earlier before
> > modifying all the files? Also right now we do not revert the changes which
> > have been made already.
> 
> This is by design.

For which reason? Mind elaborating a bit more? Is it to run --diff without having to specify a message? If yes and you agree with my above proposal we could even get rid of this code path.

> The numerous safety features above should allow for intimate inspection of
> what is being done with a good fail-safe on the -m switch.  I don't see what
> another branch buys us.  Since tagging and uploading to pypi are part of the
> process, I would advise having them in the script.

Sorry, that was a left-over on my side I wanted to remove before submitting the patch. You are right here.

(In reply to Jeff Hammel [:jhammel] from comment #6)
> 'here' is a common global (ad hoc standard) for the directory containing a
> python file. We have 'here' several different places in our code....not just
> mozbase but all over.  'here' is also the setup_development.py name for the
> variable.  IMHO it is a mistake to change it now

I think we can make an exception for 'here' then.

Comment 8

[Jeff Hammel]

> 
> So does --info also show all the dependencies? If that's the case we are totally fine here. Thanks for the explanation and I agree with you.

Yep, it does

Comment 9

[Jeff Hammel]

>> > >+    parser.add_option('-m', '--message', dest='message',
>> > >+                      help="message to commit to %s; without this, changes will not be committed to git and packages will not be uploaded to pypi" % mozilla)
>> > 
>> > Would this message apply to any mozbase package which gets upgraded with
>> > this single step?
>> 
>> Yep.  Generally it will be of the form:
>> 
>> "Bug 123456 - Bump mozrunner and mozprofile and release to pypi; r=ctalbert'

>I assume we also insert the versions beside the package name by default or has to be specified by the user? If that's the case why not hard-code the message to make it consistent. Otherwise people will use different messages for each release. 

Historically, we have had a bug associated with version bumps and releases.  If we are going to continue this (which I believe would be a good practice), then it is up to the bug writer to have a sensible bug title that accurately reflects what the commit message should be.  IMHO, doing this in the versionbump.py script is out of scope

Comment 10

[Jeff Hammel]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> > > >+    # TODO: ensure that the new versions are greater than the old versions
> > > 
> > > Do we have a bug for it already?
> > 
> > No.  We can't very well file bugs for code that doesn't exist yet.  I will
> > file a bug once this is pushed.  Until then, there is no bug as there is no
> > software
> 
> Well, how complicated would that be? Could this become a serious problem for
> our users if we are doing something wrong? Why not implementing it right
> away if this is just a simple check and we can prove that we do not break
> something by downgrading a package.

The short answer:  It can be *very* complicated.  In general you *may* have version numbers like

1.0
1.01
1.0.1
1.0pre
1.0a1
1.0beta2

Etc.  Parsing these and ensuring that we're doing what we're supposed to is not trivial.  Also, we have absolutely no protection from this now, which is why it is a new feature.  I'll remove the TODO item and ticket once this is landed.

> > > >+        if not options.dry_run:
> > > >+            f = file(setup_py, 'w')
> > > >+            f.write(contents)
> > > >+            f.close()
> > > 
> > > So if something goes wrong in writing content to the files how do we safely
> > > revert all the changes?
> > 
> > We don't.  The user may run `git reset` his/her self, though in this case
> > the user *might* want to have the repository state handy if they want to
> > diagnose the failure.
> 
> But in some cases we run revert(). So why not automatically roll back in all
> cases something goes wrong? That would give a clean state of the repository
> without additional user interaction.

If the script fails, then something bad has happened.  If we revert the repository state, we lose information about *what* that is bad that has happened.

> > > >+        revert(options.git) # get back to your old state
> > > >+        parser.exit()       # you're done
> > > 
> > > I think at this level parser.exit() is not appropriate to use anymore. 
> > 
> > What do you mean at this level?  It is in scope.  Or should I call
> > sys.exit(0) ? (And why?)
> 
> IMHO parser.exit() should only be used when checking options and args for
> existence and validating their values. Here we are far away from this code.
> So yes, I would propose sys.exit(0).

I'll change it.  Again, I don't see why.

> > > >+    # push the changes
> > > >+    if not options.message:
> > > >+        print "No commit --message given; not updating git or pushing to pypi"
> > > >+        parser.exit()
> > > 
> > > Shouldn't we check this at the beginning and bail out earlier before
> > > modifying all the files? Also right now we do not revert the changes which
> > > have been made already.
> > 
> > This is by design.
> 
> For which reason? Mind elaborating a bit more? Is it to run --diff without
> having to specify a message? If yes and you agree with my above proposal we
> could even get rid of this code path.

Someone that is version-bumping may want to see the state of the repository prior to version bumping. I'll email the list about what we actually wanted to do.  I've put in a workflow that makes sense to me.

> > The numerous safety features above should allow for intimate inspection of
> > what is being done with a good fail-safe on the -m switch.  I don't see what
> > another branch buys us.  Since tagging and uploading to pypi are part of the
> > process, I would advise having them in the script.
> 
> Sorry, that was a left-over on my side I wanted to remove before submitting
> the patch. You are right here.
> 
> (In reply to Jeff Hammel [:jhammel] from comment #6)
> > 'here' is a common global (ad hoc standard) for the directory containing a
> > python file. We have 'here' several different places in our code....not just
> > mozbase but all over.  'here' is also the setup_development.py name for the
> > variable.  IMHO it is a mistake to change it now
> 
> I think we can make an exception for 'here' then.

Comment 11

[Jeff Hammel]

> > > >+    # find desired versions
> > > >+    if not args:
> > > >+        parser.print_help()
> > > >+        parser.exit()
> > > 
> > > I'm sure we want to return with a value != 0.
> > 
> > Why?  Is it an error to run `versionbump.py`? I say no, but I'm sure there
> > is not consenses on the subject.
> 
> If you don't run with any arguments nothing can be done. I would call this an user failure. Also you are doing the check here and exit right away. So doing an exit with 1 is probably the best solution.

For unix programs that operate on arguments, two common behaviours are seen in the wild:
1. if no arguments are passed, print the help and exit
2. err out

I have adopted 1.  I will change it to 2. as per your request but as per most of my comments I don't agree with this nor do I think it is worth blocking a review on

Comment 12

[Jeff Hammel]

Attachment: version 2

I've tried to address all the nits.  If I have missed some, please raise in the bug

Comment 13

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

Comment on attachment 637264 [details] [diff] [review]
version 2

In general this is not a valid patch format for git and makes it hard for me to apply. I really would appreciate if we could add a pull request on github so I can easily pull in the changes and have all data (author, commit message, ...) set correctly.

>+++ b/mozinstall/setup.py
>@@ -11,12 +11,12 @@ try:
> except IOError:
>     description = None
> 
>-version = '1.0'
>+PACKAGE_VERSION = '1.0'

This conflicts now with my version bump for mozinstall yesterday. Just update your patch.

>+++ b/versionbump.py
>+import setup_development
>+
>+here = setup_development.here
>+REPOSITORY_URL = 'https://github.com/mozilla/mozbase.git'
>+
>+class CalledProcessError(Exception):

nit: At top-level we have to use two blank lines as separator.

>+def main(args=sys.argv[1:]):
>+
>+    # parse command line options
>+    usage = '%prog [options] packageA=0.1.2 <packageB=1.2> <...>'
>+    parser = optparse.OptionParser(usage=usage, description=__doc__)

When I run the script for those two specific packages I get failure messages:

./versionbump.py manifestdestiny=1.2 --dry-run
versionbump.py: error: Not a package: manifestdestiny

./versionbump.py mozinstall=1.2 --dry-run
versionbump.py: error: Not a package: mozinstall


>+    parser.add_option('--info', dest='info',
>+                      action='store_true', default=False,
>+                      help="display package version information and exit")

That's really nice! Love it!

>+    parser.add_option('--diff', dest='diff',
>+                      help="output the diff to this file ('-' for stdout)")

'-' doesn't seem to work. I don't get any output on the console:

$ ./versionbump.py mozrunner=1.2 --diff -
Pulling from https://github.com/mozilla/mozbase.git master
Running ['git', 'pull', 'https://github.com/mozilla/mozbase.git', 'master'], {'cwd': '/Volumes/data/code/mozbase', 'stderr': None, 'stdout': None}
From https://github.com/mozilla/mozbase
 * branch            master     -> FETCH_HEAD
Current branch master is up to date.
Bumping mozrunner == 5.6 => 1.2
Writing diff to 
Usage: versionbump.py [options] packageA=0.1.2 <packageB=1.2> <...>

versionbump.py: error: Error running `git diff`


Also shouldn't we reset the tree to head afterward? I expect to get a diff written to the console but not left those changes behind on the master branch.

Further if we have dependencies on other packages I would propose we also print the version of the affected package and not only the dependency from setup.py. That would kill the step to run a --info again.

$ ./versionbump.py mozprocess=3.0 --dry-run -m "asdf"
Checking for pypirc: /Users/henrik/.pypirc
Usage: versionbump.py [options] packageA=0.1.2 <packageB=1.2> <...>

versionbump.py: error: Bumping {'mozprocess': '3.0'}, but you also need to bump {'mozrunner': ['mozprocess == 0.2']}


Helpful would be:

versionbump.py: error: Bumping {'mozprocess': '3.0'}, but you also need to bump {'mozrunner': '5.6'}


Given that I can't fully test this because of the above issues, I will have to wait for an updated patch.

Comment 14

[Jeff Hammel]

> 
> >+def main(args=sys.argv[1:]):
> >+
> >+    # parse command line options
> >+    usage = '%prog [options] packageA=0.1.2 <packageB=1.2> <...>'
> >+    parser = optparse.OptionParser(usage=usage, description=__doc__)
> 
> When I run the script for those two specific packages I get failure messages:
> 
> ./versionbump.py manifestdestiny=1.2 --dry-run
> versionbump.py: error: Not a package: manifestdestiny
> 
> ./versionbump.py mozinstall=1.2 --dry-run
> versionbump.py: error: Not a package: mozinstall

These are not packages.  ./versionbump.py --info displays that mozInstall and ManifestDestiny are packages, but not mozinstall or manifestdestiny (similarly http://pypi.python.org/pypi/manifestdestiny is a 404 and http://pypi.python.org/pypi/ManifestDestiny is a 200)

Comment 15

[Jeff Hammel]

> >+    parser.add_option('--diff', dest='diff',
> >+                      help="output the diff to this file ('-' for stdout)")
> 
> '-' doesn't seem to work. I don't get any output on the console:
> 
> $ ./versionbump.py mozrunner=1.2 --diff -
> Pulling from https://github.com/mozilla/mozbase.git master
> Running ['git', 'pull', 'https://github.com/mozilla/mozbase.git', 'master'], {'cwd': '/Volumes/data/code/mozbase', 'stderr': None, 'stdout': None}
> From https://github.com/mozilla/mozbase
>  * branch            master     -> FETCH_HEAD
> Current branch master is up to date.
> Bumping mozrunner == 5.6 => 1.2
> Writing diff to 
> Usage: versionbump.py [options] packageA=0.1.2 <packageB=1.2> <...>
> 
> versionbump.py: error: Error running `git diff`
> 

Sorry, this was an error in a logic check between the refactoring of the initial patch and now (if not process.returncode vs if process.returncode)

Comment 16

[Jeff Hammel]

Attachment: update with fixes

https://github.com/mozilla/mozbase/pull/24

Comment 17

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

(In reply to Jeff Hammel [:jhammel] from comment #14)
> These are not packages.  ./versionbump.py --info displays that mozInstall
> and ManifestDestiny are packages, but not mozinstall or manifestdestiny
> (similarly http://pypi.python.org/pypi/manifestdestiny is a 404 and
> http://pypi.python.org/pypi/ManifestDestiny is a 200)

Was that an accident when the packages have been created? Why it that not the case for mozrunner. Why do only those two packages use camelcase?

Comment 18

[Jeff Hammel]

(In reply to Henrik Skupin (:whimboo) from comment #17)
> (In reply to Jeff Hammel [:jhammel] from comment #14)
> > These are not packages.  ./versionbump.py --info displays that mozInstall
> > and ManifestDestiny are packages, but not mozinstall or manifestdestiny
> > (similarly http://pypi.python.org/pypi/manifestdestiny is a 404 and
> > http://pypi.python.org/pypi/ManifestDestiny is a 200)
> 
> Was that an accident when the packages have been created? Why it that not
> the case for mozrunner. Why do only those two packages use camelcase?

AFAIK, we have no formal guide on how to name packages. I didn't create the mozInstall package so I can't speak for that one.  ManifestDestiny was originally independent of mozbase and was named independently.

Comment 19

[Jeff Hammel]

Attachment: unbitrot

https://github.com/mozilla/mozbase/pull/25

Comment 20

[Jeff Hammel]

Whether we decide to include this patch in the mozbase repo or not, it makes my life a lot easier, so I will make it a standalone file with some extra work for my own convenience

Comment 21

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

Jeff, you know my stand-point on it given our talk on IRC. It's very helpful to have that script in the repository and I don't understand why you have flipped the review request to Andrew now.

Comment 22

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

Comment on attachment 638497 [details] [diff] [review]
unbitrot

>+    parser.add_option('--strict', dest='strict',
>+                      action='store_true', default=False,
>+                      help="bump dependencies specified as '==' but not '>='")

Given latest discussions you have mentioned that we should never use '>=' for version dependencies. So shouldn't we make this True by default?

>+        call([options.git_path, 'push', '--tags', REPOSITORY_URL, 'master'],
>+             stdout=None, stderr=None, cwd=here)

You can remove 'master'. When you push tags then those are independent from any local and remote branch.

>+    for package in versions:
>+        directory = directories[package]
>+        cmd = [sys.executable,
>+               'setup.py',
>+               'egg_info',
>+               '-RDb',
>+               '',
>+               'sdist',
>+               'upload']
>+        try:
>+            call(cmd, cwd=directory)
>+        except CalledProcessError, e:
>+            print """Failure uploading package %s to pypi.

So what I have seen here is that we can break PyPI packages for a while when an error happens during uploading a package. Reason is that we we don't start with the packages without further dependencies but directly update packages which are dependent on other version bumps. Here an example:

./versionbump.py mozinfo=3.5 mozInstall=1.2 mozrunner=5.8 --dry-run -m "asdf"
Uploading to pypi: mozrunner-5.8, mozinfo-3.5, mozInstall-1.2
Running ['/usr/bin/python', 'setup.py', 'egg_info', '-RDb', '', 'sdist', 'upload'], {'cwd': '/Volumes/data/code/mozbase/mozrunner'}
Running ['/usr/bin/python', 'setup.py', 'egg_info', '-RDb', '', 'sdist', 'upload'], {'cwd': '/Volumes/data/code/mozbase/mozinfo'}
Running ['/usr/bin/python', 'setup.py', 'egg_info', '-RDb', '', 'sdist', 'upload'], {'cwd': '/Volumes/data/code/mozbase/mozinstall'}

In that case we should upload the packages in the following order: mozinfo, mozinstall, mozrunner. That way we can ensure that a new version of a dependent package is up and other packages are still installable because they depend on an older version of the formerly uploaded package.

Given the issue above I will r- this patch because we never should break PyPI. I hope it's something simple to fix Jeff.

Comment 23

[Jeff Hammel]

(In reply to Henrik Skupin (:whimboo) from comment #22)
> Comment on attachment 638497 [details] [diff] [review]
> unbitrot
> 
> >+    parser.add_option('--strict', dest='strict',
> >+                      action='store_true', default=False,
> >+                      help="bump dependencies specified as '==' but not '>='")
> 
> Given latest discussions you have mentioned that we should never use '>='
> for version dependencies. So shouldn't we make this True by default?

Our version policy is at best unclear.  For code where we're rapidly bumping versions without triage or changelogs etc I tend to use '==' to avoid breaking things. However, opinions are divided on the matter.  :carljm prefers to use '>=' which is fine if you are careful about making API changes and also careful about tying to a range of versions which currently the version bumping script doesn't support at all. We in practice use '>=' for one dependency, '==' for 4, and non-specified for one.  While I'd like to get our version story straighter, I don't think this bug is the right forum for that.

That said, I also disagree that, given our lack of current policy, that we should not be bumping '>=' as well as '==' by default.  If you have 

mozprofile 0.4 : ManifestDestiny >= 0.5.4

and you bump ManifestDestiny -> 0.6 and make incomptible API changes to ManifestDestiny, if you don't also bump the dependency requirement in mozprofile then mozprofile will be broken with the old version.  As we've all experienced this is an annoying bug to find, since there will be three classes of users:

1. Developers running setup_development.py (or otherwise having packages setup with python setup.py develop): these will see the packages in the repo and things will (presumedly) work fine assuming mozprofile is compatible with the new version of ManifestDestiny (which it damn sure should be).

2. Developers pulling down new copies of mozprofile from pypi.  They will also get the newest ManifestDestiny as well and so will also not see the problem.

3. Developers with old manifestdestiny installed that install a new mozprofile. If new mozprofile is not compatible with old manifestdestiny, they will note breakage.

The alternative, being overly aggressive in version bumping, is annoying in that it proliferates versions, but it will not break case 3 above.  And, as said, if you are making backwards compatible changes, the flag is available such that you can disable version bumping for such dependencies in such a case.

But this goes some way to versioning policy.  I would welcome this discussion in a wider format than this bug.

> >+        call([options.git_path, 'push', '--tags', REPOSITORY_URL, 'master'],
> >+             stdout=None, stderr=None, cwd=here)
> 
> You can remove 'master'. When you push tags then those are independent from
> any local and remote branch.

Verbatim from https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Versioning as are. the setup.py commands I use.

> >+    for package in versions:
> >+        directory = directories[package]
> >+        cmd = [sys.executable,
> >+               'setup.py',
> >+               'egg_info',
> >+               '-RDb',
> >+               '',
> >+               'sdist',
> >+               'upload']
> >+        try:
> >+            call(cmd, cwd=directory)
> >+        except CalledProcessError, e:
> >+            print """Failure uploading package %s to pypi.
> 
> So what I have seen here is that we can break PyPI packages for a while when
> an error happens during uploading a package. Reason is that we we don't
> start with the packages without further dependencies but directly update
> packages which are dependent on other version bumps. Here an example:
> 
> ./versionbump.py mozinfo=3.5 mozInstall=1.2 mozrunner=5.8 --dry-run -m "asdf"
> Uploading to pypi: mozrunner-5.8, mozinfo-3.5, mozInstall-1.2
> Running ['/usr/bin/python', 'setup.py', 'egg_info', '-RDb', '', 'sdist',
> 'upload'], {'cwd': '/Volumes/data/code/mozbase/mozrunner'}
> Running ['/usr/bin/python', 'setup.py', 'egg_info', '-RDb', '', 'sdist',
> 'upload'], {'cwd': '/Volumes/data/code/mozbase/mozinfo'}
> Running ['/usr/bin/python', 'setup.py', 'egg_info', '-RDb', '', 'sdist',
> 'upload'], {'cwd': '/Volumes/data/code/mozbase/mozinstall'}
> 
> In that case we should upload the packages in the following order: mozinfo,
> mozinstall, mozrunner. That way we can ensure that a new version of a
> dependent package is up and other packages are still installable because
> they depend on an older version of the formerly uploaded package.
> 
> Given the issue above I will r- this patch because we never should break
> PyPI. I hope it's something simple to fix Jeff.

So 1. It is not a simple fix but I can do it.  IMHO it is not worth the time, but you're the reviewer.

2. The packages will be unavailable for about one second while the upload takes place.  So in this case mozrunner will not have an appropriate mozinfo for about 1s.

I will endeavor to fix if I can get a patch up before bitrot happens yet again, but tbh the amount of time to be saved by this patch is probably already lost in sunk costs and since its not attached to a goal for Q2 or Q3 I'm quickly approaching the amount of time its worth it to me to have in the mozbase repo.

Comment 24

[Jeff Hammel]

Attachment: more fixes

Comment 25

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

(In reply to Jeff Hammel [:jhammel] from comment #23)
> Our version policy is at best unclear.  For code where we're rapidly bumping
> versions without triage or changelogs etc I tend to use '==' to avoid
> breaking things. However, opinions are divided on the matter.  :carljm
> prefers to use '>=' which is fine if you are careful about making API
[..]
> But this goes some way to versioning policy.  I would welcome this
> discussion in a wider format than this bug.
 
Right. Can you please start a discussion on this topic please? For this script we are ok for now. Thanks.

> > >+        call([options.git_path, 'push', '--tags', REPOSITORY_URL, 'master'],
> > >+             stdout=None, stderr=None, cwd=here)
> > 
> > You can remove 'master'. When you push tags then those are independent from
> > any local and remote branch.
> 
> Verbatim from
> https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Versioning as are. the
> setup.py commands I use.

Those lines have been added by yourself. Looks like they need to be updated. Tags are not part of a specific branch but are a snapshot of the code you currently have checked out. More information you can find here:

http://learn.github.com/p/tagging.html

> > Given the issue above I will r- this patch because we never should break
> > PyPI. I hope it's something simple to fix Jeff.
> 
> So 1. It is not a simple fix but I can do it.  IMHO it is not worth the
> time, but you're the reviewer.
> 
> 2. The packages will be unavailable for about one second while the upload
> takes place.  So in this case mozrunner will not have an appropriate mozinfo
> for about 1s.

That's not what I'm talking about. I think that scenario would be fine. But keep in mind that a network disconnect or any other failure could start to happen which prevents you from uploading additional packages to PyPI. This will not be a single second but can be an issue for a couple of hours.

> I will endeavor to fix if I can get a patch up before bitrot happens yet
> again, but tbh the amount of time to be saved by this patch is probably
> already lost in sunk costs and since its not attached to a goal for Q2 or Q3
> I'm quickly approaching the amount of time its worth it to me to have in the
> mozbase repo.

I really don't want to go over this again and again. If you want to have less-quality code in mozbase, which hasn't been tested, feel free to ask someone else for review. I for myself will not give an r+ if I can see obvious problems with the patch. Also given that our mozmill-automation package will highly be dependent on mozmill/mozbase packages on PyPI, I don't want to see even more any breakage of those packages.

Comment 26

[Jeff Hammel]

(In reply to Henrik Skupin (:whimboo) from comment #25)
> (In reply to Jeff Hammel [:jhammel] from comment #23)
> > Our version policy is at best unclear.  For code where we're rapidly bumping
> > versions without triage or changelogs etc I tend to use '==' to avoid
> > breaking things. However, opinions are divided on the matter.  :carljm
> > prefers to use '>=' which is fine if you are careful about making API
> [..]
> > But this goes some way to versioning policy.  I would welcome this
> > discussion in a wider format than this bug.
>  
> Right. Can you please start a discussion on this topic please? For this
> script we are ok for now. Thanks.

I have not gained much traction in the posts related to the subject:

https://groups.google.com/forum/?fromgroups#!topic/mozilla.tools/1e2JNN91ugE
https://groups.google.com/forum/#!topic/mozilla.tools/kXxOmaJWO_A/discussion

Without a clear path to decision making, I'm not going to start a new topic related to how we peg our versions.  For the time being, I support '==' until our APIs stabilize quite a bit and hopefully more software consumes mozbase packages, but am not willing to try to lead a discussion to that end unless there is a clear path of decision making.  I do not have the time nor the inclination.

> > > >+        call([options.git_path, 'push', '--tags', REPOSITORY_URL, 'master'],
> > > >+             stdout=None, stderr=None, cwd=here)
> > > 
> > > You can remove 'master'. When you push tags then those are independent from
> > > any local and remote branch.
> > 
> > Verbatim from
> > https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Versioning as are. the
> > setup.py commands I use.
> 
> Those lines have been added by yourself. Looks like they need to be updated.
> Tags are not part of a specific branch but are a snapshot of the code you
> currently have checked out. More information you can find here:
> 
> http://learn.github.com/p/tagging.html
> 
> > > Given the issue above I will r- this patch because we never should break
> > > PyPI. I hope it's something simple to fix Jeff.
> > 
> > So 1. It is not a simple fix but I can do it.  IMHO it is not worth the
> > time, but you're the reviewer.
> > 
> > 2. The packages will be unavailable for about one second while the upload
> > takes place.  So in this case mozrunner will not have an appropriate mozinfo
> > for about 1s.
> 
> That's not what I'm talking about. I think that scenario would be fine. But
> keep in mind that a network disconnect or any other failure could start to
> happen which prevents you from uploading additional packages to PyPI. This
> will not be a single second but can be an issue for a couple of hours.

In my 6 or so years developing python I have never actually encountered this issue. As such, I consider it fairly rare.

In any case, it is fixed in the current patch.

> > I will endeavor to fix if I can get a patch up before bitrot happens yet
> > again, but tbh the amount of time to be saved by this patch is probably
> > already lost in sunk costs and since its not attached to a goal for Q2 or Q3
> > I'm quickly approaching the amount of time its worth it to me to have in the
> > mozbase repo.
> 
> I really don't want to go over this again and again. If you want to have
> less-quality code in mozbase, which hasn't been tested, feel free to ask
> someone else for review. I for myself will not give an r+ if I can see
> obvious problems with the patch. Also given that our mozmill-automation
> package will highly be dependent on mozmill/mozbase packages on PyPI, I
> don't want to see even more any breakage of those packages.

I had tested the initial patch fairly thoroughly. Several things broke when I renamed everything. I think we should weigh the balance of having code that is 100% compliant with style-guides versus the practical advantage of code and the time taken to achieving them as weighed against our goal priorities.

The reason I submitted this patch for review was that I put a few hours on it one weekend and a few more hours in one of those rare lulls between. Probably most team members would say that this is already too long to spend on such a patch which, assuming you don't mess it up, can't be done 30 or so minutes per version bump.  But I often make mistakes and I find consulting a wiki and copy+pasting commands both error-prone and the opposite of intent-driven development.  Since you have asked me several times this last quarter to bump mozbase package versions, I wanted something that would make my life easier and save me time.  I hoped to add this to mozbase repository so that others could also utilize this script and it would save them time too.

However, I have changed my mind.  This isn't related to my goals.  Someone should figure out how we're versioning mozbase and I hope that they would do it in a manner that involved discussion between all the stakeholders, but I know there are wide opinions on the subject, ranging from whether mozbase should be a single lumped package to whether mozbase should be a bunch of different repos to whether all internal versions should be specified via '==' vs '>=' etc.  I haven't made much ground in getting consensus on this.  But at least I threw my hat into the ring.  I have definitive opinions and I have definitive reasons for my opinions.  But since everyone thinks their way of doing things is so easy for some purpose (and I'm including myself in here) and since there is no clear driver of the project that has fostered these discussions, the discussions just kind of die.

As for this particular bug, the 4-5 hours of cost in writing and testing the initial patch and the some 10-20 hours in changing to conform to PEP-8 and personal style preferences undictated in https://wiki.mozilla.org/Auto-tools/Projects/MozBase nor ever also met with consensus to fighting with version control to testing to writing long bug comments defending what I had intended to be an act of kindness to anyone bumping mozmill versions are sunk costs.  The only functional change between https://bugzilla.mozilla.org/attachment.cgi?id=638789 and the initial patch is the resolution of dependency order in uploading to pypi.

For the case of me saving time in version bumping, I now have a standalone script I can use. This will work with the current state of the repository.  Depending on the unresolved versioning discussion it may not work, so I don't think it is worth it for now in putting any more effort into having an in-repository version bumping script until we resolve how we actually want to do versioning.  I can promise that if anyone wants to lead this discussion that I will offer my concerns but ultimately I mostly wants something that works for me and for Mozilla and for my concerns to be understood.  Sunk costs are sunk costs and I am sorry for wasting your time, Henrik, as well as everyone else's in trying for an expedient resolution of this bug.  I am going to unassign myself as I don't intend to work on it in lieu of real Q3 goals, as this doesn't qualify.  But the patch is there if anyone wants to develop it and land it.

Despite the many disagreements I have with what has become mozbase policy, I still believe that the underlying promise of building quality best of breed modular packages with which to construct and maintain test harnesses is a worth cause.  I hope someone -- or some ones -- will step up to leadership of the project and work to build consensus in a way that leaves everyone with a good feeling in their heart and the drive to work on the project rather than aversion.  Mozbase needs a leader.  My attempts to step up and lead have met with nothing but hung juries, so it appears it is not me.

Comment 27

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

Comment on attachment 638789 [details] [diff] [review]
more fixes

Sorry for the delay in actually reviewing this patch. But I haven't thought you will be around today. 

I have tested the latest version of the patch in detail and it works as expected at every stage now. So please get it landed. It's a great addition and will help us a lot in bumping versions in mozbase.

Comment 28

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

(In reply to Jeff Hammel [:jhammel] from comment #26)
> bumping mozmill versions are sunk costs.  The only functional change between
> https://bugzilla.mozilla.org/attachment.cgi?id=638789 and the initial patch
> is the resolution of dependency order in uploading to pypi.

That's not true. There are some other bug fixes and improvements for users of this script.

Comment 29

[Jeff Hammel]

pushed: https://github.com/mozilla/mozbase/commit/82f13ef486341958e7a34484b2d30a003c81a38e