Open Stack

On 12/03/2012 03:23 AM, Mark McLoughlin wrote:
> On Fri, 2012-11-30 at 13:55 -0800, Monty Taylor wrote:
>> Hey Matt!
>>
>> On 11/30/2012 01:24 PM, Matt Joyce wrote:
> 
>>> 2.  Second Concern - .git derives our versioning
>>>
>>> First, thank god for the versioninfo file.  Without it, cinderclient
>>> would be unbuildable in certain situations.
>>
>> Totally. It's pretty important.
>>
>>> It boils down to this logic:
>>>
>>> from cinderclient/openstack/common/setup.py  ( which btw is common
>>> SOLELY to cinderclient every other client does this logic in a different
>>> yet standard fashion ).
>>
>> I did not realize cinderclient had diveged. We should fix that. (patch
>> coming)
>>
>>> def get_post_version(projectname):
>>>     """Return a version which is equal to the tag that's on the current
>>>     revision if there is one, or tag plus number of additional revisions
>>>     if the current revision has no tag."""
>>>
>>>     if os.path.isdir('.git'):
>>>         version = _get_git_post_version()
>>>         write_versioninfo(projectname, version)
>>>         return version
>>>     return open(os.path.join(projectname, 'versioninfo'),
>>> 'r').read().strip()
>>>
>>> This basically says either grab the version from .git, or the
>>> versioninfo file.  Except that generating the versioninfo file requires
>>> .git.
>>
>>
>> Well, there is a bug in the logic there. Currently it tries to keep the
>> versioninfo file up to date at all costs, so if it finds that it has a
>> git repo, it will recreate the versioninfo file.
>>
>> Except that makes evil for git-buildpackage. Sorry, I just didn't catch it.
>>
>> The fix for that is to always trust the versioninfo file if it exists.
>> Since it gets created on tarball creation as part of our release
>> process, it should never not be there - and to only fall back to git if
>> the versioninfo file does not exist.
>>
>> I can come up with no valid situation where a person will have a copy of
>> the code with neither an upstream git repo OR a tarball generated
>> versioninfo file. (although please check me if I'm wrong there)
> 
> I saw the review[1] before this thread.
> 
> AFAICT the logic you're proposing is:
> 
>   if versioninfo exists:
>       version = read_versioninfo()
>   else:
>       version = do_some_git_magic()
>       write_versioninfo(version)
>   return version
> 
> So, if you clone a project, do something with setup.py, versioninfo gets
> created at that time and never gets re-created again?

That is correct. The reason the logic was reversed was to try to always
make sure that the information was uptodate ... but since then and since
the g-bp issue has arisen, we walked through the iterations the other
day and I have not been able to find an actual operational sequence
where it would be a problem where the current situation is any better
(or one not based on versioninfo)

However, just having written that, I believe I've got an even more
bulletproof idea. Given that versioninfo is generated from git data, we
should be able to determine if we are in the git repo that generated the
versioninfo file in the first place, and therefore whether it is safe to
re-generate it. Like so:

  if versioninfo exists:
      version = read_version_info()
  if (.git exists and version in .git) or not versioninfo exists:
      version = do_some_git_magic()
      write_versioninfo(versin)
  else:
      version = "0.0.0.unsupported.archive.type"
  return version

That should handle the following cases:
- clean git checkout
  - will not find versioninfo file
  - will find git
  - will generate versioninfo
- dist tarball
  - will find versioninfo file
  - will use versioninfo file
- dist tarball injected into g-bp repo
  - will find versioninfo file
  - will find .git
  - will not find info in versioninfo in .git
  - will use versioninfo file
- dirty git checkout
  - will find versioninfo file
  - will find .git
  - will find verisioninfo info in .git
  - will generate versioninfo
- git-archive tarball
  - will not find versioninfo
  - will not find .git
  - will return a version of "0.0.0.unsupported.archive.type"

Have I missed a case? If not, I can code up a follow-on patch to the
logic-reversal patch which does the above.

Monty