Add compiler selection arguments #476
Conversation
Add some standard flags for compiler selection, with environment variables as potential fallbacks. Adds tests for the new options and add tests for two previously untested error conditions to bring test coverage up to 100 percent.
hiker
left a comment
hiker
left a comment
There was a problem hiding this comment.
Thanks for adding these options. I have a few mostly styling related questions added as comments.
I also would say that this PR is actually too small (unusual for me to say something like this :) ). I can't easily see how this is supposed to be used at all (e.g. what is going to happen with the command line option for C++ compiler). It would be easier to understand if there would be an application using it.
In general, imho this appears to be a total duplication of PR #414, which already has these options integrated (and missing applications of your new class makes it impossible to see if there is anything you are planning to do that cannot be done with the existing class). If there is an advantage of your argument class, it should be integrated into the new base class. I would be interested in a comment on this integration (which would ensure that all CLI tools behave as identical as possible, which is good for the user :) )
But as you indicated, we can discuss this in the next telco.
| """Fab command argument parser.""" | ||
|
|
||
| # Fallback compiler family names | ||
| _default_cc = "gcc" |
There was a problem hiding this comment.
This seems wrong. What if gcc (etc) is not available on the system? Default should at least be a compiler that works.
There was a problem hiding this comment.
Agreed, I don't think a "default compiler" is a useful concept. If a compiler can't be determined then that's an error the user needs to remedy.
| help="name of the C compiler (default: %(default)s)", | ||
| ) | ||
|
|
||
| if "--cxx" not in self._option_string_actions: |
There was a problem hiding this comment.
To the best of my knowledge Fab doesn't support C++. Why add a command line for C++?
There was a problem hiding this comment.
It certainly needs to. C++ is an increasingly popular choice for scientific software.
|
|
||
| if "--version" not in self._option_string_actions: | ||
| group.add_argument( | ||
| "--version", action="version", version=f"%(prog)s {self.version}" |
There was a problem hiding this comment.
Line is too long (assuming we want to follow PEP8 coding style, which is 79 characters). There are other lines that are long (though this is not your fault in this PR). Though I think we agreed to gradually update our files (you touch it, you fix it :) ):
./arguments.py:71:80: E501 line too long (85 > 79 characters)
./arguments.py:99:80: E501 line too long (84 > 79 characters)
./arguments.py:122:80: E501 line too long (87 > 79 characters)
./arguments.py:184:80: E501 line too long (81 > 79 characters)
./arguments.py:302:80: E501 line too long (80 > 79 characters)
There was a problem hiding this comment.
This file should be in tests/unit_tests/cui/test_arguments.py (to mirror the original location).
| parser.parse_args(["--version"]) | ||
| assert exc.value.code == 0 | ||
|
|
||
| captured = capsys.readouterr() |
There was a problem hiding this comment.
Not your fault, but test_version does not have a doc string. Could you please fix this?
| Mock(option_strings=["--test"]), "error testing" | ||
| ) | ||
|
|
||
| monkeypatch.setattr(argparse.ArgumentParser, "parse_known_args", replacement) |
There was a problem hiding this comment.
This line is too long (again assuming standard line length, fwiw, the coding styles leave this unspecified). Other lines:
./test_cui_arguments.py:137:80: E501 line too long (85 > 79 characters)
./test_cui_arguments.py:227:80: E501 line too long (82 > 79 characters)
./test_cui_arguments.py:233:80: E501 line too long (84 > 79 characters)
./test_cui_arguments.py:236:80: E501 line too long (82 > 79 characters)
./test_cui_arguments.py:358:80: E501 line too long (86 > 79 characters)
./test_cui_arguments.py:359:80: E501 line too long (88 > 79 characters)
./test_cui_arguments.py:360:80: E501 line too long (81 > 79 characters)
./test_cui_arguments.py:361:80: E501 line too long (83 > 79 characters)
./test_cui_arguments.py:362:80: E501 line too long (85 > 79 characters)
| import pytest | ||
| from typing import Optional | ||
| from pyfakefs.fake_filesystem import FakeFilesystem | ||
| from unittest.mock import Mock |
There was a problem hiding this comment.
Importing unittest and typing must come before importing from fab (and imho the same for pyfakefs, i.e. fab should come last). Also, ideally the imports should be sorted alphabetically, to make it easier to detect if there are two imports from the same module.
MatthewHambley
left a comment
MatthewHambley
left a comment
There was a problem hiding this comment.
The primary issue is the default compilers.
| """Fab command argument parser.""" | ||
|
|
||
| # Fallback compiler family names | ||
| _default_cc = "gcc" |
There was a problem hiding this comment.
Agreed, I don't think a "default compiler" is a useful concept. If a compiler can't be determined then that's an error the user needs to remedy.
| help="name of the C compiler (default: %(default)s)", | ||
| ) | ||
|
|
||
| if "--cxx" not in self._option_string_actions: |
There was a problem hiding this comment.
It certainly needs to. C++ is an increasingly popular choice for scientific software.
3 participants
Add some standard flags for compiler selection, with environment variables as potential fallbacks.
Adds tests for the new options and add tests for two previously untested error conditions to bring test coverage up to 100 percent.