Skip to content
103 changes: 72 additions & 31 deletions easybuild/framework/easyconfig/easyconfig.py
Original file line number Diff line number Diff line change
Expand Up @@ -1365,55 +1365,96 @@ def robot_find_minimal_toolchain_of_dependency(dep, modtool, parent_tc=None):
return minimal_toolchain


def det_location_for(path, target_dir, soft_name, target_file):
"""
Determine path to easyconfigs directory for specified software name, using specified target file name.

:param path: path of file to copy
:param target_dir: (parent) target directory, should contain easybuild/easyconfigs subdirectory
:param soft_name: software name (to determine location to copy to)
:param target_file: target file name
:return: full path to the right location
"""
subdir = os.path.join('easybuild', 'easyconfigs')

if os.path.exists(os.path.join(target_dir, subdir)):
letter = soft_name.lower()[0]
if letter not in [chr(i) for i in range(ord('a'), ord('z') + 1)]:
raise EasyBuildError("Don't know which letter subdir to use for software name %s", soft_name)

target_path = os.path.join('easybuild', 'easyconfigs', letter, soft_name, target_file)
_log.debug("Target path for %s: %s", path, target_path)

target_path = os.path.join(target_dir, target_path)

else:
raise EasyBuildError("Subdirectory %s not found in %s", subdir, target_dir)

return target_path
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 return full_target_path?



def copy_file(path, target_dir, soft_name, target_name):
Copy link
Member

Choose a reason for hiding this comment

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

@Caylo I'd move this to easybuild.tools.filetools, it's a useful function in general


target_path = det_location_for(path, target_dir, soft_name, target_name)
new = os.path.exists(target_path)
Copy link
Member

Choose a reason for hiding this comment

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

move this out of this function, this should b a generic function to copy files (also: add a docstring)


try:
mkdir(os.path.dirname(target_path), parents=True)
shutil.copy2(path, target_path)
_log.info("%s copied to %s", path, target_path)
except OSError as err:
raise EasyBuildError("Failed to copy %s to %s: %s", path, target_path, err)

return target_path, new
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should return anything, you should pass is both the source and target path (and the new thing is something very specific, not relevant in general)



def copy_easyconfigs(paths, target_dir):
"""
Copy easyconfig files to specified directory, in the 'right' location and using the filename expected by robot.

@paths: list of paths to copy to git working dir
@target_dir: target directory
@return: dict with useful information on copied easyconfig files (corresponding EasyConfig instances, paths, status)
:param paths: list of paths to copy to git working dir
:param target_dir: target directory
:return: dict with useful information on copied easyconfig files (corresponding EasyConfig instances, paths, status)
"""
file_info = {
'ecs': [],
'paths_in_repo': [],
'new': [],
}

a_to_z = [chr(i) for i in range(ord('a'), ord('z') + 1)]
subdir = os.path.join('easybuild', 'easyconfigs')
for path in paths:
ecs = process_easyconfig(path, validate=False)
if len(ecs) == 1:
file_info['ecs'].append(ecs[0]['ec'])

if os.path.exists(os.path.join(target_dir, subdir)):
for path in paths:
ecs = process_easyconfig(path, validate=False)
if len(ecs) == 1:
file_info['ecs'].append(ecs[0]['ec'])
name = file_info['ecs'][-1].name
ec_filename = '%s-%s.eb' % (name, det_full_ec_version(file_info['ecs'][-1]))
soft_name = file_info['ecs'][-1].name
ec_filename = '%s-%s.eb' % (soft_name, det_full_ec_version(file_info['ecs'][-1]))

letter = name.lower()[0]
if letter not in a_to_z:
raise EasyBuildError("Don't know which letter subdir to use for %s", name)
target_path, new = copy_file(path, target_dir, soft_name, ec_filename)
file_info['paths_in_repo'].append(target_path)
file_info['new'].append(new)
Copy link
Member

Choose a reason for hiding this comment

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

no need for a new variable, it's only used in one place so let's do file_info['new'].append(os.path.exists(target_path))


target_path = os.path.join(subdir, letter, name, ec_filename)
_log.debug("Target path for %s: %s", path, target_path)
else:
raise EasyBuildError("Multiple EasyConfig instances obtained from easyconfig file %s", path)

full_target_path = os.path.join(target_dir, target_path)
try:
file_info['new'].append(not os.path.exists(full_target_path))
return file_info

mkdir(os.path.dirname(full_target_path), parents=True)
shutil.copy2(path, full_target_path)
_log.info("%s copied to %s", path, full_target_path)
except OSError as err:
raise EasyBuildError("Failed to copy %s to %s: %s", path, target_path, err)

file_info['paths_in_repo'].append(target_path)
else:
raise EasyBuildError("Multiple EasyConfig instances obtained from easyconfig file %s", path)
else:
raise EasyBuildError("Subdirectory %s not found in %s", subdir, target_dir)
def copy_patch_files(patch_specs, target_dir):
"""
Copy patch files to specified directory, in the 'right' location according to the software name they relate to.

return file_info
:param patch_specs: list of tuples with patch file location and name of software they are for
:param target_dir: target directory
"""
patched_files = {
'paths_in_repo': [],
}
for patch_path, soft_name in patch_specs:
target_path, new = copy_file(patch_path, target_dir, soft_name, os.path.basename(patch_path))
patched_files['paths_in_repo'].append(target_path)

return patched_files


class ActiveMNS(object):
Expand Down
6 changes: 6 additions & 0 deletions easybuild/tools/filetools.py
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,12 @@ def extract_cmd(filepath, overwrite=False):
return cmd_tmpl % {'filepath': filepath, 'target': target}


def is_patch_file(path):
"""Determine whether file at specified path is a patch file (based on +++ and --- lines being present)."""
txt = read_file(path)
return bool(re.search(r'^\+{3}\s', txt, re.M) and re.search(r'^-{3}\s', txt, re.M))


def det_patched_files(path=None, txt=None, omit_ab_prefix=False, github=False, filter_deleted=False):
"""
Determine list of patched files from a patch.
Expand Down
108 changes: 98 additions & 10 deletions easybuild/tools/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,11 @@
from vsc.utils import fancylogger
from vsc.utils.missing import nub

from easybuild.framework.easyconfig.easyconfig import copy_easyconfigs
from easybuild.framework.easyconfig.easyconfig import copy_easyconfigs, copy_patch_files, process_easyconfig
from easybuild.tools.build_log import EasyBuildError, print_msg
from easybuild.tools.config import build_option
from easybuild.tools.filetools import apply_patch, det_patched_files, download_file, extract_file
from easybuild.tools.filetools import mkdir, read_file, which, write_file
from easybuild.tools.filetools import is_patch_file, mkdir, read_file, which, write_file
from easybuild.tools.systemtools import UNKNOWN, get_tool_version
from easybuild.tools.utilities import only_if_module_is_available

Expand Down Expand Up @@ -627,6 +627,22 @@ def _easyconfigs_pr_common(paths, start_branch=None, pr_branch=None, target_acco

_log.debug("git status: %s", git_repo.git.status())

# seperate easyconfigs and patch files
ec_paths = [p for p in existing_paths if p.endswith('.eb') or p.endswith('.yeb') or not is_patch_file(p)]
Copy link
Member

Choose a reason for hiding this comment

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

the two endswith checks are useless if you also check not is_patch_file, since non-easyconfig files will fall through on the last check anyway

so, just use not is_patch_file(p)) (and assume only easyconfigs are passed in)

patch_paths = [p for p in existing_paths if p not in ec_paths]

# copy easyconfig files to right place
target_dir = os.path.join(git_working_dir, pr_target_repo)
print_msg("copying easyconfigs to %s..." % target_dir)
file_info = copy_easyconfigs(ec_paths, target_dir)
Copy link
Member

Choose a reason for hiding this comment

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

add guard if ec_paths: ?

Copy link
Member

Choose a reason for hiding this comment

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

nvm


# figure out to which software name patches relate, and copy them to the right place
if patch_paths:
patch_specs = det_patch_specs(patch_paths, file_info)

print_msg("copying patch files to %s..." % target_dir)
patch_info = copy_patch_files(patch_specs, target_dir)

# determine path to files to delete (if any)
deleted_paths = []
for fn in delete_files:
Expand All @@ -642,16 +658,16 @@ def _easyconfigs_pr_common(paths, start_branch=None, pr_branch=None, target_acco
raise EasyBuildError("Path doesn't exist or file to delete isn't found in target branch: %s", fn)

# copy edited/added files to right place
file_info = copy_easyconfigs(existing_paths, repo_path)

if existing_paths:
name_version = file_info['ecs'][0].name + string.translate(file_info['ecs'][0].version, None, '-.')
else:
name_version = os.path.basename(delete_files[0])
file_info = copy_easyconfigs(ec_paths, repo_path)

# checkout target branch
if pr_branch is None:
pr_branch = '%s_new_pr_%s' % (time.strftime("%Y%m%d%H%M%S"), name_version)
if ec_paths:
label = file_info['ecs'][0].name + string.translate(file_info['ecs'][0].version, None, '-.')
else:
label = ''.join(random.choice(string.letters) for _ in range(10))
pr_branch = '%s_new_pr_%s' % (time.strftime("%Y%m%d%H%M%S"), label)


# create branch to commit to and push;
# use force to avoid errors if branch already exists (OK since this is a local temporary copy of the repo)
Expand All @@ -662,6 +678,10 @@ def _easyconfigs_pr_common(paths, start_branch=None, pr_branch=None, target_acco
_log.debug("Staging all %d new/modified easyconfigs", len(file_info['paths_in_repo']))
git_repo.index.add(file_info['paths_in_repo'])

if patch_paths:
_log.debug("Staging all %d new/modified patch files", len(patch_info['paths_in_repo']))
git_repo.index.add(patch_info['paths_in_repo'])

# stage deleted files
if deleted_paths:
git_repo.index.remove(deleted_paths)
Expand Down Expand Up @@ -716,6 +736,74 @@ def _easyconfigs_pr_common(paths, start_branch=None, pr_branch=None, target_acco
return file_info, deleted_paths, git_repo, pr_branch, diff_stat


def det_patch_specs(patch_paths, file_info):
""" Determine software names for patch files """
print_msg("determining software names for patch files...")
patch_specs = []
for patch_path in patch_paths:
soft_name = None
patch_file = os.path.basename(patch_path)

# consider patch lists of easyconfigs being provided
for ec in file_info['ecs']:
if patch_file in ec['patches']:
soft_name = ec.name
break

if soft_name:
patch_specs.append((patch_path, soft_name))
else:
# fall back on scanning all eb files for patches
print "Matching easyconfig for %s not found on the first try:" % patch_path,
print "scanning all easyconfigs to determine where patch file belongs (this may take a while)..."
soft_name = scan_all_easyconfigs(patch_file)
if soft_name:
patch_specs.append((patch_path, soft_name))
else:
# still nothing found
raise EasyBuildError("Failed to determine software name to which patch file %s relates", patch_path)

return patch_specs


def scan_all_easyconfigs(patch_name):
Copy link
Member

Choose a reason for hiding this comment

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

please add a test for this new function

Copy link
Member

Choose a reason for hiding this comment

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

rename to find_software_name_for_patch?

"""
Scan all easyconfigs in the robot path to determine which software a patch file belongs to

@param patch_name: name of the patch file
Copy link
Member

Choose a reason for hiding this comment

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

:param, and also cover :return:

"""
robot_paths = build_option('robot_path')
soft_name = ""
Copy link
Member

Choose a reason for hiding this comment

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

use None as default

found = False
for robot_path in robot_paths:
all_ecs = []
Copy link
Member

Choose a reason for hiding this comment

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

move this out of this loop, and construct a full set of easyconfigs first before iterating over them?

all_ecs = []
for robot_path in robot_paths:
    ...

for idx, filename in enumerate(all_ecs):
    ...

for (dirpath, _, filenames) in os.walk(robot_path):
all_ecs.extend([os.path.join(dirpath, fn) for fn in filenames])

for idx, filename in enumerate(all_ecs):
if found:
break
if os.path.splitext(filename)[-1] in ['.eb', '.yeb']:
Copy link
Member

Choose a reason for hiding this comment

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

use the EB_FORMAT_EXTENSION and YEB_FORMAT_EXTENSION constants

Copy link
Member

Choose a reason for hiding this comment

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

and maybe also filter on the presence of patches already? that'll make this function a bit easier to follow

it does mean reading all files which contain 'patches' twice, but thanks to filesystem caching you'll probably not notice

path = os.path.join(dirpath, filename)
rawtxt = read_file(path)
if "patches" in rawtxt and filename != "TEMPLATE.eb":
Copy link
Member

Choose a reason for hiding this comment

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

filter out TEMPLATE.eb when you build the all_ecs list

try:
ecs = process_easyconfig(path, validate=False)
for ec in ecs:
ec['ec']._generate_template_values()
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 needed?

if patch_name in ec['ec'].asdict()['patches']:
Copy link
Member

Choose a reason for hiding this comment

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

just do ec['ec']['patches']?

soft_name = ec['ec'].asdict()['name']
found = True
Copy link
Member

Choose a reason for hiding this comment

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

add a break here, we're done searching

except EasyBuildError as err:
_log.debug("Ignoring easyconfig %s that fails to parse: %s", path, err)
pass
Copy link
Member

Choose a reason for hiding this comment

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

no need for the pass since you have the log.debug

sys.stdout.write('\r%s of %s easyconfigs checked' % (idx+1, len(all_ecs)))
Copy link
Member

Choose a reason for hiding this comment

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

just do len(all_ecs) once outside of the for loop, the length won't change

sys.stdout.flush()

print ''
Copy link
Member

Choose a reason for hiding this comment

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

use sys.stdout.write('\n') here for consistency

return soft_name


@only_if_module_is_available('git', pkgname='GitPython')
def new_pr(paths, title=None, descr=None, commit_msg=None):
"""Open new pull request using specified files."""
Expand Down Expand Up @@ -752,6 +840,7 @@ def new_pr(paths, title=None, descr=None, commit_msg=None):
classes_counted = sorted([(classes.count(c), c) for c in nub(classes)])
class_label = ','.join([tc for (cnt, tc) in classes_counted if cnt == classes_counted[-1][0]])


if title is None:
if file_info['ecs'] and all(file_info['new']) and not deleted_paths:
# mention software name/version in PR title (only first 3)
Expand All @@ -762,7 +851,6 @@ def new_pr(paths, title=None, descr=None, commit_msg=None):
main_title = ', '.join(names_and_versions[:3] + ['...'])

title = "{%s}[%s] %s" % (class_label, toolchain_label, main_title)

else:
raise EasyBuildError("Don't know how to make a PR title for this PR. "
"Please include a title (use --pr-title)")
Expand Down
6 changes: 6 additions & 0 deletions test/framework/filetools.py
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,12 @@ def test_find_flexlm_license(self):
del os.environ['LM_LICENSE_FILE']
self.assertEqual(ft.find_flexlm_license(lic_specs=[None]), ([], None))

def test_is_patch_file(self):
"""Test for is_patch_file() function."""
testdir = os.path.dirname(os.path.abspath(__file__))
self.assertFalse(ft.is_patch_file(os.path.join(testdir, 'easyconfigs', 'toy-0.0.eb')))
self.assertTrue(ft.is_patch_file(os.path.join(testdir, 'sandbox', 'sources', 'toy', 'toy-0.0_typo.patch')))

Copy link
Member

Choose a reason for hiding this comment

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

@Caylo: please add a test for copy_file here too

def test_is_alt_pypi_url(self):
"""Test is_alt_pypi_url() function."""
url = 'https://pypi.python.org/packages/source/e/easybuild/easybuild-2.7.0.tar.gz'
Expand Down
14 changes: 9 additions & 5 deletions test/framework/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -2265,20 +2265,22 @@ def test_fixed_installdir_naming_scheme(self):
def test_new_update_pr(self):
"""Test use of --new-pr (dry run only)."""
if self.github_token is None:
print "Skipping test_new_pr, no GitHub token available?"
print "Skipping test_new_update_pr, no GitHub token available?"
return

# copy toy test easyconfig
test_ecs_dir = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'easyconfigs')
testdir = os.path.dirname(os.path.abspath(__file__))
toy_ec = os.path.join(self.test_prefix, 'toy.eb')
toy_patch = os.path.join(testdir, 'sandbox', 'sources', 'toy', 'toy-0.0_typo.patch')
# purposely picked one with non-default toolchain/versionsuffix
shutil.copy2(os.path.join(test_ecs_dir, 'toy-0.0-gompi-1.3.12-test.eb'), toy_ec)
shutil.copy2(os.path.join(testdir, 'easyconfigs', 'toy-0.0-gompi-1.3.12-test.eb'), toy_ec)

args = [
'--new-pr',
'--experimental',
'--github-user=%s' % GITHUB_TEST_ACCOUNT,
toy_ec,
toy_patch,
'-D',
'--disable-cleanup-tmpdir',
]
Expand All @@ -2296,7 +2298,8 @@ def test_new_update_pr(self):
r"\(created using `eb --new-pr`\)", # description
r"^\* overview of changes:",
r".*/toy-0.0-gompi-1.3.12-test.eb\s+\|\s+[0-9]+\s+\++",
r"^\s*1 file changed",
r".*/toy-0.0_typo.patch\s+\|\s+[0-9]+\s+\++",
r"^\s*2 files changed",
]
for regex in regexs:
regex = re.compile(regex, re.M)
Expand Down Expand Up @@ -2333,7 +2336,8 @@ def test_new_update_pr(self):
r"^\* title: \"test-1-2-3\"",
r"^\* overview of changes:",
r".*/toy-0.0-gompi-1.3.12-test.eb\s+\|\s+[0-9]+\s+\++",
r"^\s*1 file changed",
r".*/toy-0.0_typo.patch\s+\|\s+[0-9]+\s+\++",
r"^\s*2 files changed",
]
for regex in regexs:
regex = re.compile(regex, re.M)
Expand Down