Merge lp:~yuningdodo/usb-creator/usb-creator.lp1208129-detect-os-version-with-regex into lp:usb-creator

[Yu Ning]

* usbcreator/install.py: detect os version with regex and check for the result. (LP: #1208129)

[Mathieu Trudel-Lapierre (cyphermox)]

Seems like that regex would be bugged, and would match with the full version number including point release with group(0) (full match), and only match the last block satisfying "\d+\." for group(1). You can work around this by modifying it a bit: ((\d+\.)\d+)+?

This further passes a quick unscientific test with a full .disk/info line:

Previously:

In [2]: m = re.search("(\d+\.)+\d+", "Ubuntu 15.04.1 \"Vivid Vervet\" - Alpha amd64 (20150224.3)")

In [3]: m.group(0)
Out[3]: '15.04.1'

In [4]: m.group(1)
Out[4]: '04.'

With changes:

In [5]: m = re.search("((\d+\.)\d+)+?", "Ubuntu 15.04.1 \"Vivid Vervet\" - Alpha amd64 (20150224.3)")

In [6]: m.group(0)
Out[6]: '15.04'

In [7]: m.group(1)
Out[7]: '15.04'

review: Needs Fixing

[Mathieu Trudel-Lapierre (cyphermox)]

Actually, perhaps "((\d+)(\.\d+))((\.\d+)+)?" is an even better regex for this; given that it will also record the point release number should they ever be needed.

[Yu Ning (yuningdodo)]

Mathieu, thanks for the review.

According to the context, my understanding is we should find out the whole version number, such as 14.04.1, then let debian_support.Version() to extract the major version (14.04). Do you think this is reasonable?

[Mathieu Trudel-Lapierre (cyphermox)]

Hmm, looks like you may be right. In that sense, the regex would be absolutely fine.

review: Approve

=== modified file 'usbcreator/install.py'
--- usbcreator/install.py	2013-01-28 12:44:46 +0000
+++ usbcreator/install.py	2015-02-19 07:11:31 +0000
@@ -212,17 +212,21 @@
 
         if os.path.exists(os.path.join(self.target, '.disk', 'info')):
             with open(os.path.join(self.target, '.disk', 'info'),'r') as f:
-                contents = f.readline().split()
-            if len(contents) > 2:
+                contents = f.readline()
+            import re
+            m = re.search('(\d+\.)+\d+', contents)
+            if m:
                 # Consider point releases the same as the initial release
                 # (10.04.4 -> 10.04)
-                target_os_ver = debian_support.Version(contents[1])
+                target_os_ver = debian_support.Version(m.group(0))
 
         return target_os_ver, our_os_ver
 
     def need_syslinux_legacy(self):
         target_os_ver, our_os_ver = self.os_vers()
-        return our_os_ver >= '10.10' and target_os_ver <= '10.04'
+        # FIXME: check for debian, linuxmint, etc.
+        return target_os_ver and our_os_ver \
+                and our_os_ver >= '10.10' and target_os_ver <= '10.04'
 
     def install_bootloader(self, grub_location=''):
         logging.debug('install_bootloader')