You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@madlib.apache.org by iyerr3 <gi...@git.apache.org> on 2016/10/17 19:24:24 UTC
[GitHub] incubator-madlib pull request #70: Build: Fix version parsing in madpack
GitHub user iyerr3 opened a pull request:
https://github.com/apache/incubator-madlib/pull/70
Build: Fix version parsing in madpack
Primary change is line 359. Other changes are whitespace fixes.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/iyerr3/incubator-madlib bugfix/madpack_version_parse
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-madlib/pull/70.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #70
----
commit 560e32de98d2303540cb8f0a42aad552eb75db96
Author: Rahul Iyer <ri...@apache.org>
Date: 2016-10-17T18:47:29Z
Build: Fix version parsing in madpack
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-madlib issue #70: Build: Fix version parsing in madpack
Posted by stephendotcarter <gi...@git.apache.org>.
Github user stephendotcarter commented on the issue:
https://github.com/apache/incubator-madlib/pull/70
Returning elements as str does not work when comparing different versions on line and is causing the install to fail on Greenplum 4.3.10:
```
if _get_rev_num(match_details.group(1)) >= _get_rev_num('4.3.5'):
return '4.3ORCA'
```
This evaluates as:
```
>>> ['4','3','10'] > ['4','3','5']
False
```
They need to be int for the comparison to work:
```
>>> [4,3,10] > [4,3,5]
True
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-madlib issue #70: Build: Fix version parsing in madpack
Posted by stephendotcarter <gi...@git.apache.org>.
Github user stephendotcarter commented on the issue:
https://github.com/apache/incubator-madlib/pull/70
There is another line where we need to convert back to str for concatenation:
```
https://github.com/iyerr3/incubator-madlib/blob/0e5aa9f330c706a3d1e715879bbfd792953f1834/src/madpack/madpack.py#L518
temp_schema = schema + '_v' + ''.join(str(_get_rev_num(dbrev)))
```
In testing this gives me:
```
>>> ver = [4,3,10]
>>> 'test' + ''.join(str(ver))
'test[4, 3, 10]'
```
I think the line should be:
```
>>> 'test' + ''.join(map(str, ver))
'test4310'
```
And following should use `_is_rev_gte`:
```
https://github.com/iyerr3/incubator-madlib/blob/0e5aa9f330c706a3d1e715879bbfd792953f1834/src/madpack/madpack.py#L323
if _get_rev_num(match_details.group(1)) >= _get_rev_num('4.3.5'):
```
Other than that I have not seen any other locations that call `_get_rev_num`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-madlib pull request #70: Build: Fix version parsing in madpack
Posted by mktal <gi...@git.apache.org>.
Github user mktal commented on a diff in the pull request:
https://github.com/apache/incubator-madlib/pull/70#discussion_r83715757
--- Diff: src/madpack/madpack.py ---
@@ -357,7 +356,7 @@ def _get_rev_num(rev):
@param rev version text
"""
try:
- num = re.findall('[0-9]', rev)
--- End diff --
In that case we could use `num = re.findall('[0-9]+', rev)`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-madlib pull request #70: Build: Fix version parsing in madpack
Posted by iyerr3 <gi...@git.apache.org>.
Github user iyerr3 commented on a diff in the pull request:
https://github.com/apache/incubator-madlib/pull/70#discussion_r83716925
--- Diff: src/madpack/madpack.py ---
@@ -357,7 +356,7 @@ def _get_rev_num(rev):
@param rev version text
"""
try:
- num = re.findall('[0-9]', rev)
--- End diff --
The primary bug in `re.findall('[0-9]', rev)` was that a version string like `1.3.10` resulted in output `[1, 3, 1, 0]` instead of `[1, 3, 10]`. This PR solely aims to fix that. Ideally I would like to use something like `distutils.version.LooseVersion` to compare versions, but we also need the version parsed in a list and I'm not aware if `LooseVersion` provides that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-madlib pull request #70: Build: Fix version parsing in madpack
Posted by mktal <gi...@git.apache.org>.
Github user mktal commented on a diff in the pull request:
https://github.com/apache/incubator-madlib/pull/70#discussion_r83715452
--- Diff: src/madpack/madpack.py ---
@@ -357,7 +356,7 @@ def _get_rev_num(rev):
@param rev version text
"""
try:
- num = re.findall('[0-9]', rev)
--- End diff --
Could rev be something like `Greenplum Database 4.3.10.0`, containing text before the version number?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-madlib issue #70: Build: Fix version parsing in madpack
Posted by iyerr3 <gi...@git.apache.org>.
Github user iyerr3 commented on the issue:
https://github.com/apache/incubator-madlib/pull/70
Need final approval to close this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-madlib issue #70: Build: Fix version parsing in madpack
Posted by iyerr3 <gi...@git.apache.org>.
Github user iyerr3 commented on the issue:
https://github.com/apache/incubator-madlib/pull/70
Thanks! Updated code.
On Sun, Oct 30, 2016 at 9:39 AM, Stephen Carter <no...@github.com>
wrote:
> There is another line where we need to convert back to str for
> concatenation:
>
> https://github.com/iyerr3/incubator-madlib/blob/0e5aa9f330c706a3d1e715879bbfd792953f1834/src/madpack/madpack.py#L518
> temp_schema = schema + '_v' + ''.join(str(_get_rev_num(dbrev)))
>
> In testing this gives me:
>
> >>> ver = [4,3,10]
> >>> 'test' + ''.join(str(ver))
> 'test[4, 3, 10]'
>
> I think the line should be:
>
> >>> 'test' + ''.join(map(str, ver))
> 'test4310'
>
> And following should use _is_rev_gte:
>
> https://github.com/iyerr3/incubator-madlib/blob/0e5aa9f330c706a3d1e715879bbfd792953f1834/src/madpack/madpack.py#L323
> if _get_rev_num(match_details.group(1)) >= _get_rev_num('4.3.5'):
>
> Other than that I have not seen any other locations that call _get_rev_num
>
> \u2014
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/incubator-madlib/pull/70#issuecomment-257161952>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ACIkB8oFuVQOI_ogbi02z4Cfh89YQMmmks5q5MgrgaJpZM4KZAPH>
> .
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-madlib issue #70: Build: Fix version parsing in madpack
Posted by fmcquillan99 <gi...@git.apache.org>.
Github user fmcquillan99 commented on the issue:
https://github.com/apache/incubator-madlib/pull/70
@iyerr3 Thanks for catching this
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-madlib pull request #70: Build: Fix version parsing in madpack
Posted by iyerr3 <gi...@git.apache.org>.
Github user iyerr3 commented on a diff in the pull request:
https://github.com/apache/incubator-madlib/pull/70#discussion_r83716471
--- Diff: src/madpack/madpack.py ---
@@ -357,7 +356,7 @@ def _get_rev_num(rev):
@param rev version text
"""
try:
- num = re.findall('[0-9]', rev)
--- End diff --
All the usages of `_get_rev_num` indicates that we only send a pre-parsed version number containing just the digits. I guess we can indicate that further in the doc string
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-madlib pull request #70: Build: Fix version parsing in madpack
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-madlib/pull/70
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-madlib issue #70: Build: Fix version parsing in madpack
Posted by iyerr3 <gi...@git.apache.org>.
Github user iyerr3 commented on the issue:
https://github.com/apache/incubator-madlib/pull/70
Apologies for the rebased branch. New changes include:
```
Changes:
- Fix madpack to assume revision string can be any valid Semantic
versioning scheme.
- Change MADlib version to 1.10.0-dev
- Update MADLIB_VERSION_STRING to use underscore instead of the possible
hyphen since some downstream programs (like gppkg) don't accept hyphen
in the version string.
```
Please review and comment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-madlib issue #70: Build: Fix version parsing in madpack
Posted by iyerr3 <gi...@git.apache.org>.
Github user iyerr3 commented on the issue:
https://github.com/apache/incubator-madlib/pull/70
@fmcquillan99 This fix is necessary to correctly install on any platform that reports a sub-version number `10` or higher.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-madlib pull request #70: Build: Fix version parsing in madpack
Posted by mktal <gi...@git.apache.org>.
Github user mktal commented on a diff in the pull request:
https://github.com/apache/incubator-madlib/pull/70#discussion_r83716436
--- Diff: src/madpack/madpack.py ---
@@ -357,7 +356,7 @@ def _get_rev_num(rev):
@param rev version text
"""
try:
- num = re.findall('[0-9]', rev)
--- End diff --
Seems that rev is from `match_details.group(1)` which only contains numbers. Feel free to ignore the above comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-madlib issue #70: Build: Fix version parsing in madpack
Posted by iyerr3 <gi...@git.apache.org>.
Github user iyerr3 commented on the issue:
https://github.com/apache/incubator-madlib/pull/70
See recent update to enable versions of the form "1.9.1-rc". Per semantic versioning, all identifier labels should be a suffix starting with a hyphen. This is considered a valid version string.
The caveat is that we don't yet have: `1.9.1 > 1.9.1-rc` which is one of the requirements of semantic versioning.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-madlib issue #70: Build: Fix version parsing in madpack
Posted by iyerr3 <gi...@git.apache.org>.
Github user iyerr3 commented on the issue:
https://github.com/apache/incubator-madlib/pull/70
Thanks for the comment, @stephendotcarter.
I thought I had changed that to int last week but clearly I didn't push to remote. Please check the updated code - I've also added some unittests at the end of the file for basic sanity check (`RUN_TESTS` will be changed to `False` before merging).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---