Skip to content

CMake: centralize target language standard requirements - #3512

Open
agourakis82 wants to merge 4 commits into
boutproject:nextfrom
agourakis82:fix/3502-target-language-standards
Open

agourakis82 wants to merge 4 commits into
boutproject:nextfrom
agourakis82:fix/3502-target-language-standards

Conversation

@agourakis82

Copy link
Copy Markdown

Fixes #3502.

Centralize the C++20 minimum in a target-local helper shared by CXX, CUDA and
HIP. Preserve newer standards selected by callers and require the selected
standard instead of allowing silent fallback. Remove the global HIP standard
defaults and duplicate per-target settings.

Add CMake regression tests for the minimum, preservation of C++23, older
standards (including C++98), and unchanged directory defaults/unrelated targets.
Keep the library's public C++20 compile-feature requirement.

Validation:

  • Full CPU library and serial unit-test executable built with GCC 15.2 in
    C++23 mode; all three CTest entries passed. NetCDF and PVODE enabled;
    optional FFTW, LAPACK and SUNDIALS disabled in this profile.
  • Real CUDA helper probes compiled with NVCC 12.9.86 (C++20) and Clang 18.1.8
    (C++23).
  • Real HIP helper probes compiled with ROCm 7.2.4 / AMD Clang 22.0.0git:
    requested C++17 raised to C++20, and requested C++23 preserved.
  • GPU probes checked both compiler flags and compile-time language macros.

CUDA/HIP validation is compile-only for the helper, not a full BOUT++ GPU
library build or GPU execution. No solver or physics changes.

@agourakis82

Copy link
Copy Markdown
Author

The Read the Docs status links to build 35073848 at this PR's exact head, 7b486720. Its public API reports cancelled, duration 0, and notifications for inactivity termination and manual cancellation; it does not report a compiler or Sphinx error.

Could a maintainer restart the documentation build and approve the held GitHub Actions runs? The cancelled build does not provide a documentation validation result for this patch.

@agourakis82

Copy link
Copy Markdown
Author

Follow-up in bb50cd5 addresses the failure from the previous run.

The | in the ctest -R expression was being passed through the generated Makefile to /bin/sh, which interpreted it as a pipeline and tried to execute cmake-language-standards as a command (exit 127). This happened after the preceding tests had completed. The target now uses two separate ${CMAKE_CTEST_COMMAND} invocations, so the regex is never parsed by the shell.

I configured this exact commit with CMake and MPI on a private Linux cluster and ran the focused CMake test: 2/2 tests passed, including cmake-language-standards-build. The full unit-target build also exposed an unrelated baseline configuration issue when NetCDF is disabled (RegisterUnavailableOptionsIO is not defined in that configuration); I am not treating that as evidence against this patch. The GitHub check suite has not reported yet for bb50cd5.

@agourakis82

Copy link
Copy Markdown
Author

The new CI failure is in the format-command workflow checkout, before clang-format runs; it is unrelated to the patch.

auto-formatting.yml uses actions/checkout with ref: ${{ github.head_ref }} but does not set repository. For this cross-repository PR, github.head_ref is fix/3502-target-language-standards, which exists in agourakis82/BOUT-dev but not in boutproject/BOUT-dev. The job therefore fails with: A branch or tag with the name ... could not be found.

The workflow needs to check out the pull request head repository explicitly (or use the pull request head SHA), e.g. repository: ${{ github.event.pull_request.head.repo.full_name }} together with the head ref/SHA. No source change is needed for this CI failure.

@agourakis82

Copy link
Copy Markdown
Author

I pushed c48430e with the workflow-only fix. The new runs for this head are currently action_required with no jobs created, so GitHub has not executed them yet; this is the approval gate for workflows from a fork, triggered here because the PR modifies .github/workflows. Once a maintainer approves the runs, format-command should exercise the corrected PR-ref checkout.

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.

Set C++/CUDA/HIP standards consistently

1 participant