Skip to content

Commit a94d78b

Browse files
authored
ci: Gate the compiler warnings the glue answers for, in rcc and in each (#3004)
1 parent cfb61cb commit a94d78b

8 files changed

Lines changed: 251 additions & 15 deletions

File tree

‎.github/workflows/custom/after-install/action.yml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,16 @@ runs:
3232
run: Rscript tools/split-aaa-tests.R --check
3333
shell: bash
3434

35+
# The compiler-warning gate (handbook/build/warnings/): compiles the glue
36+
# in src/ with the gate's warning flags, in a makefile of its own rather
37+
# than ~/.R/Makevars, which R CMD check would read too. Fails on a warning
38+
# in a glue file and only counts those in src/vendor/. Seconds, and the
39+
# answer is the same in every matrix entry, so it runs in the smoke test only.
40+
- name: Check that the glue compiles without warnings
41+
if: github.job == 'rcc-smoke' && runner.os != 'Windows'
42+
run: scripts/warnings.sh glue
43+
shell: bash
44+
3545
# The roclet that links the C manual from the reference pages, named in
3646
# DESCRIPTION's Roxygen field, is vendored in tools/igraph.r2cdocs
3747
# (handbook/architecture/r-layer/documentation/). Only the smoke test

‎handbook/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ What stays outside the tree, intent and evidence, and where each lives, is [`met
1111

1212
* [`usage/`](usage/): installing the package and its flavors, and the deprecated dotted function names
1313
* [`architecture/`](architecture/): the R layer, the C glue, the vendored igraph C library, and the Stimulus code generator
14-
* [`build/`](build/): the source build and its configuration
14+
* [`build/`](build/): the source build and its configuration, and the compiler warnings it may raise
1515
* [`testing/`](testing/): the suite, snapshots, guards, and reverse dependencies
1616
* [`branches/`](branches/): the series and their refs, the mirrors, the flavors, the invariants
1717
* [`operations/`](operations/): vendoring, CI, releases, review, triage, and the site

‎handbook/build/README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,4 @@
33
Turning the tree into an installed package.
44

55
* [`source-build/`](source-build/): `configure`, the Makevars, and the build artifacts
6+
* [`warnings/`](warnings/): what the compiler may say about the glue and the vendored code, and the gate that checks it

‎handbook/build/warnings/README.md‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
# Warnings
2+
3+
What the compiler may say about the code this repository owns, and where that is checked.
4+
[`scripts/warnings.sh`](/scripts/warnings.sh) is the check, and its header says how to run it.
5+
6+
**The glue compiles without warnings; the vendored code is counted, never enforced.**
7+
A warning belongs to the file it points at, not to the translation unit that raised it,
8+
so a glue file that includes an igraph header does not answer for what that header says.
9+
10+
* **Glue** is `src/` outside `src/vendor/`: the hand-written C and C++,
11+
`src/rinterface.c`, whose generator and definitions live here ([`architecture/stimulus/`](/handbook/architecture/stimulus/README.md)),
12+
and `src/cpp11.cpp`.
13+
A warning there is fixed here, at its cause: in the source, or in the generator for a generated file.
14+
* **Vendored** is `src/vendor/`: the igraph C library and what it vendors in turn, ARPACK's Fortran among it.
15+
A warning there is fixed upstream, in igraph/igraph, and arrives with the commit the series vendors
16+
([`operations/vendoring/model/`](/handbook/operations/vendoring/model/README.md)).
17+
The gate never fails on it, so the series replays upstream as it is.
18+
* **External** is a header outside the checkout, R's or cpp11's, and is counted like the vendored code.
19+
20+
The one answer never available is silencing a single site with a `#pragma` or a cast that only hides the warning.
21+
22+
## The flags
23+
24+
`-Wall -pedantic` is what CRAN's own GCC checks compile with, and the gate starts there.
25+
`-Wextra` is added, because on the glue it costs nothing today.
26+
Two categories are then switched off, each for a reason the glue cannot remove:
27+
28+
* **`-Wunused-parameter`.**
29+
Callbacks implement signatures igraph and R dictate, the RNG and attribute handlers among them.
30+
* **`-Wcast-function-type`.**
31+
R registers native routines through `DL_FUNC`, a cast `cpp11::cpp_register()` writes for every entry point in `src/cpp11.cpp`.
32+
33+
ISO C's conversions between function and object pointers are dropped from the report,
34+
the exemption R CMD check makes for them too:
35+
igraph keeps an R closure behind `igraph_function_pointer_t`, so the glue cannot avoid one.
36+
37+
**None of this reaches `src/Makevars` or `~/.R/Makevars`.**
38+
CRAN rejects `-Wno-*` in a package's own flags,
39+
and a user Makevars is read by every later build in the job, R CMD check's among them.
40+
The gate compiles the glue objects through a makefile of its own and leaves the package build alone.
41+
42+
## What R CMD check already catches
43+
44+
R CMD check scans the install log for a fixed list of *significant warnings*
45+
(`tools:::.check_packages`: implicit declarations, `-Wformat`, `-Wstrict-prototypes` and a few more),
46+
and the `check` step fails on any WARNING, vendored code included.
47+
It sees only the flags the job's R was built with, which are fewer than CRAN's.
48+
A full build under CRAN's `-Wall -pedantic` raises none of the significant ones today,
49+
so the gate here is for the warnings CRAN prints without failing on.
50+
51+
## Where it runs
52+
53+
* **On every pull request**, in the `rcc` smoke test, through `custom/after-install`.
54+
The answer is the same in every matrix entry, so it runs in one.
55+
* **On every series commit**, as the `warnings` gate of [`scripts/rcc-one.sh`](/scripts/rcc-one.sh)
56+
([`operations/ci/per-commit/`](/handbook/operations/ci/per-commit/README.md)).
57+
A vendored commit that makes the glue warn, a renamed or deprecated declaration say,
58+
is repaired on the R side like any other breakage the walk surfaces
59+
([`operations/vendoring/adaptation/`](/handbook/operations/vendoring/adaptation/README.md)).
60+
* **Locally**, `scripts/warnings.sh` for the glue in seconds,
61+
and `scripts/warnings.sh vendored` for the whole vendored tree, which lists its warnings and always succeeds.
62+
63+
The gate is GCC's, and CI's GCC is newer than a development box's,
64+
so a run that is clean locally is evidence rather than proof.
65+
Clang is welcome as a second opinion, and is not wired into the gate.

‎scripts/rcc-one.sh‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
# | style | gate `style` |
2424
# | update-snapshots | gate `snapshots`, incl. the snapshot-<sha> branch |
2525
# | roxygenize | gate `roxygen` |
26+
# | custom/after-install | gate `warnings`, scripts/warnings.sh glue |
2627
# | render-readme | skipped upstream too (no Config/cynkra/render-readme) |
2728
# | commit | gate `clean` -- workflow_dispatch fails on any diff |
2829
# | check | gate `check` |
@@ -43,7 +44,7 @@
4344
#
4445
# Environment variables:
4546
# EACH_GATES - space-separated subset of
46-
# "regen flavor style snapshots roxygen clean check pkgdown"
47+
# "regen flavor style snapshots roxygen clean warnings check pkgdown"
4748
# (default: all of them, parity with rcc-smoke)
4849
# EACH_SNAPSHOT_BRANCH - publish the snapshot-<sha>-rcc-smoke-null branch when
4950
# the snapshots gate changed anything
@@ -66,7 +67,7 @@
6667

6768
set -uo pipefail
6869

69-
ALL_GATES="regen flavor style snapshots roxygen clean check pkgdown"
70+
ALL_GATES="regen flavor style snapshots roxygen clean warnings check pkgdown"
7071
GATES="${EACH_GATES:-${ALL_GATES}}"
7172
SNAPSHOT_BRANCH="${EACH_SNAPSHOT_BRANCH:-${GITHUB_ACTIONS:-false}}"
7273
STAGE_DIR="${EACH_STAGE_DIR:-}"
@@ -191,6 +192,7 @@ stage_budget() {
191192
snapshots) echo 1800 ;; # the snapshot tests: ~350 s
192193
roxygen) echo 900 ;; # ~60 s, after installing the vendored roclet
193194
clean) echo 900 ;;
195+
warnings) echo 600 ;; # recompiles the ten glue files: ~10 s
194196
check) echo 2700 ;; # rebuilds the package, then the suite again: ~400 s
195197
pkgdown) echo 2700 ;; # every reference page and article: ~650 s
196198
*) echo 3600 ;;
@@ -596,6 +598,13 @@ gate_clean() {
596598
return 1
597599
}
598600

601+
# The compiler-warning gate (handbook/build/warnings/): the glue must compile
602+
# without warnings under the gate's flags; what src/vendor/ says is counted, so
603+
# a vendored commit that brings a new upstream warning does not turn red here.
604+
gate_warnings() {
605+
scripts/warnings.sh glue
606+
}
607+
599608
# Mirrors r-lib/actions/check-r-package@v2 as invoked by
600609
# cynkra/cynkratemplate's check action.
601610
gate_check() {

‎scripts/warnings.sh‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
#!/bin/bash
2+
# The compiler-warning gate: compile part of src/ with the gate's warning flags
3+
# and judge what the compiler says about the files this repository owns.
4+
# Handbook: handbook/build/warnings/README.md
5+
#
6+
# Usage:
7+
# scripts/warnings.sh [glue] # compile the glue, fail on a glue warning
8+
# scripts/warnings.sh vendored # compile everything else, report and exit 0
9+
#
10+
# A warning belongs to the file it points at, not to the translation unit that
11+
# raised it: a glue file is not answerable for what an igraph header says.
12+
# So every warning is attributed to a scope by its path,
13+
# only the glue's fail the gate, and the others are counted.
14+
#
15+
# The flags reach this one compile through a makefile of its own,
16+
# never src/Makevars (CRAN rejects -Wno-* there)
17+
# and never ~/.R/Makevars (every later build in the job would read it,
18+
# R CMD check among them, and turn these warnings into its own).
19+
# The user Makevars is included, so ccache's wrappers still apply.
20+
#
21+
# Linux and macOS only: it needs the src/Makevars that ./configure writes.
22+
23+
set -euo pipefail
24+
25+
cd "$(dirname "$0")/.."
26+
root="$(pwd)"
27+
28+
scope="${1:-glue}"
29+
case "${scope}" in
30+
glue | vendored) ;;
31+
*)
32+
echo "Usage: scripts/warnings.sh [glue|vendored]" >&2
33+
exit 2
34+
;;
35+
esac
36+
37+
# The reasons for each flag are the handbook page's.
38+
WARNINGS="-Wall -Wextra -pedantic -Wno-unused-parameter -Wno-cast-function-type"
39+
40+
tmp="$(mktemp -d)"
41+
trap 'rm -rf "${tmp}"' EXIT
42+
43+
user_makevars="${R_MAKEVARS_USER:-${HOME}/.R/Makevars}"
44+
{
45+
[ -f "${user_makevars}" ] && echo "include ${user_makevars}"
46+
# What R CMD INSTALL puts on the command line for the LinkingTo packages.
47+
Rscript -e 'l <- tools:::.get_requires_from_package_db(read.dcf("DESCRIPTION")[1, ], "LinkingTo")
48+
cat("CLINK_CPPFLAGS =", paste0("-I\"", vapply(l, function(p) system.file("include", package = p), ""), "\""), "\n")'
49+
for v in CFLAGS CXXFLAGS CXX11FLAGS CXX14FLAGS CXX17FLAGS CXX20FLAGS FFLAGS; do
50+
echo "${v} += ${WARNINGS}"
51+
done
52+
echo 'print-%: ; @echo "objects: $($*)"'
53+
} > "${tmp}/warnings.mk"
54+
55+
[ -f src/Makevars ] || ./configure
56+
57+
r_make() {
58+
(cd src && R CMD make -f Makevars -f "$(R RHOME)/etc/Makeconf" -f "${tmp}/warnings.mk" "$@")
59+
}
60+
61+
# The object lists as src/Makevars has them, one per line.
62+
# Make may say other things on the way (src/deps.mk refreshes the .dd files),
63+
# and everything here is later passed to `rm`, so only *.o words are kept.
64+
objects_of() {
65+
r_make -s "${@/#/print-}" | sed -n 's/^objects: //p' | tr ' ' '\n' | grep -E '^[A-Za-z0-9_./-]+\.o$' | sort -u
66+
}
67+
68+
objects_of GLUE_C_SOURCES GLUE_CPP_SOURCES > "${tmp}/glue.txt"
69+
if [ "${scope}" = glue ]; then
70+
objects="$(cat "${tmp}/glue.txt")"
71+
else
72+
objects="$(objects_of OBJECTS | grep -vxF -f "${tmp}/glue.txt")"
73+
fi
74+
if [ -z "${objects}" ]; then
75+
echo "No ${scope} objects in src/Makevars; nothing was judged."
76+
exit 1
77+
fi
78+
79+
# Removed first, so that every object is compiled and so speaks;
80+
# ccache replays a cached compile's warnings, so a warm cache hides nothing.
81+
# shellcheck disable=SC2086
82+
(cd src && rm -f ${objects})
83+
# shellcheck disable=SC2086
84+
r_make -j"$(nproc)" ${objects} > "${tmp}/build.log" 2>&1 || {
85+
cat "${tmp}/build.log"
86+
echo "The compile failed; see above."
87+
exit 1
88+
}
89+
90+
# One line per distinct warning, paths relative to src/ where they are ours.
91+
# R CMD check itself exempts ISO C's function pointer conversions
92+
# (tools:::.check_packages), and the igraph API stores R closures behind
93+
# igraph_function_pointer_t, so the glue cannot avoid them; they are dropped.
94+
grep -E '^[^ ].*:[0-9]+:[0-9]+: warning:' "${tmp}/build.log" |
95+
sed -e "s|^${root}/src/||" -e "s|^${root}/||" -e 's|^src/||' |
96+
grep -vE 'warning: *ISO C forbids.*function pointer' |
97+
sort -u > "${tmp}/warnings.txt" || true
98+
99+
# vendored: src/vendor/, including the lexers whose #line points back there;
100+
# external: anything outside the checkout (R's headers, cpp11's, the system's);
101+
# glue: the rest of src/.
102+
awk -F: '
103+
{ s = ($1 ~ /(^|\/)vendor\//) ? "vendored" : ($1 ~ /^\//) ? "external" : "glue"; print s "\t" $0 }
104+
' "${tmp}/warnings.txt" > "${tmp}/scoped.txt"
105+
106+
fortran="$(grep -c '^Warning:' "${tmp}/build.log" || true)"
107+
108+
echo "Compiled $(echo "${objects}" | wc -w) objects with ${WARNINGS}."
109+
for s in glue vendored external; do
110+
echo " $(grep -c "^${s} " "${tmp}/scoped.txt" || true) warnings in ${s} files"
111+
done
112+
[ "${scope}" = vendored ] && echo " ${fortran} Fortran warnings (vendored ARPACK)"
113+
114+
if [ "${scope}" = vendored ]; then
115+
cut -f2- "${tmp}/scoped.txt"
116+
exit 0
117+
fi
118+
119+
if grep -q '^glue ' "${tmp}/scoped.txt"; then
120+
echo
121+
echo "The glue must compile without warnings (handbook/build/warnings/README.md):"
122+
grep '^glue ' "${tmp}/scoped.txt" | cut -f2-
123+
echo
124+
echo "Context from the compiler:"
125+
cat "${tmp}/build.log"
126+
exit 1
127+
fi
128+
echo "The glue compiles without warnings."

‎src/rinterface.c‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9783,7 +9783,7 @@ SEXP R_igraph_maximal_cliques_subset(SEXP graph, SEXP subset, SEXP outfile, SEXP
97839783
igraph_vector_int_t c_subset;
97849784
igraph_vector_int_list_t c_res;
97859785
igraph_integer_t c_no;
9786-
FILE* c_outfile;
9786+
FILE* c_outfile = 0;
97879787
igraph_integer_t c_min_size;
97889788
igraph_integer_t c_max_size;
97899789
SEXP res;
@@ -13992,7 +13992,7 @@ SEXP R_igraph_write_graph_gml(SEXP graph, SEXP outstream, SEXP options, SEXP id,
1399213992
FILE* c_outstream;
1399313993
igraph_write_gml_sw_t c_options;
1399413994
igraph_vector_t c_id;
13995-
const char* c_creator;
13995+
const char* c_creator = 0;
1399613996

1399713997
/* Convert input */
1399813998
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
1808518085
/* Declarations */
1808618086
igraph_t c_graph;
1808718087
igraph_vector_int_list_t c_basis;
18088-
igraph_integer_t c_start;
18088+
igraph_integer_t c_start = 0;
1808918089
igraph_integer_t c_bfs_cutoff;
1809018090
igraph_vector_t c_weights;
1809118091
SEXP basis;
@@ -18437,7 +18437,7 @@ SEXP R_igraph_random_spanning_tree(SEXP graph, SEXP vid) {
1843718437
/* Declarations */
1843818438
igraph_t c_graph;
1843918439
igraph_vector_int_t c_res;
18440-
igraph_integer_t c_vid;
18440+
igraph_integer_t c_vid = 0;
1844118441
SEXP res;
1844218442

1844318443
SEXP r_result;
@@ -18565,9 +18565,8 @@ SEXP R_igraph_is_bipartite_coloring(SEXP graph, SEXP types) {
1856518565
igraph_bool_t c_res;
1856618566

1856718567
SEXP res;
18568-
SEXP mode;
1856918568

18570-
SEXP r_result, r_names;
18569+
SEXP r_result;
1857118570
/* Convert input */
1857218571
Rz_SEXP_to_igraph(graph, &c_graph);
1857318572
Rz_SEXP_to_vector_bool(types, &c_types);
@@ -18954,7 +18953,7 @@ SEXP R_igraph_invalidate_cache(SEXP graph) {
1895418953
SEXP R_igraph_vertex_path_from_edge_path(SEXP graph, SEXP start, SEXP edge_path, SEXP mode) {
1895518954
/* Declarations */
1895618955
igraph_t c_graph;
18957-
igraph_integer_t c_start;
18956+
igraph_integer_t c_start = 0;
1895818957
igraph_vector_int_t c_edge_path;
1895918958
igraph_vector_int_t c_vertex_path;
1896018959
igraph_neimode_t c_mode;

‎tools/py-stimulus/src/stimulus/generators/r.py‎

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -523,26 +523,50 @@ def chunk_declaration(self, desc: FunctionDescriptor) -> str:
523523
def do_par(spec: ParamSpec) -> str:
524524
type_desc = self.get_type_descriptor(spec.type)
525525
try:
526-
return type_desc.declare_c_variable(f"c_{spec.name}", mode=spec.mode)
526+
decl = type_desc.declare_c_variable(f"c_{spec.name}", mode=spec.mode)
527527
except NoSuchTypeError:
528528
return f"/* {spec.name} has no corresponding C type */"
529529

530+
# An optional input passed by value is only assigned when it is
531+
# not NULL, and only read when it is not NULL either (see
532+
# chunk_call()), but the compiler cannot tell that the two
533+
# Rf_isNull() calls agree and warns with -Wmaybe-uninitialized.
534+
call = type_desc.get("CALL", "%C%")
535+
if isinstance(call, dict):
536+
call = call.get(spec.mode_str, "")
537+
if (
538+
spec.is_optional
539+
and spec.is_input
540+
and not spec.is_output
541+
and call == "%C%"
542+
and decl.endswith(";")
543+
and "=" not in decl
544+
):
545+
decl = decl[:-1] + " = 0;"
546+
return decl
547+
548+
def has_c_type(spec: ParamSpec) -> bool:
549+
return self.get_type_descriptor(spec.type).get_c_type(spec.mode) is not None
550+
530551
inout = [do_par(spec) for spec in desc.iter_parameters()]
552+
# Only the outputs with a C counterpart are converted and returned
553+
# (see chunk_outconv()); declaring the others leaves unused variables.
531554
out = [
532555
f"SEXP {spec.name};"
533556
for spec in desc.iter_parameters()
534-
if spec.mode is ParamMode.OUT
557+
if spec.mode is ParamMode.OUT and has_c_type(spec)
535558
]
536559

537-
retpars = [spec.name for spec in desc.iter_output_parameters()]
560+
all_retpars = list(desc.iter_output_parameters())
561+
retpars = [spec for spec in all_retpars if has_c_type(spec)]
538562

539563
return_type_desc = self.get_type_descriptor(desc.return_type)
540-
if retpars or return_type_desc.name in ("VOID", "ERROR"):
564+
if all_retpars or return_type_desc.name in ("VOID", "ERROR"):
541565
retdecl = ""
542566
else:
543567
retdecl = return_type_desc.declare_c_variable("c_result")
544568

545-
if len(retpars) == 0 and desc.return_type == "ERROR":
569+
if len(all_retpars) == 0 and desc.return_type == "ERROR":
546570
res = "\n".join(inout + out + [retdecl])
547571
elif len(retpars) <= 1:
548572
res = "\n".join(inout + out + [retdecl] + ["SEXP r_result;"])

0 commit comments

Comments
 (0)