On 09/17/2013 08:54 AM, Viktor Mihajlovski wrote:

Commit 4a7fae9 changed the revision returned by get_provider_version to a string. However, the revision has to be a number because (since git is used) it's basically the number of commits in HEAD. Comparing a string against an int in Python 2.x will always result in the string value being greater than the int value. Since there are a lot of compares against revision numbers all over the test scripts, this change is probably the most economic.

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- Not being a python guy I got wacked myself when I was testing my (soon to come) cimtest changes against a pre-console libvirt-cim. '1272' > 1276 silently returned True leading to epic failures :-(

suites/libvirt-cim/lib/XenKvmLib/const.py | 4 +++- suites/libvirt-cim/lib/XenKvmLib/reporting.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-)

I guess this is a mea culpa and me too moment all wrapped into one. Python isn't necessarily my strength and I probably should have looked more deeply into all the callers. I guess I was too hyperfocused on the error I got that resulted in the bad changes I made...

diff --git a/suites/libvirt-cim/lib/XenKvmLib/const.py b/suites/libvirt-cim/lib/XenKvmLib/const.py index a8d0254..a454b2f 100755 --- a/suites/libvirt-cim/lib/XenKvmLib/const.py +++ b/suites/libvirt-cim/lib/XenKvmLib/const.py @@ -179,7 +179,9 @@ def get_provider_version(virt, ip): revision = revision.strip("+") if revision.isdigit(): revision = int(revision) + else: + raise Exception("revision %s is not a digit", revision)

- return str(revision), str(changeset) + return revision, changeset

diff --git a/suites/libvirt-cim/lib/XenKvmLib/reporting.py b/suites/libvirt-cim/lib/XenKvmLib/reporting.py index 67ec974..88375b0 100644 --- a/suites/libvirt-cim/lib/XenKvmLib/reporting.py +++ b/suites/libvirt-cim/lib/XenKvmLib/reporting.py @@ -101,7 +101,7 @@ def get_env_data(ip, virt): rev, changeset = get_provider_version(virt, ip) cimtest_revision, cimtest_changeset = get_cimtest_version()

- lc_ver = "Libvirt-cim revision: %s\nLibvirt-cim changeset: %s\n" % \ + lc_ver = "Libvirt-cim revision: %d\nLibvirt-cim changeset: %s\n" % \

Still not quite right - 'changeset' is more of a hexadecimal value, but printing as such (either %x or %d) would result in a traceback and the error message "a number is required, not unicode". If I print 'Revision' and 'Changeset' in 'get_provider_version' right after they are fetched from 'inst', I get: 1262 eb776e2 To "fix" I changed the "(rev, changeset)" to "(rev, str(changeset))" I adjusted and pushed. Of course I did a bit more digging just to be sure... 'changeset' is ever compared against is 'sles11_changeset' It's initially generated in the libvirt-cim build as a "'git rev-parse --short HEAD' > .changeset" by autoconfiscate.sh. If unavailable it's set to 'Unknown' in the configure script as part of a build environment variable "LIBVIRT_CIM_CS". While at it - I checked the generation of "revision" too. In this case, it's the result of the number of commits from a "git rev-list HEAD | wc -l > .revision". It's then saved/used as the build environment variable "LIBVIRT_CIM_RV". In my example, it shows as 1262, although to be technically correct it should be 1279. If it's not possible to get the value it defaults to '0' (zero). So printing/using as an 'int' is certainly the right thing to do. I'm not a "build guru", but I think the libvirt-cim build probably needs to generate the values more frequently than just in autoconfiscate.sh seeing as that's 'perhaps' run less frequently than someone making changes to the git repository and rebuilding... John

(rev, changeset) cimtest_ver = "Cimtest revision: %s\nCimtest changeset: %s\n" % \ (cimtest_revision, cimtest_changeset)