Skip to content

Add compiler selection arguments #476

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

Open
t00sa wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add compiler selection arguments #476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 36 additions & 1 deletion source/fab/cui/arguments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ def inner(self, *args, **kwargs):
self._setup_needed = False
self._add_location_group()
self._add_output_group()
self._add_compiler_group()
self._add_info_group()

result = func(self, *args, **kwargs)
Expand Down Expand Up @@ -87,6 +88,11 @@ def inner(self, *args, **kwargs):
class FabArgumentParser(argparse.ArgumentParser):
"""Fab command argument parser."""

# Fallback compiler family names
_default_cc = "gcc"
Copy link
Collaborator

@hiker hiker Aug 18, 2025

Choose a reason for hiding this comment

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

This seems wrong. What if gcc (etc) is not available on the system? Default should at least be a compiler that works.

Copy link
Collaborator

@MatthewHambley MatthewHambley Aug 22, 2025

Choose a reason for hiding this comment

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

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.

_default_cxx = "g++"
_default_fc = "gfortran"

def __init__(self, *args, **kwargs):

self.version = kwargs.pop("version", str(fab_version))
Expand Down Expand Up @@ -171,13 +177,42 @@ def _add_info_group(self):
"""Add informative options."""

# Create an info group
group = self.add_argument_group("info arguments")
group = self.add_argument_group("fab info arguments")

if "--version" not in self._option_string_actions:
group.add_argument(
"--version", action="version", version=f"%(prog)s {self.version}"
Copy link
Collaborator

@hiker hiker Aug 18, 2025

Choose a reason for hiding this comment

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

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)

)

def _add_compiler_group(self):
"""Add compiler options."""

group = self.add_argument_group("fab compiler arguments")

if "--cc" not in self._option_string_actions:
group.add_argument(
"--cc",
type=str,
default=os.environ.get("CC", self._default_cc),
help="name of the C compiler (default: %(default)s)",
)

if "--cxx" not in self._option_string_actions:
Copy link
Collaborator

@hiker hiker Aug 18, 2025

Choose a reason for hiding this comment

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

To the best of my knowledge Fab doesn't support C++. Why add a command line for C++?

Copy link
Collaborator

@MatthewHambley MatthewHambley Aug 22, 2025

Choose a reason for hiding this comment

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

It certainly needs to. C++ is an increasingly popular choice for scientific software.

group.add_argument(
"--cxx",
type=str,
default=os.environ.get("CXX", self._default_cxx),
help="name of the C++ compiler (default: %(default)s)",
)

if "--fc" not in self._option_string_actions:
group.add_argument(
"--fc",
type=str,
default=os.environ.get("FC", self._default_fc),
help="name of the Fortran compiler (default: %(default)s)",
)

def _configure_logging(self, namespace: argparse.Namespace) -> None:
"""Configure output logging.

Expand Down
105 changes: 104 additions & 1 deletion tests/unit_tests/test_cui_arguments.py
View file
Open in desktop
Copy link
Collaborator

@hiker hiker Aug 18, 2025

Choose a reason for hiding this comment

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

This file should be in tests/unit_tests/cui/test_arguments.py (to mirror the original location).

Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import pytest
from typing import Optional
from pyfakefs.fake_filesystem import FakeFilesystem
from unittest.mock import Mock
Copy link
Collaborator

@hiker hiker Aug 18, 2025

Choose a reason for hiding this comment

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

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.



class TestFullPathType:
Expand Down Expand Up @@ -123,6 +124,23 @@ def test_no_help(self):
assert isinstance(args, argparse.Namespace)
assert args.file is None

def test_error_handling(self, monkeypatch):
"""Check the ArgumentError exception handling."""

parser = FabArgumentParser()

def replacement(*args, **kwargs):
raise argparse.ArgumentError(
Mock(option_strings=["--test"]), "error testing"
)

monkeypatch.setattr(argparse.ArgumentParser, "parse_known_args", replacement)
Copy link
Collaborator

@hiker hiker Aug 18, 2025

Choose a reason for hiding this comment

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

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)


args = parser.parse_fabfile_only([])
assert isinstance(args, argparse.Namespace)
assert args.file is None
assert args.zero_config


class TestParser:
"""Test the core parser and its default options."""
Expand Down Expand Up @@ -214,7 +232,9 @@ def test_outupt_with_quiet(self, argv, capsys):
pytest.param(
[], None, Path("fab-workspace").expanduser().resolve(), id="default"
),
pytest.param([], Path("/tmp/fab"), Path("/tmp/fab").resolve(), id="environment"),
pytest.param(
[], Path("/tmp/fab"), Path("/tmp/fab").resolve(), id="environment"
),
pytest.param(
["--workspace", "/run/fab"],
Path("/tmp/fab"),
Expand Down Expand Up @@ -296,3 +316,86 @@ def test_version(self, capsys, monkeypatch):
captured = capsys.readouterr()
Copy link
Collaborator

@hiker hiker Aug 18, 2025

Choose a reason for hiding this comment

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

Not your fault, but test_version does not have a doc string. Could you please fix this?


assert captured.out.startswith("fab ")

def test_error_handling(self, monkeypatch):
"""Check the ArgumentError exception handling."""

parser = FabArgumentParser()

def replacement(*args, **kwargs):
return None, None

monkeypatch.setattr(argparse.ArgumentParser, "parse_args", replacement)

with pytest.raises(ValueError) as exc:
parser.parse_args(["--version"])
assert "invalid return value from wrapped function" in str(exc.value)


class TestCompilerFlags:
"""Test the compiler target flags."""

def test_defaults(self, monkeypatch):
"""Test class default settings."""

if "CC" in os.environ:
monkeypatch.delenv("CC")
if "CXX" in os.environ:
monkeypatch.delenv("CXX")
if "FC" in os.environ:
monkeypatch.delenv("FC")

parser = FabArgumentParser()
args = parser.parse_args([])

assert args.cc == "gcc"
assert args.cxx == "g++"
assert args.fc == "gfortran"

@pytest.mark.parametrize(
"argv,env,cc,cxx,fc",
[
pytest.param(["--cc", "icc"], {}, "icc", "g++", "gfortran", id="cc flag"),
pytest.param(["--cxx", "icx"], {}, "gcc", "icx", "gfortran", id="cxx flag"),
pytest.param(["--fc", "ifx"], {}, "gcc", "g++", "ifx", id="fc flag"),
pytest.param([], {"CC": "icc"}, "icc", "g++", "gfortran", id="CC env"),
pytest.param([], {"CXX": "icx"}, "gcc", "icx", "gfortran", id="CXX env"),
pytest.param([], {"FC": "ifx"}, "gcc", "g++", "ifx", id="FC env"),
pytest.param(
["--cc", "cc"],
{"CC": "icc"},
"cc",
"g++",
"gfortran",
id="cc flag+env",
),
pytest.param(
["--cxx", "CC"],
{"CXX": "icc"},
"gcc",
"CC",
"gfortran",
id="cxx flag+env",
),
pytest.param(
["--fc", "ftn"],
{"FC": "ifx"},
"gcc",
"g++",
"ftn",
id="FC flag+env",
),
],
)
def test_from_env(self, argv, env, cc, cxx, fc, monkeypatch):
"""Check compiler settings and environment overrides."""

for key, value in env.items():
monkeypatch.setenv(key, value)

parser = FabArgumentParser()
args = parser.parse_args(argv)

assert args.cc == cc
assert args.cxx == cxx
assert args.fc == fc