diff --git a/.github/workflows/custom/after-install/action.yml b/.github/workflows/custom/after-install/action.yml index 85b22604c60..bb28e3f3fda 100644 --- a/.github/workflows/custom/after-install/action.yml +++ b/.github/workflows/custom/after-install/action.yml @@ -32,6 +32,16 @@ runs: run: Rscript tools/split-aaa-tests.R --check shell: bash + # The compiler-warning gate (handbook/build/warnings/): compiles the glue + # in src/ with the gate's warning flags, in a makefile of its own rather + # than ~/.R/Makevars, which R CMD check would read too. Fails on a warning + # in a glue file and only counts those in src/vendor/. Seconds, and the + # answer is the same in every matrix entry, so it runs in the smoke test only. + - name: Check that the glue compiles without warnings + if: github.job == 'rcc-smoke' && runner.os != 'Windows' + run: scripts/warnings.sh glue + shell: bash + # The roclet that links the C manual from the reference pages, named in # DESCRIPTION's Roxygen field, is vendored in tools/igraph.r2cdocs # (handbook/architecture/r-layer/documentation/). Only the smoke test diff --git a/handbook/README.md b/handbook/README.md index d906d0d87ba..463370fc652 100644 --- a/handbook/README.md +++ b/handbook/README.md @@ -11,7 +11,7 @@ What stays outside the tree, intent and evidence, and where each lives, is [`met * [`usage/`](usage/): installing the package and its flavors, and the deprecated dotted function names * [`architecture/`](architecture/): the R layer, the C glue, the vendored igraph C library, and the Stimulus code generator -* [`build/`](build/): the source build and its configuration +* [`build/`](build/): the source build and its configuration, and the compiler warnings it may raise * [`testing/`](testing/): the suite, snapshots, guards, and reverse dependencies * [`branches/`](branches/): the series and their refs, the mirrors, the flavors, the invariants * [`operations/`](operations/): vendoring, CI, releases, review, triage, and the site diff --git a/handbook/build/README.md b/handbook/build/README.md index d1fe13038c3..4a9bd8fb0ba 100644 --- a/handbook/build/README.md +++ b/handbook/build/README.md @@ -3,3 +3,4 @@ Turning the tree into an installed package. * [`source-build/`](source-build/): `configure`, the Makevars, and the build artifacts +* [`warnings/`](warnings/): what the compiler may say about the glue and the vendored code, and the gate that checks it diff --git a/handbook/build/warnings/README.md b/handbook/build/warnings/README.md new file mode 100644 index 00000000000..781a06e4f29 --- /dev/null +++ b/handbook/build/warnings/README.md @@ -0,0 +1,65 @@ +# Warnings + +What the compiler may say about the code this repository owns, and where that is checked. +[`scripts/warnings.sh`](/scripts/warnings.sh) is the check, and its header says how to run it. + +**The glue compiles without warnings; the vendored code is counted, never enforced.** +A warning belongs to the file it points at, not to the translation unit that raised it, +so a glue file that includes an igraph header does not answer for what that header says. + +* **Glue** is `src/` outside `src/vendor/`: the hand-written C and C++, + `src/rinterface.c`, whose generator and definitions live here ([`architecture/stimulus/`](/handbook/architecture/stimulus/README.md)), + and `src/cpp11.cpp`. + A warning there is fixed here, at its cause: in the source, or in the generator for a generated file. +* **Vendored** is `src/vendor/`: the igraph C library and what it vendors in turn, ARPACK's Fortran among it. + A warning there is fixed upstream, in igraph/igraph, and arrives with the commit the series vendors + ([`operations/vendoring/model/`](/handbook/operations/vendoring/model/README.md)). + The gate never fails on it, so the series replays upstream as it is. +* **External** is a header outside the checkout, R's or cpp11's, and is counted like the vendored code. + +The one answer never available is silencing a single site with a `#pragma` or a cast that only hides the warning. + +## The flags + +`-Wall -pedantic` is what CRAN's own GCC checks compile with, and the gate starts there. +`-Wextra` is added, because on the glue it costs nothing today. +Two categories are then switched off, each for a reason the glue cannot remove: + +* **`-Wunused-parameter`.** + Callbacks implement signatures igraph and R dictate, the RNG and attribute handlers among them. +* **`-Wcast-function-type`.** + R registers native routines through `DL_FUNC`, a cast `cpp11::cpp_register()` writes for every entry point in `src/cpp11.cpp`. + +ISO C's conversions between function and object pointers are dropped from the report, +the exemption R CMD check makes for them too: +igraph keeps an R closure behind `igraph_function_pointer_t`, so the glue cannot avoid one. + +**None of this reaches `src/Makevars` or `~/.R/Makevars`.** +CRAN rejects `-Wno-*` in a package's own flags, +and a user Makevars is read by every later build in the job, R CMD check's among them. +The gate compiles the glue objects through a makefile of its own and leaves the package build alone. + +## What R CMD check already catches + +R CMD check scans the install log for a fixed list of *significant warnings* +(`tools:::.check_packages`: implicit declarations, `-Wformat`, `-Wstrict-prototypes` and a few more), +and the `check` step fails on any WARNING, vendored code included. +It sees only the flags the job's R was built with, which are fewer than CRAN's. +A full build under CRAN's `-Wall -pedantic` raises none of the significant ones today, +so the gate here is for the warnings CRAN prints without failing on. + +## Where it runs + +* **On every pull request**, in the `rcc` smoke test, through `custom/after-install`. + The answer is the same in every matrix entry, so it runs in one. +* **On every series commit**, as the `warnings` gate of [`scripts/rcc-one.sh`](/scripts/rcc-one.sh) + ([`operations/ci/per-commit/`](/handbook/operations/ci/per-commit/README.md)). + A vendored commit that makes the glue warn, a renamed or deprecated declaration say, + is repaired on the R side like any other breakage the walk surfaces + ([`operations/vendoring/adaptation/`](/handbook/operations/vendoring/adaptation/README.md)). +* **Locally**, `scripts/warnings.sh` for the glue in seconds, + and `scripts/warnings.sh vendored` for the whole vendored tree, which lists its warnings and always succeeds. + +The gate is GCC's, and CI's GCC is newer than a development box's, +so a run that is clean locally is evidence rather than proof. +Clang is welcome as a second opinion, and is not wired into the gate. diff --git a/scripts/rcc-one.sh b/scripts/rcc-one.sh index b6d810facff..25c045bb7b5 100755 --- a/scripts/rcc-one.sh +++ b/scripts/rcc-one.sh @@ -23,6 +23,7 @@ # | style | gate `style` | # | update-snapshots | gate `snapshots`, incl. the snapshot- branch | # | roxygenize | gate `roxygen` | +# | custom/after-install | gate `warnings`, scripts/warnings.sh glue | # | render-readme | skipped upstream too (no Config/cynkra/render-readme) | # | commit | gate `clean` -- workflow_dispatch fails on any diff | # | check | gate `check` | @@ -43,7 +44,7 @@ # # Environment variables: # EACH_GATES - space-separated subset of -# "regen flavor style snapshots roxygen clean check pkgdown" +# "regen flavor style snapshots roxygen clean warnings check pkgdown" # (default: all of them, parity with rcc-smoke) # EACH_SNAPSHOT_BRANCH - publish the snapshot--rcc-smoke-null branch when # the snapshots gate changed anything @@ -66,7 +67,7 @@ set -uo pipefail -ALL_GATES="regen flavor style snapshots roxygen clean check pkgdown" +ALL_GATES="regen flavor style snapshots roxygen clean warnings check pkgdown" GATES="${EACH_GATES:-${ALL_GATES}}" SNAPSHOT_BRANCH="${EACH_SNAPSHOT_BRANCH:-${GITHUB_ACTIONS:-false}}" STAGE_DIR="${EACH_STAGE_DIR:-}" @@ -191,6 +192,7 @@ stage_budget() { snapshots) echo 1800 ;; # the snapshot tests: ~350 s roxygen) echo 900 ;; # ~60 s, after installing the vendored roclet clean) echo 900 ;; + warnings) echo 600 ;; # recompiles the ten glue files: ~10 s check) echo 2700 ;; # rebuilds the package, then the suite again: ~400 s pkgdown) echo 2700 ;; # every reference page and article: ~650 s *) echo 3600 ;; @@ -596,6 +598,13 @@ gate_clean() { return 1 } +# The compiler-warning gate (handbook/build/warnings/): the glue must compile +# without warnings under the gate's flags; what src/vendor/ says is counted, so +# a vendored commit that brings a new upstream warning does not turn red here. +gate_warnings() { + scripts/warnings.sh glue +} + # Mirrors r-lib/actions/check-r-package@v2 as invoked by # cynkra/cynkratemplate's check action. gate_check() { diff --git a/scripts/warnings.sh b/scripts/warnings.sh new file mode 100755 index 00000000000..060167576ca --- /dev/null +++ b/scripts/warnings.sh @@ -0,0 +1,128 @@ +#!/bin/bash +# The compiler-warning gate: compile part of src/ with the gate's warning flags +# and judge what the compiler says about the files this repository owns. +# Handbook: handbook/build/warnings/README.md +# +# Usage: +# scripts/warnings.sh [glue] # compile the glue, fail on a glue warning +# scripts/warnings.sh vendored # compile everything else, report and exit 0 +# +# A warning belongs to the file it points at, not to the translation unit that +# raised it: a glue file is not answerable for what an igraph header says. +# So every warning is attributed to a scope by its path, +# only the glue's fail the gate, and the others are counted. +# +# The flags reach this one compile through a makefile of its own, +# never src/Makevars (CRAN rejects -Wno-* there) +# and never ~/.R/Makevars (every later build in the job would read it, +# R CMD check among them, and turn these warnings into its own). +# The user Makevars is included, so ccache's wrappers still apply. +# +# Linux and macOS only: it needs the src/Makevars that ./configure writes. + +set -euo pipefail + +cd "$(dirname "$0")/.." +root="$(pwd)" + +scope="${1:-glue}" +case "${scope}" in + glue | vendored) ;; + *) + echo "Usage: scripts/warnings.sh [glue|vendored]" >&2 + exit 2 + ;; +esac + +# The reasons for each flag are the handbook page's. +WARNINGS="-Wall -Wextra -pedantic -Wno-unused-parameter -Wno-cast-function-type" + +tmp="$(mktemp -d)" +trap 'rm -rf "${tmp}"' EXIT + +user_makevars="${R_MAKEVARS_USER:-${HOME}/.R/Makevars}" +{ + [ -f "${user_makevars}" ] && echo "include ${user_makevars}" + # What R CMD INSTALL puts on the command line for the LinkingTo packages. + Rscript -e 'l <- tools:::.get_requires_from_package_db(read.dcf("DESCRIPTION")[1, ], "LinkingTo") + cat("CLINK_CPPFLAGS =", paste0("-I\"", vapply(l, function(p) system.file("include", package = p), ""), "\""), "\n")' + for v in CFLAGS CXXFLAGS CXX11FLAGS CXX14FLAGS CXX17FLAGS CXX20FLAGS FFLAGS; do + echo "${v} += ${WARNINGS}" + done + echo 'print-%: ; @echo "objects: $($*)"' +} > "${tmp}/warnings.mk" + +[ -f src/Makevars ] || ./configure + +r_make() { + (cd src && R CMD make -f Makevars -f "$(R RHOME)/etc/Makeconf" -f "${tmp}/warnings.mk" "$@") +} + +# The object lists as src/Makevars has them, one per line. +# Make may say other things on the way (src/deps.mk refreshes the .dd files), +# and everything here is later passed to `rm`, so only *.o words are kept. +objects_of() { + r_make -s "${@/#/print-}" | sed -n 's/^objects: //p' | tr ' ' '\n' | grep -E '^[A-Za-z0-9_./-]+\.o$' | sort -u +} + +objects_of GLUE_C_SOURCES GLUE_CPP_SOURCES > "${tmp}/glue.txt" +if [ "${scope}" = glue ]; then + objects="$(cat "${tmp}/glue.txt")" +else + objects="$(objects_of OBJECTS | grep -vxF -f "${tmp}/glue.txt")" +fi +if [ -z "${objects}" ]; then + echo "No ${scope} objects in src/Makevars; nothing was judged." + exit 1 +fi + +# Removed first, so that every object is compiled and so speaks; +# ccache replays a cached compile's warnings, so a warm cache hides nothing. +# shellcheck disable=SC2086 +(cd src && rm -f ${objects}) +# shellcheck disable=SC2086 +r_make -j"$(nproc)" ${objects} > "${tmp}/build.log" 2>&1 || { + cat "${tmp}/build.log" + echo "The compile failed; see above." + exit 1 +} + +# One line per distinct warning, paths relative to src/ where they are ours. +# R CMD check itself exempts ISO C's function pointer conversions +# (tools:::.check_packages), and the igraph API stores R closures behind +# igraph_function_pointer_t, so the glue cannot avoid them; they are dropped. +grep -E '^[^ ].*:[0-9]+:[0-9]+: warning:' "${tmp}/build.log" | + sed -e "s|^${root}/src/||" -e "s|^${root}/||" -e 's|^src/||' | + grep -vE 'warning: *ISO C forbids.*function pointer' | + sort -u > "${tmp}/warnings.txt" || true + +# vendored: src/vendor/, including the lexers whose #line points back there; +# external: anything outside the checkout (R's headers, cpp11's, the system's); +# glue: the rest of src/. +awk -F: ' + { s = ($1 ~ /(^|\/)vendor\//) ? "vendored" : ($1 ~ /^\//) ? "external" : "glue"; print s "\t" $0 } +' "${tmp}/warnings.txt" > "${tmp}/scoped.txt" + +fortran="$(grep -c '^Warning:' "${tmp}/build.log" || true)" + +echo "Compiled $(echo "${objects}" | wc -w) objects with ${WARNINGS}." +for s in glue vendored external; do + echo " $(grep -c "^${s} " "${tmp}/scoped.txt" || true) warnings in ${s} files" +done +[ "${scope}" = vendored ] && echo " ${fortran} Fortran warnings (vendored ARPACK)" + +if [ "${scope}" = vendored ]; then + cut -f2- "${tmp}/scoped.txt" + exit 0 +fi + +if grep -q '^glue ' "${tmp}/scoped.txt"; then + echo + echo "The glue must compile without warnings (handbook/build/warnings/README.md):" + grep '^glue ' "${tmp}/scoped.txt" | cut -f2- + echo + echo "Context from the compiler:" + cat "${tmp}/build.log" + exit 1 +fi +echo "The glue compiles without warnings." diff --git a/src/rinterface.c b/src/rinterface.c index f74588cd392..fd6f1034f34 100644 --- a/src/rinterface.c +++ b/src/rinterface.c @@ -9783,7 +9783,7 @@ SEXP R_igraph_maximal_cliques_subset(SEXP graph, SEXP subset, SEXP outfile, SEXP igraph_vector_int_t c_subset; igraph_vector_int_list_t c_res; igraph_integer_t c_no; - FILE* c_outfile; + FILE* c_outfile = 0; igraph_integer_t c_min_size; igraph_integer_t c_max_size; SEXP res; @@ -13992,7 +13992,7 @@ SEXP R_igraph_write_graph_gml(SEXP graph, SEXP outstream, SEXP options, SEXP id, FILE* c_outstream; igraph_write_gml_sw_t c_options; igraph_vector_t c_id; - const char* c_creator; + const char* c_creator = 0; /* Convert input */ Rz_SEXP_to_igraph(graph, &c_graph); @@ -18085,7 +18085,7 @@ SEXP R_igraph_fundamental_cycles(SEXP graph, SEXP start, SEXP bfs_cutoff, SEXP w /* Declarations */ igraph_t c_graph; igraph_vector_int_list_t c_basis; - igraph_integer_t c_start; + igraph_integer_t c_start = 0; igraph_integer_t c_bfs_cutoff; igraph_vector_t c_weights; SEXP basis; @@ -18437,7 +18437,7 @@ SEXP R_igraph_random_spanning_tree(SEXP graph, SEXP vid) { /* Declarations */ igraph_t c_graph; igraph_vector_int_t c_res; - igraph_integer_t c_vid; + igraph_integer_t c_vid = 0; SEXP res; SEXP r_result; @@ -18565,9 +18565,8 @@ SEXP R_igraph_is_bipartite_coloring(SEXP graph, SEXP types) { igraph_bool_t c_res; SEXP res; - SEXP mode; - SEXP r_result, r_names; + SEXP r_result; /* Convert input */ Rz_SEXP_to_igraph(graph, &c_graph); Rz_SEXP_to_vector_bool(types, &c_types); @@ -18954,7 +18953,7 @@ SEXP R_igraph_invalidate_cache(SEXP graph) { SEXP R_igraph_vertex_path_from_edge_path(SEXP graph, SEXP start, SEXP edge_path, SEXP mode) { /* Declarations */ igraph_t c_graph; - igraph_integer_t c_start; + igraph_integer_t c_start = 0; igraph_vector_int_t c_edge_path; igraph_vector_int_t c_vertex_path; igraph_neimode_t c_mode; diff --git a/tools/py-stimulus/src/stimulus/generators/r.py b/tools/py-stimulus/src/stimulus/generators/r.py index 088964b58aa..7dc67160058 100644 --- a/tools/py-stimulus/src/stimulus/generators/r.py +++ b/tools/py-stimulus/src/stimulus/generators/r.py @@ -523,26 +523,50 @@ def chunk_declaration(self, desc: FunctionDescriptor) -> str: def do_par(spec: ParamSpec) -> str: type_desc = self.get_type_descriptor(spec.type) try: - return type_desc.declare_c_variable(f"c_{spec.name}", mode=spec.mode) + decl = type_desc.declare_c_variable(f"c_{spec.name}", mode=spec.mode) except NoSuchTypeError: return f"/* {spec.name} has no corresponding C type */" + # An optional input passed by value is only assigned when it is + # not NULL, and only read when it is not NULL either (see + # chunk_call()), but the compiler cannot tell that the two + # Rf_isNull() calls agree and warns with -Wmaybe-uninitialized. + call = type_desc.get("CALL", "%C%") + if isinstance(call, dict): + call = call.get(spec.mode_str, "") + if ( + spec.is_optional + and spec.is_input + and not spec.is_output + and call == "%C%" + and decl.endswith(";") + and "=" not in decl + ): + decl = decl[:-1] + " = 0;" + return decl + + def has_c_type(spec: ParamSpec) -> bool: + return self.get_type_descriptor(spec.type).get_c_type(spec.mode) is not None + inout = [do_par(spec) for spec in desc.iter_parameters()] + # Only the outputs with a C counterpart are converted and returned + # (see chunk_outconv()); declaring the others leaves unused variables. out = [ f"SEXP {spec.name};" for spec in desc.iter_parameters() - if spec.mode is ParamMode.OUT + if spec.mode is ParamMode.OUT and has_c_type(spec) ] - retpars = [spec.name for spec in desc.iter_output_parameters()] + all_retpars = list(desc.iter_output_parameters()) + retpars = [spec for spec in all_retpars if has_c_type(spec)] return_type_desc = self.get_type_descriptor(desc.return_type) - if retpars or return_type_desc.name in ("VOID", "ERROR"): + if all_retpars or return_type_desc.name in ("VOID", "ERROR"): retdecl = "" else: retdecl = return_type_desc.declare_c_variable("c_result") - if len(retpars) == 0 and desc.return_type == "ERROR": + if len(all_retpars) == 0 and desc.return_type == "ERROR": res = "\n".join(inout + out + [retdecl]) elif len(retpars) <= 1: res = "\n".join(inout + out + [retdecl] + ["SEXP r_result;"])