You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Edmund Wong <ed...@kdtc.net> on 2009/09/13 17:36:44 UTC
[PATCH] v2 Fixed gen_win.py to display Serf library version#
Hi,
After going over the review by arfrever, lgo and Bert, I've made a
new patch. Thanks guys.
Edmund
Log:
[[[
Fixed gen_win.py such that it checks for the presence of the
Serf library (which it does prior to this patch) and also
outputs the library version (which is what this patch does).
* build/generator/gen_win.py
(_get_serf_version): New function to return the serf library
version in a 3-tuple format.
(_find_serf): Added a version check. If the 3-tuple version
structure has -1 in the first index, the serf library is
not built; otherwise, the library is set to be built. Then
a message is displayed informing the user of the results.
Patch by: Edmund Wong <ed at kdtc.net>
Suggested by: danielsh
Review by: arfrever
lgo
rhuijben
]]]
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2394318
Re: [PATCH] v2 Fixed gen_win.py to display Serf library version#
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Edmund Wong wrote on Mon, 14 Sep 2009 at 08:49 +0800:
> That is a point. Regexes isn't my strong point. So would the
> following suffice?
>
> maj_t = re.compile(r'SERF_MAJOR_VERSION(\s+)(\d+)')
>
Yes, but you don't need the first pair of parentheses, since you aren't
interested either in capturing the whitespace and or in the "grouping"
semantics of parentheses.
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2394398
Re: [PATCH] v2 Fixed gen_win.py to display Serf library version#
Posted by Edmund Wong <ed...@kdtc.net>.
Daniel Shahaf wrote:
> Edmund Wong wrote on Mon, 14 Sep 2009 at 01:36 +0800:
>> [[[
>>
>> Fixed gen_win.py such that it checks for the presence of the
>> Serf library (which it does prior to this patch) and also
>> outputs the library version (which is what this patch does).
>>
>> * build/generator/gen_win.py
>> (_get_serf_version): New function to return the serf library
>> version in a 3-tuple format.
>
> The '(' should be indented two spaces.
I'll get this fixed.
>
>> (_find_serf): Added a version check. If the 3-tuple version
>> structure has -1 in the first index, the serf library is
>> not built; otherwise, the library is set to be built. Then
>> a message is displayed informing the user of the results.
>>
>
> IMO, the part about -1 and 3-tuple is a low-enough implementation detail
> to not belong in the log message. Just say what the added code thus
> (extract the version, message if successful, skip building(?) otherwise).
I see. I'll modify that part of the log message.
>> +
>> + if self.serf_path and os.path.exists(self.serf_path):
>> + if (os.path.exists(os.path.join(self.serf_path, 'serf.h'))):
>> + fp = open(os.path.join(self.serf_path, 'serf.h'))
>> + txt = fp.read()
>> + maj_t = re.compile(r'SERF_MAJOR_VERSION (\d+)')
>> + min_t = re.compile(r'SERF_MINOR_VERSION (\d+)')
>> + patch_t = re.compile(r'SERF_PATCH_VERSION (\d+)')
>> + ver_maj_re = maj_t.search(txt)
>> + ver_min_re = min_t.search(txt)
>> + ver_patch_re = patch_t.search(txt)
>
> This reads the entire file to the variable 'txt'. It would be better to
> read it incrementally (e.g., one line at a time).
I just realized i didn't even close the file.
>
> (And perhaps relax the regexes by allowing any number of whitespace
> there? This is almost a matter of personal taste, though.)
That is a point. Regexes isn't my strong point. So would the
following suffice?
maj_t = re.compile(r'SERF_MAJOR_VERSION(\s+)(\d+)')
Edmund
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2394394
Re: [PATCH] v2 Fixed gen_win.py to display Serf library version#
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Edmund Wong wrote on Mon, 14 Sep 2009 at 01:36 +0800:
> [[[
>
> Fixed gen_win.py such that it checks for the presence of the
> Serf library (which it does prior to this patch) and also
> outputs the library version (which is what this patch does).
>
> * build/generator/gen_win.py
> (_get_serf_version): New function to return the serf library
> version in a 3-tuple format.
The '(' should be indented two spaces.
> (_find_serf): Added a version check. If the 3-tuple version
> structure has -1 in the first index, the serf library is
> not built; otherwise, the library is set to be built. Then
> a message is displayed informing the user of the results.
>
IMO, the part about -1 and 3-tuple is a low-enough implementation detail
to not belong in the log message. Just say what the added code thus
(extract the version, message if successful, skip building(?) otherwise).
> Index: build/generator/gen_win.py
> ===================================================================
> --- build/generator/gen_win.py (revision 39272)
> +++ build/generator/gen_win.py (working copy)
> @@ -1318,6 +1318,32 @@
>
> sys.stderr.write(msg)
>
> + def _get_serf_version(self):
> + "Retrieves the serf version from serf.h"
> +
> + ver_maj = -1
> + ver_min = -1
> + ver_patch = -1
> +
> + if self.serf_path and os.path.exists(self.serf_path):
> + if (os.path.exists(os.path.join(self.serf_path, 'serf.h'))):
> + fp = open(os.path.join(self.serf_path, 'serf.h'))
> + txt = fp.read()
> + maj_t = re.compile(r'SERF_MAJOR_VERSION (\d+)')
> + min_t = re.compile(r'SERF_MINOR_VERSION (\d+)')
> + patch_t = re.compile(r'SERF_PATCH_VERSION (\d+)')
> + ver_maj_re = maj_t.search(txt)
> + ver_min_re = min_t.search(txt)
> + ver_patch_re = patch_t.search(txt)
This reads the entire file to the variable 'txt'. It would be better to
read it incrementally (e.g., one line at a time).
(And perhaps relax the regexes by allowing any number of whitespace
there? This is almost a matter of personal taste, though.)
I can't test the patch because right now I'm not on windows.
> + if ver_maj_re:
> + ver_maj = int(ver_maj_re.groups()[0])
> + if ver_min_re:
> + ver_min = int(ver_min_re.groups()[0])
> + if ver_patch_re:
> + ver_patch = int(ver_patch_re.groups()[0])
> +
> + return (ver_maj, ver_min, ver_patch)
> +
> def _find_serf(self):
> "Check if serf and its dependencies are available"
>
> @@ -1325,6 +1351,15 @@
> if self.serf_path and os.path.exists(self.serf_path):
> if self.openssl_path and os.path.exists(self.openssl_path):
> self.serf_lib = 'serf'
> + version = self._get_serf_version()
> + if version[0] != -1:
> + self.serf_ver = str(version[0]) +"."+str(version[1]) + "." + \
> + str(version[2])
> + msg = 'Found serf version %s\n' % self.serf_ver
> + else:
> + msg = 'Could not determine version, ra_serf will not be built\n'
> + self.serf_lib = None
> + sys.stderr.write(msg)
> else:
> sys.stderr.write('openssl not found, ra_serf will not be built\n')
> else:
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2394370
Re: [PATCH] v2 Fixed gen_win.py to display Serf library version#
Posted by Edmund Wong <ed...@kdtc.net>.
Roman Donchenko wrote:
> I think if serf\serf.h exists but version can't be determined, it should
> print "unknown version", but still try to enable Serf. That way, it will
> be less trouble if some extraneous spaces crop up, or something.
I guess if I lightened the regex a bit, this case might not happen.
Of course, it isn't a good excuse to not allow such an eventuality
happen.
Thanks
Edmund
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2394397
Re: [PATCH] v2 Fixed gen_win.py to display Serf library version#
Posted by Roman Donchenko <DX...@yandex.ru>.
Edmund Wong <ed...@kdtc.net> писал в своём письме Sun, 13 Sep 2009 21:36:44
+0400:
> Hi,
>
> After going over the review by arfrever, lgo and Bert, I've made a
> new patch. Thanks guys.
>
> Edmund
>
> Log:
>
> [[[
>
> Fixed gen_win.py such that it checks for the presence of the
> Serf library (which it does prior to this patch) and also
> outputs the library version (which is what this patch does).
>
> * build/generator/gen_win.py
> (_get_serf_version): New function to return the serf library
> version in a 3-tuple format.
> (_find_serf): Added a version check. If the 3-tuple version
> structure has -1 in the first index, the serf library is
> not built; otherwise, the library is set to be built. Then
> a message is displayed informing the user of the results.
>
> Patch by: Edmund Wong <ed at kdtc.net>
> Suggested by: danielsh
> Review by: arfrever
> lgo
> rhuijben
>
> ]]]
I think if serf\serf.h exists but version can't be determined, it should
print "unknown version", but still try to enable Serf. That way, it will
be less trouble if some extraneous spaces crop up, or something.
Roman.
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2394376