Skip to content
85 changes: 56 additions & 29 deletions easybuild/framework/easyconfig/easyconfig.py
Original file line number Diff line number Diff line change
Expand Up @@ -1359,6 +1359,39 @@ def robot_find_minimal_toolchain_of_dependency(dep, modtool, parent_tc=None):
return minimal_toolchain


def _copy_file_to_easyconfigs_dir(path, target_dir, soft_name, target_file):
"""
Copy file at specified 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)
@aram target_file: target file name
@return: full path to copied file
"""
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)

full_target_path = os.path.join(target_dir, target_path)
try:
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)
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_easyconfigs(paths, target_dir):
"""
Copy easyconfig files to specified directory, in the 'right' location and using the filename expected by robot.
Expand All @@ -1373,42 +1406,36 @@ def copy_easyconfigs(paths, target_dir):
'new': [],
}

a_to_z = [chr(i) for i in range(ord('a'), ord('z') + 1)]
subdir = os.path.join('easybuild', 'easyconfigs')

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]))
for path in paths:
ecs = process_easyconfig(path, validate=False)
if len(ecs) == 1:
file_info['ecs'].append(ecs[0]['ec'])
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 = _copy_file_to_easyconfigs_dir(path, target_dir, soft_name, ec_filename)
full_target_path = os.path.join(target_dir, target_path)

target_path = os.path.join(subdir, letter, name, ec_filename)
_log.debug("Target path for %s: %s", path, target_path)
file_info['new'].append(not os.path.exists(full_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.

hmm, the exists check will always return True here, since it was just copied?

file_info['paths_in_repo'].append(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 = []
for patch_path, soft_name in patch_specs:
patched_files.append(_copy_file_to_easyconfigs_dir(patch_path, target_dir, soft_name, os.path.basename(patch_path)))
Copy link
Member

Choose a reason for hiding this comment

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

line too long?


return patched_files

class ActiveMNS(object):
"""Wrapper class for active module naming scheme."""
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
42 changes: 38 additions & 4 deletions easybuild/tools/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@
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
from easybuild.tools.build_log import EasyBuildError, print_msg
from easybuild.tools.config import build_option
from easybuild.tools.filetools import det_patched_files, download_file, extract_file, mkdir, read_file
from easybuild.tools.filetools import det_patched_files, download_file, extract_file, is_patch_file, mkdir, read_file
from easybuild.tools.filetools import 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 @@ -595,8 +595,38 @@ def _easyconfigs_pr_common(paths, start_branch=None, pr_branch=None, target_acco

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

# copy files to right place
file_info = copy_easyconfigs(paths, os.path.join(git_working_dir, pr_target_repo))
# seperate easyconfigs and patch files
ec_paths = [p for p in paths if p.endswith('.eb') or not is_patch_file(p)]
patch_paths = [p for p in 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 = []
print_msg("determining software names for patch files...")
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

# FIXME: try harder if software name wasn't found yet?
Copy link
Member

Choose a reason for hiding this comment

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

@Caylo hmm, ideally, this should be tackled too...

As it is, only the easyconfig files that are being tweaked are being considered when trying to figure out which software the patch belongs too, but this may not be enough in practice

For example:

eb --new-pr foo.eb
# oops, forgot patch
eb --update-pr 1234 foo.patch

So, I think we should fall back to scanning all easyconfigs, and check which patches they require... This may take a while, but it's the best we can do imho (we can't rely on the patch name).


if soft_name:
patch_specs.append((patch_path, soft_name))
else:
raise EasyBuildError("Failed to determine software name to which patch file %s relates", patch_path)
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 hoist this blob of code that determines patch_specs into a separate function, it'll help make things clearer since it's rather big (and will get bigger if the FIXME is handled)


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

# checkout target branch
if pr_branch is None:
Expand All @@ -612,6 +642,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))
git_repo.index.add(patch_info)

# overview of modifications
if build_option('extended_dry_run'):
print_msg("\nFull patch:\n", log=_log, prefix=False)
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 @@ -2250,20 +2250,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)

os.environ['EASYBUILD_GITHUB_USER'] = GITHUB_TEST_ACCOUNT
args = [
'--new-pr',
'--experimental',
toy_ec,
toy_patch,
'-D',
'--disable-cleanup-tmpdir',
]
Expand All @@ -2281,7 +2283,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 @@ -2318,7 +2321,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