Skip to content
Merged
Show file tree
Hide file tree
Changes from 27 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
3 changes: 3 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ language: python
python:
- "2.6"
- "2.7"
- "3.5"
- "3.6"

script:
# 'hpcugent' is a mandatory remote for setup.py to work (because of get_name_url)
- git remote add hpcugent https://github.com/hpcugent/vsc-install.git
Expand Down
1 change: 0 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,6 @@ except (ExceptionOne, ExceptionTwo) ...
turning off these errors
-------------------------


If in any of these cases you think: yes, I really needed to do this,
I'm monkeypatching things, I'm adding extra functionality that does indeed have an extra(default) paramenter, etc, etc
you can let pylint know to ignore this error in this one specific block of code
Expand Down
9 changes: 8 additions & 1 deletion lib/vsc/install/commontest.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

@author: Stijn De Weirdt (Ghent University)
"""

import logging
import optparse
import os
Expand Down Expand Up @@ -63,6 +64,11 @@
except ImportError:
pass

# Prospector doesn't have support for 3.5 / 3.6
# https://github.com/PyCQA/prospector/issues/233
if sys.version_info >= (3, 5):
HAS_PROTECTOR = False
Copy link
Member

Choose a reason for hiding this comment

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

can you fix the typo everywhere? (@JensTimmerman unless this is intentional)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The doesn;t is my mistake, and I'd be happy to fix that!

Copy link
Member

Choose a reason for hiding this comment

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

i meant this (old, not your) typo: HAS_PROTECTOR -> HAS_PROSPECTOR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha, I didn't even notice that bit! In that variable names should be meaningful, and in that this is referring to prospector, I too think this should be corrected. I don't see the variable anywhere else in the vsc-install and did a grep for vsc-base as well, so as long as it's not imported in any of the lower lower vsc tools modules, it should be ok to change.



class CommonTest(TestCase):
"""
Expand All @@ -87,15 +93,16 @@ class CommonTest(TestCase):
# Whitelist: if match, fail test
PROSPECTOR_BLACKLIST = [
# 'wrong-import-position', # not sure about this, these usually have a good reason
"Redefining built-in 'basestring'", # basestring is defined when running on top of Python 3.x
Copy link
Member

Choose a reason for hiding this comment

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

actually, you should remove this one too. just ack the pylint failure on the 2 lines in vsc-install where you do the redefinition.
for all other repos that use this, it's better to introduce a is_string function in vsc.utils.missing, and in that module, also ack the redefinition (or better yet, make a function that doesn't require a redefinition). i'm going to assume that most use cases of basestring are from isinstance(somestring, basestring), so that test should be modified everywhere (it's more work, but much cleaner)

'Locally disabling', # shows up when you locally disable a warning, this is the point
'Useless suppression', # shows up when you locally disable/suppress a warning, this is the point
'redefined-builtin',
Copy link
Member

Choose a reason for hiding this comment

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

readd it to the whitelist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry to be clear - it originally was on the blacklist, and you want it removed from the blacklist and added instead to the white list?

]
# to dissable any of these warnings in a block, you can do things like add a comment # pylint: disable=C0321
PROSPECTOR_WHITELIST = [
'undefined',
'no-value-for-parameter',
'dangerous-default-value',
'redefined-builtin',
Copy link
Member

Choose a reason for hiding this comment

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

why is this removed? isn't adding the blacklist entry sufficient?

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 think it is being removed in favor of the blacklist entry, which is more specific for the intended bullitin:

"Redefining built-in 'basestring'"

Copy link
Member

Choose a reason for hiding this comment

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

i think this disables all redefined builtin errors. you should keep this one

'bare-except',
'E713', # not 'c' in d: -> 'c' not in d:
'arguments-differ',
Expand Down
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 fh:
txt = fh.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 fh:
wholetext = fh.read()
newtext = ''
if shebang is not None:
newtext += shebang + "\n"
Expand Down
51 changes: 35 additions & 16 deletions lib/vsc/install/shared_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,21 @@
@author: Stijn De Weirdt (Ghent University)
@author: Andy Georges (Ghent University)
"""
import __builtin__

import sys

if sys.version_info < (3, 0):
import __builtin__
else:
basestring = (bytes, str) # make sure 'basestring' (a builtin in Python 2.x) is defined when using Python 3
import builtins as __builtin__ # make builtins accessible via same way as in Python 3

import glob
import hashlib
import inspect
import json
import os
import shutil
import sys
import re

import setuptools.command.test
Expand Down Expand Up @@ -109,7 +116,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 @@ -148,7 +155,7 @@ def _log(self, level, msg, args):

RELOAD_VSC_MODS = False

VERSION = '0.10.32'
VERSION = '0.11.00'

log.info('This is (based on) vsc.install.shared_setup %s' % VERSION)

Expand Down Expand Up @@ -361,13 +368,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 = {}
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity, why is this modified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous strategy was starting with a dictionary, res, and removing items from it as it was being iterated through. This triggers errors with testing, and generally isn't a good idea! To fix, I decided to add keepers, and instead of removing from res, I am adding entries from res to keepers that are intended to return. We then return "keepers" instead of res.

Copy link
Member

@boegel boegel May 13, 2018

Choose a reason for hiding this comment

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

To clarify: this is required because in Python 3 .items() returns an iterator, so modifying the dict we're iterating over means trouble in that case. In Python 2, .items() return a list (a copy of the contents of the dict), so modifying the original dict while iterating over .items() is fine in that case.

Copy link
Member

Choose a reason for hiding this comment

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

thx!

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 +1056,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 +1210,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 fh:
for chunk in iter(lambda: fh.read(4096), b""):
hasher.update(chunk)
return hasher.hexdigest()


def get_license(self, license_name=None):
"""
Expand Down Expand Up @@ -1254,11 +1270,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
for name, klass in new_target['cmdclass'].items():
keepers = new_target['cmdclass'].copy()
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity, why is this modified

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same issue is here as for the previous keepers - you can't iterate through a dictionary and change it at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The copy() creates a new copy in memory. If we didn't do that, we would just be pointing to the same dictionary.

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: .items() is a list (copy) in Python 2, while in Python 3 it's an iterator

for name in new_target['cmdclass']:
klass = new_target['cmdclass'][name]
try:
new_target['cmdclass'][name] = getattr(vsc_setup_klass, klass.__name__)
keepers[name] = getattr(vsc_setup_klass, klass.__name__)
except AttributeError:
del new_target['cmdclass'][name]
del keepers[name]
log.info("Not including new_target['cmdclass']['%s']" % name)
new_target['cmdclass'] = keepers

# prepare classifiers
classifiers = new_target.setdefault('classifiers', [])
Expand Down Expand Up @@ -1385,7 +1405,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 +1430,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 Expand Up @@ -1481,7 +1501,6 @@ def action_target(self, target, setupfn=None, extra_sdist=None, urltemplate=None

self.prepare_rpm(target)
x = self.parse_target(target, urltemplate)

setupfn(**x)


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 # Python 2
except NameError:
basestring = (bytes, str) # Python 3

try:
from cStringIO import StringIO # Python 2
except ImportError:
from io import StringIO # Python 3

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 fh:
result = fh.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
16 changes: 9 additions & 7 deletions test/shared_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,8 @@ def test_rel_gitignore(self):
base_dir = os.path.join(os.path.dirname(os.path.realpath(__file__)), './testdata')
try:
self.setup.rel_gitignore(['testdata'], base_dir=base_dir)
except Exception as e:
self.assertTrue('.pyc' in e.message, msg=e)
except Exception as err:
self.assertTrue('.pyc' in str(err))
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,14 +127,13 @@ 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: %s' % str(args))
print('kwargs: %s' % kwargs)

self.mock_stdout(True)
action_target({'name': 'vsc-test', 'version': '1.0.0'}, setupfn=fake_setup)
txt = self.get_stdout()
self.mock_stdout(False)

self.assertTrue(re.search(r"^args:\s*\(\)", txt, re.M))
self.assertTrue(re.search(r"^kwargs:\s*\{.*'name':\s*'vsc-test'", txt, re.M))

Expand All @@ -150,6 +149,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 +161,8 @@ 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'])

self.assertEqual(sorted(setup.SHARED_TARGET['packages']), ['vsc', 'vsc.test'])
package = {
'name': 'vsc-test',
'excluded_pkgs_rpm': ['vsc', 'vsc.test'],
Expand All @@ -170,7 +171,8 @@ 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'])

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

def test_parse_target(self):
"""Test for parse target"""
Expand Down