Compiler with path new #435
Conversation
Compiler wrapper
Bash-style substitution is not currently handled
Previously they were passed into the Linker.link() function
Linker flags for common libraries
… new stle arguments and vice versa.
Co-authored-by: allynt <allyn.treshansky@gmail.com>
Co-authored-by: allynt <allyn.treshansky@gmail.com>
|
I still had way too many merging problems :( I hope I got them all sorted out correctly. A few comments and questions:
I have no idea what happened with 3. It looks a bit like a bad merge somewhere? I have had git getting confused quite a bit, but I didn't check any further. Removing library definitions is de-facto not necessary (if they are not needed they don't matter, if they are needed, you don't want to delete them - if at all, you replace them, which you can). So, I think that's KISS :) |
|
@yaswant , @MatthewHambley - I can't select the reviewer group, please assign someone :) |
There was a problem hiding this comment.
Why is this change modifying workflows?
| @@ -68,6 +69,16 @@ def check_available(self) -> bool: | |||
| return False | |||
| return True | |||
|
|
|||
| def set_full_path(self, full_path: str): | |||
There was a problem hiding this comment.
If we're dealing with paths we might use the Path class.
| @@ -40,6 +40,7 @@ def __init__(self, name: str, exec_name: Union[str, Path], | |||
| self._logger = logging.getLogger(__name__) | |||
| self._name = name | |||
| self._exec_name = str(exec_name) | |||
| self._full_path = "" | |||
There was a problem hiding this comment.
Why not combine _exec_name and _full_path into an _executable: Path which con hold a relative or absolute path. Then when the executable name is needed use _executable.name.
This would save having to do if self._full_path... choices later.
| def change_exec_name(self, exec_name: str): | ||
| '''Changes the name of the executable This function should in general | ||
| not be used (typically it is better to create a new tool instead). The | ||
| function is only provided to support CompilerWrapper (like mpif90), | ||
| which need all parameters from the original compiler, but call the | ||
| wrapper. The name of the compiler will be changed just before | ||
| compilation, and then set back to its original value | ||
| ''' | ||
| self._exec_name = exec_name | ||
|
|
There was a problem hiding this comment.
Happy to see the back of change_exec_name, never a fan of that.
| path_name = Path(name) | ||
| full_path = None | ||
| if path_name.is_absolute(): | ||
| # We have a full path: | ||
| name = path_name.name | ||
| full_path = str(path_name) |
There was a problem hiding this comment.
At least some of this feels like it should be in the tool itself, as noted above.
| assert len(recwarn) == 2 | ||
| user_warning = recwarn.pop(UserWarning) | ||
| assert ("_metric_send_conn not set, cannot send metrics" in | ||
| str(user_warning.message)) | ||
| dep_warning = recwarn.pop(DeprecationWarning) | ||
| assert ("RootIncFiles is deprecated as .inc files are due to be " | ||
| "removed." in str(dep_warning.message)) | ||
|
|
There was a problem hiding this comment.
I was not aware of recwarn but I think it is neater than the slightly clumsy with warns(... syntax.
In the case of these warnings where we are testing for the presence of almost the whole string and there is no variable component (such as a filename) in the string we should probably test for equality. i.e. assert user_warning.message == "...".
| @@ -22,7 +22,7 @@ class TestRootIncFiles: | |||
| """ | |||
| Tests include files are handled correctly. | |||
| """ | |||
| def test_vanilla(self, tmp_path: Path) -> None: | |||
| def test_vanilla(self, tmp_path: Path, recwarn) -> None: | |||
There was a problem hiding this comment.
Just checked the documentation and the type is recwarn: WarningsRecord.
| ''' | ||
| cc = Compiler("gcc", "gcc", "gnu", version_regex="some_regex", | ||
| category=Category.C_COMPILER, openmp_flag="-fopenmp") | ||
| assert cc._exec_name == "gcc" |
There was a problem hiding this comment.
Tests looking at "private" state is concerning. What's going on here?
| ['mpicc', '--version'] | ||
| ] | ||
|
|
||
|
|
||
| def test_version_consistency(stub_c_compiler: CCompiler, |
There was a problem hiding this comment.
I take it this test is no longer relevant. Are these the teste mentioned in the comment trail? If not a quick explanation of why they are removed would be handy.
| fake_process.register(['ifort', '-V'], stdout='ifort (IFORT) 1.2.3 foo') | ||
| fake_process.register(['icc', '--version'], stdout='icc (ICC) 1.2.3 foo') | ||
| fake_process.register(['ifort', '--version'], | ||
| stdout='ifort (IFORT) 1.2.3 foo') |
There was a problem hiding this comment.
It's getting increasingly hard to find information on the ifort/icc compilers but I did some checking and -V is the correct argument for getting what Intel call "the logo" which is the version information.
What was the reason for changing this?
This new branch tries to resolve all conflicts that #402 had (i.e. it's a merge of 402 with Matthew's changes).