Allow --force to use regex if --try-toolchain cannot map intelligently.#2605
Conversation
| raise EasyBuildError("Toolchain %s is not equivalent to toolchain %s in terms of capabilities. " | ||
| "Use --force if you are sure.", | ||
| target_toolchain['name'], source_toolchain['name']) | ||
| print_warning("Combining --try-toolchain with --force for toolchains with unequal capabilities: using regex.") |
There was a problem hiding this comment.
line too long (126 > 120 characters)
|
This just moves the exception two levels up in the call stack and adjusts for --force. Maybe there is a better way @ocaisa? |
ocaisa
left a comment
There was a problem hiding this comment.
LGTM just need to fix the tests that expect map_toolchain_hierarchies to throw an error if no mapping exists
| print_warning("Combining --try-toolchain with --force for toolchains with unequal capabilities: " | ||
| "using regex.") | ||
| revert_to_regex = True | ||
| modifying_toolchains = False |
There was a problem hiding this comment.
@bartoldeman Please refactor this a bit to i) use positive logic, ii) use if/else (it's a bit confusing now imho).
I would also clarify the "regex" part a bit in the warning, and also the error message(see below).
if build_option('force'):
print_warning("Combining --try-toolchain with --force for toolchains with unequal capabilities: "
"disabling recursion & not changing (sub)toolchains.")
revert_to_regex = True
modifying_toolchains = False
else:
raise EasyBuildError("Toolchain %s is not equivalent to toolchain %s in terms of capabilities, aborting "
"(if you know what you are doing, you can use --force to proceed anyway)",
target_toolchain['name'], source_toolchain['name'])There was a problem hiding this comment.
Of course this should go below the if src_to_dst_tc_mapping is None: :)
| minimal_matching_toolchain = match_minimum_tc_specs(toolchain_spec, target_tc_hierarchy) | ||
| if not minimal_matching_toolchain: | ||
| return None | ||
| tc_mapping[toolchain_spec['name']] = minimal_matching_toolchain |
There was a problem hiding this comment.
Same here, please use positive logic + if/else, and maybe also add a debug log message:
if minimal_matching_toolchain:
tc_mapping[toolchain_spec['name']] = minimal_matching_toolchain
else:
_log.debug("No minimal matching toolchain found for %s using %s", toolchain_spec, target_tc_hierarchy)
return NoneI'm also not a big fan of the inline return statement. Can we avoid this?
|
|
||
| # Warn if we are changing compiler families, this is very likely to cause problems | ||
| if target_compiler_family != source_tc_spec['comp_family']: | ||
| if minimal_matching_toolchain and target_compiler_family != source_tc_spec['comp_family']: |
There was a problem hiding this comment.
@bartoldeman Wouldn't this make more sense?
if target_compiler_family and target_compiler_family != source_tc_spec['comp_family']:|
Why is the test failing anyway? The error should still be thrown unless It seems to signal that the default behavior is being changed? |
|
The failing tests are for |
|
I simplified the change by simply catching the deeper down exception into a new higher-level exception. This way I no longer need to distinguish between {} and None which is always a bit icky IMHO |
| try: | ||
| src_to_dst_tc_mapping = map_toolchain_hierarchies(source_toolchain, target_toolchain, modtool) | ||
| except EasyBuildError as err: | ||
| if build_option('force'): |
There was a problem hiding this comment.
@bartoldeman We should probably check whether the raised exception is indeed the one raised by match_minimum_tc_specs, since (at least in theory) an EasyBuildError could be raised for a variety of reasons....
That could look something like this (untested):
except EasyBuildError as err:
# make sure exception was raised by match_minimum_tc_specs because toolchain mapping could not be done
if "No possible mapping from source toolchain" in err.msg:
if build_option('force'):
warning_msg = "Combining --try-toolchain with --force for toolchains with unequal capabilities: "
warning_msg += "disabling recursion and not changing (sub)toolchains for dependencies."
print_warning(warning_msg)
revert_to_regex = True
modifying_toolchains = False
else:
error_msg = err.msg + '\n'
error_msg += "Toolchain %s is not equivalent to toolchain %s in terms of capabilities. "
error_msg += "(if you know what you are doing, you can use --force to proceed anyway)"
raise EasyBuildError(error_msg, target_toolchain['name'], source_toolchain['name'])
else:
# simply re-raise the exception if something else went wrong
raise err|
Reopening for @boegelbot and his master @boegel to see if this was a Travis glitch with Ubuntu updates... |
Fixes #2592