Skip to content

check minimum gcc version - #143

Open
roiser wants to merge 2 commits into
MadGraphTeam:mainfrom
roiser:feat-mingcc
Open

roiser wants to merge 2 commits into
MadGraphTeam:mainfrom
roiser:feat-mingcc

Conversation

@roiser

@roiser roiser commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Hi, I would like to propose to check the minimum compiler version, e.g. I ran into the problem that madspace was not compiling with gcc 11 (which I knew) but thought this may be useful. Note this is only checking the major version of gcc for the time being, cheers

@oliviermattelaer

Copy link
Copy Markdown
Contributor

Two points here:

  • gcc 13 is working (I just used it on the GPU node to test the GPU side)
  • the minimal version should not be set in a "user" configuration file (better to hardcode it in the code itself)

@oliviermattelaer

Copy link
Copy Markdown
Contributor

Good for me :-)
We can merge

@oliviermattelaer

Copy link
Copy Markdown
Contributor

Now the automated check (we should add the automatic review for this repo like we have for madgraph5).
Indicates that we do have repetition with this new command, so maybe worth trying to unify that as much as possible.
Here is the report.

Thanks for this — the check itself is welcome. One remark before merging: get_gcc_version() re-implements code that already exists in misc.py, so we'd end up with the same logic in three places.

What's already there:

Existing helper Location Overlap with this PR
get_gfortran_version(compiler) madgraph/various/misc.py:730 identical Popen([compiler, '-dumpversion']) + re version parsing
detect_if_cpp_compiler_is_clang(compiler) madgraph/various/misc.py:952 the --version banner probe used here to reject Apple's clang-as-gcc
check_compiler(options, block) madgraph/interface/amcatnlo_run_interface.py:164 the "parse, compare to a minimum, warn or raise" logic (hand-rolled there with .split('.') / int())

There is a fourth, inline comparison at misc.py:709 (''.join(get_gfortran_version().split('.')) >= '46').

Suggested merge — one parsing core, everything else layered on top:

def get_compiler_version(compiler, skip_clang=False):
    """Version of `compiler` as a tuple of int, e.g. (13, 2, 1).
       None if it is not in the PATH, its version cannot be parsed, or
       -- when skip_clang is True -- it is in fact clang."""
  • get_gfortran_version() keeps its string return (its three callers compare strings) but becomes a three-line formatter over the core;
  • get_gcc_version() disappears, the clang rejection delegating to the existing detect_if_cpp_compiler_is_clang() via skip_clang=True;
  • check_gcc_version('13') becomes check_compiler_version('13', compiler='gcc'), so a g++ or gfortran minimum is a one-liner.

Net +51/−8 instead of +49/−0, with two fewer parallel implementations. Verified on macOS: gcc reports (21, 0, 0) (Apple clang) and is correctly skipped, gfortran still returns the same string; ./tests/test_manager.py -p U test_cmd is green (19 tests).

Two related points:

  1. The motivating case (madspace failing on gcc 11) is really a g++ requirement — madspace/CMakeLists.txt:320 asks for cxx_std_20 — while the check probes gcc. They share a version on Linux, so it works, but a generic helper lets us check the compiler we actually care about.
  2. '13' is hardcoded at the call site, yet check_gcc_version keeps a logger.debug('ignoring invalid min_gcc_version: ...') guard that only makes sense if the value comes from a config option. Was a min_gcc_version option intended?

@roiser

roiser commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

thanks, all good points, I'll have a look!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants