545570 - [deb package] Insert '~' between version number part and alpha or beta part

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

The debian package version should use '~' between the main version number and any alpha or beta specifier. We already insert a '~' before any occurrence of "pre" in the version string as well.

1.1a1 -> 1.1~a1
1.1a2pre -> 1.1~a2~pre

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

Uses a python script to chop up the mozilla version string and return a debian version string.

I saw we do similar things with python scripts in other parts of the build system

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Since the nightlies are currently 1.1a2pre, and will become 1.1~a2~datetime, we will need to tell nightly users to uninstall/reinstall because the nightlies won't update otherwise.

Comment 3

[Aakash Desai [:aakashd]]

Do we have any idea how many users are on nightlies?

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 426555 [details] [diff] [review]
patch

The makefile stuff looks good, please have someone else look at the python though.  Its really not something I can competently review.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 426555 [details] [diff] [review]
patch

Ted, can you look at the python part?

Comment 6

[Tuukka Tolvanen (sp3000)]

Comment on attachment 426555 [details] [diff] [review]
patch

+# 2.1a3    > "2.1~a3~datetime" (with optional suffix appended)

the left side should read "2.1a3 datetime" to document usage

+# 3.2      > "3.2"

the actual result for that input is ""

>+from datetime import datetime

this isn't used

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #3)
> Do we have any idea how many users are on nightlies?

Mozilla Metrics currently show zero (0) ADUs from 1.1a2pre

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #6)
> (From update of attachment 426555 [details] [diff] [review])
> +# 2.1a3    > "2.1~a3~datetime" (with optional suffix appended)
> 
> the left side should read "2.1a3 datetime" to document usage

ok, makes sense

> +# 3.2      > "3.2"
> 
> the actual result for that input is ""

crud, I broke something. Adding a '*' after the <type> group fixes it.

> >+from datetime import datetime
> 
> this isn't used

yeah, I started the script with the code to generate the datetime stamp, but decided to move it out. Good clean up.

New patch coming up

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

Addresses review comments

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 426624 [details] [diff] [review]
patch 2

Carrying blassey's r+

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 426624 [details] [diff] [review]
patch 2

>diff --git a/installer/print-debianversion.py b/installer/print-debianversion.py
>new file mode 100644
>--- /dev/null
>+++ b/installer/print-debianversion.py
>+def get_debian_version(version):
>+  """ Returns the debian version from the version string argument """
>+
>+  suffix = ''
>+  if len(sys.argv) == 3:
>+    suffix = '~' + sys.argv[2]

I think you should handle sys.argv[2] outside of get_debian_version, and pass it in as an optional parameter or something, like:
def get_debian_version(version, buildid=None):
  ...

>+
>+  def mfunc(m):
>+    prefix = m.group('prefix')
>+    if prefix == None:
>+        print >>sys.stderr, "version prefix is required"

Seems weird to have a print inside of your regex replacement function. Also, keep your indententation consistent. I prefer 2-space indent, but whatever you use, make it consistent.

>+
>+    type = ''
>+    if m.group('type') <> None:

What are you programming in, VB? Use !=.

>+        type = '~' + m.group('type')
>+
>+    return "%s%s%s" % (prefix, type, suffix)
>+
>+  result, c = re.subn(r'^(?P<prefix>(\d+\.)*\d+)(?P<type>[ab]\d+)*(?P<suffix>pre)*', mfunc, version)

Do you really need the complexity of the replacement function? You could just do an re.search, then pull out the groups and return the string you want. Something like:
m = re.search(r'^(?P<prefix>(\d+\.)*\d+)(?P<type>[ab]\d+)*(?P<suffix>pre)*', version)
if m:
  type = ""
  if m.group("type") is not None:
    type = '~' + m.group('type')
  prefix = ...
  return "%s%s%s" % (prefix, type, suffix)

I guess it works out to more code, but seems more coherent to me.

>+  if c != 1:
>+    return ''

Do you really want to silently return an empty string if this fails? Seems better to have it exit with an error.

>+  return result
>+
>+if len(sys.argv) >= 2:
>+  print get_debian_version(sys.argv[1])

Generally, we like Python scripts to be importable as modules, so you write something like:
def main():
  ...

if __name__ == '__main__':
  main()

Also, if you do that, you could write some simple unit tests for this, which would make me a lot happier. We have some sample Python unittests in the tree here:
http://mxr.mozilla.org/mozilla-central/source/config/tests/
And here's the simple Makefile rule that runs them:
http://mxr.mozilla.org/mozilla-central/source/config/Makefile.in#171

Just some simple tests validating that calling this method with specified versions gives the right result would be fine.

Comment 12

[Aki Sasaki (not active)]

This was fixed a while ago.