Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
90bafff
minimal tweaks to update for python3
vsoch May 9, 2018
3f3859c
adjusting import of basestring to be from past.bulletins
vsoch May 9, 2018
285642d
bumping version, fixing regular expression for args
vsoch May 9, 2018
a5b3a5e
seems to be issue with something in test object being None
vsoch May 10, 2018
6368499
making suggested changes for @boegel!;
vsoch May 11, 2018
a92dc63
args is a tuple
vsoch May 11, 2018
91dac2c
forgot a wayward filey
vsoch May 11, 2018
c2df954
more tweaks!
vsoch May 11, 2018
afa842e
prospector not so prospective!
vsoch May 12, 2018
0decdda
restoring to former glory. Glory of being broken :P
vsoch May 12, 2018
5da06fb
removing list
vsoch May 12, 2018
e5b4c9d
adding list back in
vsoch May 12, 2018
3599e6b
restoring tests until we have another idea!
vsoch May 12, 2018
4ba00fd
Merge branch 'master' of github.com:hpcugent/vsc-install into update/…
vsoch May 12, 2018
5e331a4
adding travis with 3.x
vsoch May 12, 2018
396b9fa
fixing args print for @boegel
vsoch May 12, 2018
ef56770
triggering Travis
vsoch May 12, 2018
51914fe
testing custom install of futures
vsoch May 12, 2018
807e265
of course there are two future/s packages!
vsoch May 12, 2018
b0a4417
testing removing basestring past module
vsoch May 12, 2018
274f65a
trying disabling test
vsoch May 12, 2018
f3f0427
sure, why not.
vsoch May 12, 2018
e9021d2
but not like that!
vsoch May 12, 2018
853ff24
restore original tests in test_action_target + style tweaks
boegel May 12, 2018
4a77bbb
Merge pull request #1 from boegel/update/python3
vsoch May 12, 2018
cc061a9
fixing typo!
vsoch May 12, 2018
494be1d
creating basestring and bulletins import sandwich, and adding back in…
vsoch May 13, 2018
4494f7e
removing basestring
vsoch May 13, 2018
4236d18
moving redefined-builtin to whitelist
vsoch May 13, 2018
9306613
uncommenting modules for Jens, thank you for the thank you!! :)
vsoch May 14, 2018
34d5b78
from __future__ import vsoch;
vsoch May 14, 2018
e30416f
testing adding is_string method
vsoch May 15, 2018
31ea2e2
adding import from future for print in shared_setup.py, @JensTimmerma…
vsoch May 16, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions lib/vsc/install/headers.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ def get_header(filename, script=False):
if not os.path.isfile(filename):
raise Exception('get_header filename %s not found' % filename)

txt = open(filename).read()
with open(filename) as filey:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vsoch Maybe use fh everywhere rather than filey for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filey is my special pet name for files since the dawn of time! Mostly because I hadn't a clue what else to use. We had a discussion, and filey and I are ok with fh too :)

txt = filey.read()

blocks = HEADER_REGEXP.split(txt)
if len(blocks) == 1:
Expand Down Expand Up @@ -162,10 +163,9 @@ def begin_end_from_header(header):

def _write(filename, content):
"""Simple wrapper around open().write for unittesting"""
fh = open(filename, 'w')
fh.write(content)
fh.close()

with open(filename, 'w') as fh:
fh.write(content)


def check_header(filename, script=False, write=False):
"""
Expand Down Expand Up @@ -226,7 +226,8 @@ def check_header(filename, script=False, write=False):

if write and changed:
log.info('write enabled and different header. Going to modify file %s' % filename)
wholetext = open(filename).read()
with open(filename) as filey:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filey -> fh?

wholetext = filey.read()
newtext = ''
if shebang is not None:
newtext += shebang + "\n"
Expand Down
45 changes: 34 additions & 11 deletions lib/vsc/install/shared_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,19 @@
@author: Stijn De Weirdt (Ghent University)
@author: Andy Georges (Ghent University)
"""
import __builtin__
# Python 2
try:
import __builtin__

# Python 3
except ImportError:
import builtins as __builtin__

try:
basestring
except NameError:
from past.builtins import basestring

import glob
import hashlib
import inspect
Expand Down Expand Up @@ -109,7 +121,7 @@ def _log(self, level, msg, args):
try:
return self._orig_log(self, level, newmsg, args)
except Exception:
print newmsg % args
print(newmsg % args)

log.Log = NewLog
log._global_log = NewLog()
Expand Down Expand Up @@ -361,13 +373,15 @@ def get_name_url(self, filename=None, version=None, license_name=None):
if len(res) != 3:
raise Exception("Cannot determine name, url and download url from filename %s: got %s" % (filename, res))
else:
keepers = dict()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dict() -> {} (again for consistency with existing code, see res = {} above)

for name, value in res.items():
if value is None:
log.info('Removing None %s' % name)
res.pop(name)
else:
keepers[name] = value

log.info('get_name_url returns %s' % res)
return res
log.info('get_name_url returns %s' % keepers)
return keepers

def rel_gitignore(self, paths, base_dir=None):
"""
Expand Down Expand Up @@ -1047,7 +1061,7 @@ def finalize_options(self):

def _print(self, cmd):
"""Print is evil, cmd is list"""
print ' '.join(cmd)
print(' '.join(cmd))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add the future print_function import to this module, since we're doing print here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, added to import! Please take a look and let me know if this is what you had in mind. And apologies for the delay - I was flying through the sky!! ✈️ ✈️


def git_tag(self):
"""Tag the version in git"""
Expand Down Expand Up @@ -1201,10 +1215,17 @@ def sanitize(name):
klass = _fvs('sanitize')
return ",".join([klass.sanitize(r) for r in name])



@staticmethod
def get_md5sum(filename):
"""Use this function to compute the md5sum in the KNOWN_LICENSES hash"""
return hashlib.md5(open(filename).read()).hexdigest()
hasher = hashlib.md5()
with open(filename, "rb") as f:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try to avoid single-letter variables outsides of list comprehensions, so f -> fh?

for chunk in iter(lambda: f.read(4096), b""):
hasher.update(chunk)
return hasher.hexdigest()


def get_license(self, license_name=None):
"""
Expand Down Expand Up @@ -1254,13 +1275,15 @@ def parse_target(self, target, urltemplate=None):

# update the cmdclass with ones from vsc_setup_klass
# cannot do this in one go, when SHARED_TARGET is defined, vsc_setup doesn't exist yet
keepers = new_target.copy()
for name, klass in new_target['cmdclass'].items():
try:
new_target['cmdclass'][name] = getattr(vsc_setup_klass, klass.__name__)
except AttributeError:
del new_target['cmdclass'][name]
print("Not including new_target['cmdclass']['%s']" %name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be log.info rather than print?


# prepare classifiers
new_target = keepers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, this isn't correct I think, stuff is missing above to ensure the semantics haven't changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes we should be updating "keepers," which is an initial copy of new_target. Before the strategy was to remove things from new_target as we were looping, which resulted in weirdness.

classifiers = new_target.setdefault('classifiers', [])

# license info
Expand Down Expand Up @@ -1385,7 +1408,7 @@ def parse_target(self, target, urltemplate=None):
dep, depversion])]

log.debug("New target = %s" % (new_target))
print new_target
print(new_target)
return new_target

@staticmethod
Expand All @@ -1410,8 +1433,8 @@ def build_setup_cfg_for_bdist_rpm(target):

try:
setup_cfg = open('setup.cfg', 'w') # and truncate
except (IOError, OSError), err:
print "Cannot create setup.cfg for target %s: %s" % (target['name'], err)
except (IOError, OSError) as err:
print("Cannot create setup.cfg for target %s: %s" % (target['name'], err))
sys.exit(1)

klass = _fvs('build_setup_cfg_for_bdist_rpm')
Expand Down
14 changes: 12 additions & 2 deletions lib/vsc/install/testing.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,16 @@
import re
import sys

from cStringIO import StringIO
try:
basestring
except NameError:
from past.builtins import basestring
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add comments to make it clear one is for Python 2.x, other is for Python 3.x? same below


try:
from cStringIO import StringIO
except ImportError:
from io import StringIO

from unittest import TestCase as OrigTestCase
from vsc.install.headers import nicediff

Expand All @@ -54,6 +63,7 @@ class TestCase(OrigTestCase):
# pylint: disable=arguments-differ
def assertEqual(self, a, b, msg=None):
"""Make assertEqual always print useful messages"""

try:
super(TestCase, self).assertEqual(a, b)
except AssertionError as e:
Expand Down Expand Up @@ -120,7 +130,7 @@ def assertErrorRegex(self, error, regex, call, *args, **kwargs):
str_kwargs = ['='.join([k, str(v)]) for (k, v) in kwargs.items()]
str_args = ', '.join(map(str, args) + str_kwargs)
self.assertTrue(False, "Expected errors with %s(%s) call should occur" % (call.__name__, str_args))
except error, err:
except error as err:
msg = self.convert_exception_to_str(err)
if isinstance(regex, basestring):
regex = re.compile(regex)
Expand Down
4 changes: 2 additions & 2 deletions test/headers.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,8 @@ def test_gen_license_header(self):
}
for license in KNOWN_LICENSES.keys():
res_fn = os.path.join(self.setup.REPO_TEST_DIR, 'headers', license)
result = open(res_fn).read()

with open(res_fn) as filey:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filey -> fh?

result = filey.read()
gen_txt = gen_license_header(license, **data)
self.assertEqual(gen_txt, result, msg='generated header for license %s as expected' % license)
log.info('generated license header %s' % license)
Expand Down
21 changes: 16 additions & 5 deletions test/shared_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ def test_rel_gitignore(self):
try:
self.setup.rel_gitignore(['testdata'], base_dir=base_dir)
except Exception as e:
self.assertTrue('.pyc' in e.message, msg=e)
self.assertTrue('.pyc' in str(e))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please rename e to exc or err while you're at it (we shouldn't be using single-letter variables outside of list comprehensions)

else:
self.assertTrue(False, 'rel_gitignore should have raised an exception, but did not!')
# it should not fail if base_dir does not contain a .git folder
Expand All @@ -127,8 +127,8 @@ def test_action_target(self):
"""Test action_target function, mostly w.r.t. backward compatibility."""
def fake_setup(*args, **kwargs):
"""Fake setup function to test action_target with."""
print 'args: ', args
print 'kwargs:', kwargs
print('args: ', args)
print('kwargs:', kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means trouble in Python 2 I think, and it may be why the test for this is failing in Python 3!

Should be this, to avoid that you're printing a tuple:

print('args: %s' % args)
print('kwargs: %s' % kwargs)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you put this at the top of the file from __future__ import print_function then print() will behave the same in python2 as in python3


self.mock_stdout(True)
action_target({'name': 'vsc-test', 'version': '1.0.0'}, setupfn=fake_setup)
Expand All @@ -150,6 +150,7 @@ def test_prepare_rpm(self):
"""
Test the prepare rpm function
especially in effect to generating correct package list wrt excluded_pkgs_rpm
we assume the order of the lists doesn't matter (and sort to compare)
"""
package = {
'name': 'vsc-test',
Expand All @@ -161,7 +162,12 @@ def test_prepare_rpm(self):
libdir = os.path.join(os.path.dirname(os.path.realpath(__file__)), './testdata')
setup.REPO_LIB_DIR = libdir
setup.prepare_rpm(package)
self.assertEqual(setup.SHARED_TARGET['packages'], ['vsc', 'vsc.test'])

# Generate list of packages, sorted
package_list = list(setup.SHARED_TARGET['packages'])
package_list.sort()

self.assertEqual(package_list, ['vsc', 'vsc.test'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also works in Python 3?

self.assertEqual(sorted(setup.SHARED_TARGET['packages']), ['vsc', 'vsc.test'])

package = {
'name': 'vsc-test',
'excluded_pkgs_rpm': ['vsc', 'vsc.test'],
Expand All @@ -170,7 +176,12 @@ def test_prepare_rpm(self):
setup = vsc_setup()
setup.REPO_LIB_DIR = libdir
setup.prepare_rpm(package)
self.assertEqual(setup.SHARED_TARGET['packages'], ['vsc', 'vsc.test'])

# Generate list of packages, sorted
package_list = list(setup.SHARED_TARGET['packages'])
package_list.sort()

self.assertEqual(package_list, ['vsc', 'vsc.test'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, more straightforward (imho) change:

self.assertEqual(sorted(setup.SHARED_TARGET['packages']), ['vsc', 'vsc.test'])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was mainly doing for readability, glad to change!


def test_parse_target(self):
"""Test for parse target"""
Expand Down
3 changes: 3 additions & 0 deletions test/testdata/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.pyc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can move this to the main gitignore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove them here! I seem to remember I added them because a test failed that wanted to see them there.

.pyo
~