-
Notifications
You must be signed in to change notification settings - Fork 202
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.
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
enhance detection of patch files supplied to 'eb' command with better error messages #3709
enhance detection of patch files supplied to 'eb' command with better error messages #3709
Conversation
Instead of assuming that everything that doesn't look like a patch is an EC, assume *.patch files are patches and error out with a clear message why it was not detected as such. Avoids confusing errors such as: ERROR: Failed to process easyconfig foo.patch: Parsing easyconfig file failed: invalid syntax (<string>, line 1)
9fe5e7b
to
ed51265
Compare
@@ -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'): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Going in, thanks @Flamefire! |
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) |
There was a problem hiding this comment.
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...
Instead of assuming that everything that doesn't look like a patch is an EC, assume *.patch files are patches and error out with a clear message why it was not detected as such.
Avoids confusing errors such as: