Skip to content

enhance detection of patch files supplied to 'eb' command with better error messages #3709

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
smoors merged 3 commits into from
Jun 11, 2021
Merged

enhance detection of patch files supplied to 'eb' command with better error messages #3709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
916666a
Enhance detection of patch files with better error messages
Flamefire May 28, 2021
dd367c5
Adapt other test to use existing patch file
Flamefire May 28, 2021
ed51265
Rename testdir->test_dir
Flamefire May 28, 2021
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
8 changes: 8 additions & 0 deletions easybuild/framework/easyconfig/tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,14 @@ def categorize_files_by_type(paths):
# file must exist in order to check whether it's a patch file
elif os.path.isfile(path) and is_patch_file(path):
res['patch_files'].append(path)
elif path.endswith('.patch'):
Copy link
Contributor

@smoors smoors Jun 10, 2021

Choose a reason for hiding this comment

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

what with files that are neither easyconfig, patch, or .py file? eg. README.md files?

Copy link
Contributor Author

@Flamefire Flamefire Jun 11, 2021

Choose a reason for hiding this comment

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

They will fail to parse and error out as before. However those are uncommon enough to just do that.

Of course we could check for .eb extensions (and maybe easystack? what is their ext?) and error out here for anything else. But that would be a breaking change in case anyone (for whatever reason) uses .ec for their easyconfigs

Copy link
Member

@boegel boegel Jun 23, 2021

Choose a reason for hiding this comment

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

Easystack files currrently need to be passed as an argument to --easystack, so that outside this scope (at least currently, that may change).

Maybe we can start using .ebs or .es as extension for easystack files.

if not os.path.exists(path):
raise EasyBuildError('File %s does not exist, did you mistype the path?', path)
elif not os.path.isfile(path):
raise EasyBuildError('File %s is expected to be a regular file, but is a folder instead', path)
Copy link
Member

@boegel boegel Jun 23, 2021

Choose a reason for hiding this comment

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

This is a bit misleading: it's not because it's not a file, that it's a directory. It could be a broken symlink for example...

else:
raise EasyBuildError('%s is not detected as a valid patch file. Please verify its contents!',
path)
else:
# anything else is considered to be an easyconfig file
res['easyconfigs'].append(path)
Expand Down
22 changes: 20 additions & 2 deletions test/framework/easyconfig.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3267,10 +3267,11 @@ def test_categorize_files_by_type(self):
configuremake = os.path.join(easyblocks_dir, 'generic', 'configuremake.py')
toy_easyblock = os.path.join(easyblocks_dir, 't', 'toy.py')

gzip_ec = os.path.join(test_ecs_dir, 'test_ecs', 'g', 'gzip', 'gzip-1.4.eb')
paths = [
'bzip2-1.0.6.eb',
toy_easyblock,
os.path.join(test_ecs_dir, 'test_ecs', 'g', 'gzip', 'gzip-1.4.eb'),
gzip_ec,
toy_patch,
'foo',
':toy-0.0-deps.eb',
Expand All @@ -3279,14 +3280,31 @@ def test_categorize_files_by_type(self):
res = categorize_files_by_type(paths)
expected = [
'bzip2-1.0.6.eb',
os.path.join(test_ecs_dir, 'test_ecs', 'g', 'gzip', 'gzip-1.4.eb'),
gzip_ec,
'foo',
]
self.assertEqual(res['easyconfigs'], expected)
self.assertEqual(res['files_to_delete'], ['toy-0.0-deps.eb'])
self.assertEqual(res['patch_files'], [toy_patch])
self.assertEqual(res['py_files'], [toy_easyblock, configuremake])

# Error cases
tmpdir = tempfile.mkdtemp()
non_existing = os.path.join(tmpdir, 'does_not_exist.patch')
self.assertErrorRegex(EasyBuildError,
"File %s does not exist" % non_existing,
categorize_files_by_type, [non_existing])
patch_dir = os.path.join(tmpdir, 'folder.patch')
os.mkdir(patch_dir)
self.assertErrorRegex(EasyBuildError,
"File %s is expected to be a regular file" % patch_dir,
categorize_files_by_type, [patch_dir])
invalid_patch = os.path.join(tmpdir, 'invalid.patch')
copy_file(gzip_ec, invalid_patch)
self.assertErrorRegex(EasyBuildError,
"%s is not detected as a valid patch file" % invalid_patch,
categorize_files_by_type, [invalid_patch])

def test_resolve_template(self):
"""Test resolve_template function."""
self.assertEqual(resolve_template('', {}), '')
Expand Down
27 changes: 15 additions & 12 deletions test/framework/github.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,8 +565,8 @@ def test_find_easybuild_easyconfig(self):

def test_find_patches(self):
""" Test for find_software_name_for_patch """
testdir = os.path.dirname(os.path.abspath(__file__))
ec_path = os.path.join(testdir, 'easyconfigs')
test_dir = os.path.dirname(os.path.abspath(__file__))
ec_path = os.path.join(test_dir, 'easyconfigs')
init_config(build_options={
'allow_modules_tool_mismatch': True,
'minimal_toolchains': True,
Expand Down Expand Up @@ -916,28 +916,31 @@ def test_det_pr_target_repo(self):
# no files => return default target repo (None)
self.assertEqual(gh.det_pr_target_repo(categorize_files_by_type([])), None)

test_dir = os.path.dirname(os.path.abspath(__file__))

# easyconfigs/patches (incl. files to delete) => easyconfigs repo
# this is solely based on filenames, actual files are not opened
# this is solely based on filenames, actual files are not opened, except for the patch file which must exist
toy_patch_fn = 'toy-0.0_fix-silly-typo-in-printf-statement.patch'
toy_patch = os.path.join(test_dir, 'sandbox', 'sources', 'toy', toy_patch_fn)
test_cases = [
['toy.eb'],
['toy.patch'],
['toy.eb', 'toy.patch'],
[toy_patch],
['toy.eb', toy_patch],
[':toy.eb'], # deleting toy.eb
['one.eb', 'two.eb'],
['one.eb', 'two.eb', 'toy.patch', ':todelete.eb'],
['one.eb', 'two.eb', toy_patch, ':todelete.eb'],
]
for test_case in test_cases:
self.assertEqual(gh.det_pr_target_repo(categorize_files_by_type(test_case)), 'easybuild-easyconfigs')

# if only Python files are involved, result is easyblocks or framework repo;
# all Python files are easyblocks => easyblocks repo, otherwise => framework repo;
# files are opened and inspected here to discriminate between easyblocks & other Python files, so must exist!
testdir = os.path.dirname(os.path.abspath(__file__))
github_py = os.path.join(testdir, 'github.py')
github_py = os.path.join(test_dir, 'github.py')

configuremake = os.path.join(testdir, 'sandbox', 'easybuild', 'easyblocks', 'generic', 'configuremake.py')
configuremake = os.path.join(test_dir, 'sandbox', 'easybuild', 'easyblocks', 'generic', 'configuremake.py')
self.assertTrue(os.path.exists(configuremake))
toy_eb = os.path.join(testdir, 'sandbox', 'easybuild', 'easyblocks', 't', 'toy.py')
toy_eb = os.path.join(test_dir, 'sandbox', 'easybuild', 'easyblocks', 't', 'toy.py')
self.assertTrue(os.path.exists(toy_eb))

self.assertEqual(build_option('pr_target_repo'), None)
Expand All @@ -951,14 +954,14 @@ def test_det_pr_target_repo(self):
self.assertEqual(gh.det_pr_target_repo(categorize_files_by_type(py_files)), 'easybuild-framework')

# as soon as an easyconfig file or patch files is involved => result is easybuild-easyconfigs repo
for fn in ['toy.eb', 'toy.patch']:
for fn in ['toy.eb', toy_patch]:
self.assertEqual(gh.det_pr_target_repo(categorize_files_by_type(py_files + [fn])), 'easybuild-easyconfigs')

# if --pr-target-repo is specified, we always get this value (no guessing anymore)
init_config(build_options={'pr_target_repo': 'thisisjustatest'})

self.assertEqual(gh.det_pr_target_repo(categorize_files_by_type([])), 'thisisjustatest')
self.assertEqual(gh.det_pr_target_repo(categorize_files_by_type(['toy.eb', 'toy.patch'])), 'thisisjustatest')
self.assertEqual(gh.det_pr_target_repo(categorize_files_by_type(['toy.eb', toy_patch])), 'thisisjustatest')
self.assertEqual(gh.det_pr_target_repo(categorize_files_by_type(py_files)), 'thisisjustatest')
self.assertEqual(gh.det_pr_target_repo(categorize_files_by_type([configuremake])), 'thisisjustatest')
self.assertEqual(gh.det_pr_target_repo(categorize_files_by_type([toy_eb])), 'thisisjustatest')
Expand Down