Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,14 @@ SemVer-0 convention): expect breaking changes between 0.x and 0.(x+1), never wit
file with more bytes than tokens, past the end of the file (`deep.proto:122:1` for a 121-line
file). The rejection itself was always correct — only where it pointed was wrong.

- **A schema reached through a symlink no longer regenerates on every build.** `rapidproto_generate()`
declares the generated headers as a build rule's outputs, and the CMake helper computed that path
by resolving symlinks in a case where the generator does not — so for an entry whose link name
differs from its target's, the declared header was never the one written and the target rebuilt
forever. The same applied to a `.proto` produced by another rule under a symlinked path: CMake
cannot resolve a path that does not exist yet, so the helper now resolves the longest part of it
that does — matching what the generator computes at build time, when the file is there.

- **Ninja no longer regenerates the headers on every build.** The depfile listed its targets in
sorted order, but Ninja accepts a depfile only when its *first* target is the build rule's first
output — and a mismatch is not an error, it silently marks the outputs dirty forever. Any target
Expand Down
29 changes: 22 additions & 7 deletions check.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
#
# ./check.sh # full gate: format check, doc links, nsedge fixture coverage, build+test on
# # both compilers, compile-fail, fuzz-compile, clang-tidy, the C++20/23 header
# # smoke and the randomized differential. NOT the corpus sweep -- see `deep`.
# # smoke, the CMake-helper name check and the randomized differential. NOT the
# # corpus sweep -- see `deep`.
# ./check.sh fix # first apply clang-format, then run the full gate
# ./check.sh quick # fast inner loop: apply formatting + gcc build+test only (no clang/tidy)
# ./check.sh deep # OPT-IN heavy tier (CI / end-of-phase, NOT the inner loop): ASan+UBSan over the
Expand All @@ -25,9 +26,10 @@
#
# The independent stages (format, doc-links, fixture-coverage, gcc build+test, clang build+test,
# compile-fail, fuzz-compile, clang-tidy) run concurrently; each build is a parallel build and
# clang-tidy is parallelized across files. Three run after that block: corpus (only when asked for)
# and the differential consume the gcc stage's binaries, and the C++20/23 smoke needs the goldens. Per-stage output is captured and printed in a fixed
# order so nothing interleaves. Exits non-zero if anything is not clean.
# clang-tidy is parallelized across files. Four run after that block: corpus, the CMake-helper name
# check and the differential all consume the gcc stage's binaries (corpus only when asked for), and
# the C++20/23 smoke needs the goldens. Per-stage output is captured and printed in a fixed order so
# nothing interleaves. Exits non-zero if anything is not clean.

set -uo pipefail
cd "$(dirname "$0")"
Expand Down Expand Up @@ -656,6 +658,14 @@ job_cxx20_smoke() {
return $rc
}

# The CMake helper declares the CLI's output paths as a custom command's OUTPUT, so the two rules
# must agree exactly or the declared file is never produced and the target regenerates forever. They
# are written in different languages and have drifted once already.
job_generate_names() {
ensure_gcc_binaries rapidprotoc || return 1
tests/check_generate_names.sh build/gcc/rapidprotoc
}

job_fuzz_compile() {
local cxx=clang++-20 rc=0 out
command -v "$cxx" >/dev/null 2>&1 || cxx=c++
Expand Down Expand Up @@ -827,13 +837,13 @@ job_differential() {
# log-print loop -- so a new stage needs a key, a title, a job and a line in each. Omitting it from
# a run loop fails loudly (`FAILURES: <key>`, no .rc recorded); omitting it from DEFAULT_STAGES
# used to disable it in silence, which the NON_DEFAULT_STAGES check rejects before any stage runs.
readonly STAGE_KEYS=(format docs fixtures gcc clang cf fuzz tidy corpus cxx20 differential)
readonly STAGE_KEYS=(format docs fixtures gcc clang cf fuzz tidy corpus cxx20 names differential)

# What a bare ./check.sh runs. `corpus` is deliberately absent: sweeping ~8000 third-party schemas is
# a COMPATIBILITY check, not a fast-feedback one -- the library's own behaviour is covered by the
# explicit tests -- and at ~163s it was 30% of the gate. It moved to the deep tier (which gates every
# PR) and keeps its own CI runner, so nothing stopped watching it; it just left the inner loop.
readonly DEFAULT_STAGES=(format docs fixtures gcc clang cf fuzz tidy cxx20 differential)
readonly DEFAULT_STAGES=(format docs fixtures gcc clang cf fuzz tidy cxx20 names differential)
# Stages deliberately outside the default gate. Every STAGE_KEYS entry must be in DEFAULT_STAGES or
# here, checked below: a stage left out of BOTH is disabled by omission -- exactly the silent
# forgot-a-list failure the single table was introduced to end.
Expand All @@ -858,6 +868,7 @@ stage_title() {
tidy) echo "clang-tidy (library = strict, tests = relaxed)" ;;
corpus) echo "real-world schema corpus" ;;
cxx20) echo "generated headers at c++20/c++23" ;;
names) echo "cmake helper predicts the CLI's header paths" ;;
differential) echo "randomized differential vs protobuf" ;;
esac
}
Expand All @@ -874,6 +885,7 @@ stage_job() {
tidy) job_tidy ;;
corpus) job_corpus ;;
cxx20) job_cxx20_smoke ;;
names) job_generate_names ;;
differential) job_differential ;;
# Not optional: without it, a key added to STAGE_KEYS but not here falls out of the case with
# status 0 and an empty log -- a stage that can only ever report success, which is the exact
Expand Down Expand Up @@ -977,13 +989,16 @@ run_stage corpus
# Needs the goldens on disk (not a build product), so it can run any time after them.
[[ "$serial_gate" == 1 ]] && stage_enabled cxx20 && echo "serial gate: cxx20"
run_stage cxx20
# Consumes build/gcc's rapidprotoc, so it runs after the build stages.
[[ "$serial_gate" == 1 ]] && stage_enabled names && echo "serial gate: names"
run_stage names
# Also consumes build/gcc's binaries, and compiles a harness per schema, so it runs alone at the end.
[[ "$serial_gate" == 1 ]] && stage_enabled differential && echo "serial gate: differential"
run_stage differential

# --- print each stage's output in a fixed order (already captured, so never interleaved) ----------

for key in cxx20 format docs fixtures gcc clang cf fuzz tidy corpus differential; do
for key in cxx20 names format docs fixtures gcc clang cf fuzz tidy corpus differential; do
section "$(stage_title "$key")"
cat "$LOG/$key"
done
Expand Down
45 changes: 39 additions & 6 deletions cmake/rapidproto-generate.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,44 @@ include_guard(GLOBAL)
# path relative to the first import dir that contains it, else its basename, with ".proto" -> `ext`,
# under `out_dir`. `import_dirs_abs` is the absolute import dirs in -I order.
function(_rapidproto_output_header out_var proto_abs ext out_dir import_dirs_abs)
# Resolve symlinks (REALPATH) so this matches the generator's canonical_entry_name, which uses
# weakly_canonical (symlink-resolving). file(RELATIVE_PATH) is purely lexical, so without this a
# symlinked import dir or entry path would compute a different stem than the CLI actually writes,
# and CMake would error with "output not produced by COMMAND".
get_filename_component(_proto_real "${proto_abs}" REALPATH)
# Resolve symlinks (REALPATH) for the import-relative test, matching canonical_entry_name, which
# weakly_canonical()s both the entry and the include dir before relativizing. file(RELATIVE_PATH)
# is purely lexical, so without this a symlinked import dir would compute a different stem than
# the CLI writes, and CMake would error with "output not produced by COMMAND".
#
# The FALLBACK below deliberately does NOT use the resolved path: when the entry resolves under no
# import dir, canonical_entry_name returns the spelling it was GIVEN, and the CLI names the header
# from that. Taking REALPATH's basename here instead made the two disagree for a symlinked entry
# whose link name differs from its target -- `protolink/alias.proto -> ../real/aaa.proto` had
# CMake declare aaa.rp.hpp while the CLI wrote alias.rp.hpp, so the declared output never appeared
# and the target regenerated on every build.
# Mirror weakly_canonical: resolve the longest EXISTING prefix, then re-attach what is missing.
# REALPATH leaves the WHOLE path unresolved as soon as its last component is absent -- not just
# that component -- and an entry another rule generates does not exist when this runs, possibly
# several directories deep. Resolving only the parent would close one level and leave the rest
# disagreeing with the CLI, which resolves at BUILD time when the file is there. A build tree
# below macOS's /var -> /private/var makes this ordinary rather than exotic.
set(_probe "${proto_abs}")
set(_missing_tail "")
while(NOT EXISTS "${_probe}")
get_filename_component(_parent "${_probe}" DIRECTORY)
if(_parent STREQUAL "${_probe}")
break() # walked up to the root without finding anything that exists
endif()
get_filename_component(_missing_name "${_probe}" NAME)
if(_missing_tail STREQUAL "")
set(_missing_tail "${_missing_name}")
else()
set(_missing_tail "${_missing_name}/${_missing_tail}")
endif()
set(_probe "${_parent}")
endwhile()
get_filename_component(_probe_real "${_probe}" REALPATH)
if(_missing_tail STREQUAL "")
set(_proto_real "${_probe_real}")
else()
set(_proto_real "${_probe_real}/${_missing_tail}")
endif()
set(_rel "")
foreach(_dir IN LISTS import_dirs_abs)
get_filename_component(_dir_real "${_dir}" REALPATH)
Expand All @@ -29,7 +62,7 @@ function(_rapidproto_output_header out_var proto_abs ext out_dir import_dirs_abs
endif()
endforeach()
if(_rel STREQUAL "")
get_filename_component(_rel "${_proto_real}" NAME)
get_filename_component(_rel "${proto_abs}" NAME) # as GIVEN -- see above
endif()
string(REGEX REPLACE "\\.proto$" "" _rel "${_rel}")
set(${out_var} "${out_dir}/${_rel}${ext}" PARENT_SCOPE)
Expand Down
127 changes: 127 additions & 0 deletions tests/check_generate_names.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
#!/usr/bin/env bash
#
# The CMake helper must predict the header path the CLI actually writes.
#
# `rapidproto_generate()` declares those paths as a custom command's OUTPUT, so a disagreement is not
# a cosmetic difference: the declared output is never created, and the target regenerates on every
# build forever (Ninja does not even error). The two rules live in different languages --
# `_rapidproto_output_header` in cmake/rapidproto-generate.cmake, `canonical_entry_name` +
# `header_path` in the CLI -- so nothing but this check keeps them in step. They have drifted once:
# the helper resolved symlinks for its fallback while the CLI kept the spelling it was given, so a
# symlinked entry whose link name differed from its target's silently rebuilt forever.
#
# Each case below is a shape where the two rules could diverge. The check runs the real CLI, sees
# which header appeared, asks the real helper what it predicted, and compares.
#
# tests/check_generate_names.sh <rapidprotoc>
#
set -uo pipefail
ROOT="$(cd "$(dirname "$0")/.." && pwd)"
BIN="${1:-$ROOT/build/gcc/rapidprotoc}"
HELPER="$ROOT/cmake/rapidproto-generate.cmake"

if [[ ! -x "$BIN" ]]; then
echo ">> $BIN is not executable (build rapidprotoc first)"; exit 1
fi
if ! command -v cmake >/dev/null 2>&1; then
echo ">> cmake not found"; exit 1
fi

WORK="$(mktemp -d "$ROOT/build/generate-names.XXXXXX")" || { echo ">> cannot create a work dir"; exit 1; }
trap 'rm -rf "$WORK"' EXIT

mkdir -p "$WORK/real" "$WORK/link" "$WORK/nested/sub"
printf 'syntax = "proto3";\nmessage Aaa { int32 x = 1; }\n' >"$WORK/real/aaa.proto"
printf 'syntax = "proto3";\nmessage Sub { int32 x = 1; }\n' >"$WORK/nested/sub/deep.proto"
ln -s ../real/aaa.proto "$WORK/link/alias.proto" # link name differs from its target's
ln -s aaa.proto "$WORK/real/sibling.proto" # link beside its target, under the import dir
ln -sfn nested "$WORK/nesteddir_link" # a symlinked import DIRECTORY
printf 'syntax = "proto3";\nmessage Dotted { int32 x = 1; }\n' >"$WORK/nested/a.proto.proto"

# case = <proto path>|<import dirs, space-separated>[|generated]. Paths are relative to $WORK and
# passed absolute, as rapidproto_generate does. `generated` means the entry does not exist when the
# helper predicts and is created before the CLI runs -- the shape of a .proto emitted by another
# build rule, where the helper sees a path CMake cannot resolve yet, and the only way to observe
# that REALPATH leaves an absent path unresolved while the CLI resolves it at build time.
#
# Every case must be able to FAIL. A symlinked import dir with a top-level entry cannot: the right
# answer and the fallback answer are both the basename, so that fixture is one directory deep.
cases=(
"real/aaa.proto|real" # plain: entry under its import dir
"nested/sub/deep.proto|nested" # under the import dir, in a subdirectory
"link/alias.proto|link" # symlinked entry, target OUTSIDE the dir -> fallback
"real/sibling.proto|real" # symlinked entry resolving INSIDE the import dir
"nesteddir_link/sub/deep.proto|nesteddir_link" # symlinked import dir, entry one level down
"real/aaa.proto|nested" # entry under an import dir that does not hold it
"nesteddir_link/sub/gen.proto|nesteddir_link|generated" # not on disk when the helper predicts
"nesteddir_link/deep/x/gen2.proto|nesteddir_link|generated" # ...with whole directories missing
# Two overlapping import dirs: the rule is FIRST match in -I order, so these two cases differ only
# in that order and pin both it and the loop's early exit. Without them a helper that scanned to
# the last match, or dropped the break, stayed green.
"nested/sub/deep.proto|nested nested/sub"
"nested/sub/deep.proto|nested/sub nested"
"nested/a.proto.proto|nested" # ".proto" mid-name: only the trailing one is stripped
)

fail=0
for case in "${cases[@]}"; do
IFS='|' read -r rel_proto rel_import mode <<<"$case"
proto="$WORK/$rel_proto"
mode="${mode:-}"
# One or more import dirs, in -I order; the helper takes them as a CMake list.
imports=()
for dir in $rel_import; do imports+=("$WORK/$dir"); done
cli_includes=()
for dir in "${imports[@]}"; do cli_includes+=(-I "$dir"); done
helper_dirs=$(printf '%s;' "${imports[@]}"); helper_dirs="${helper_dirs%;}"

# For a generated entry the helper must predict BEFORE the file exists, which is the whole point:
# CMake resolves paths at configure time and the CLI at build time. Its DIRECTORIES must be absent
# too -- REALPATH gives up on the whole path at the first missing component, so a case whose
# parents already exist cannot see a helper that only resolves one level up.
if [[ "$mode" == generated ]]; then
rm -f "$proto"
rmdir -p --ignore-fail-on-non-empty "$(dirname "$proto")" 2>/dev/null || true
fi

cat >"$WORK/predict.cmake" <<CMAKE
include("$HELPER")
_rapidproto_output_header(h "$proto" ".rp.hpp" "" "$helper_dirs")
message("\${h}")
CMAKE
if ! predicted=$(cmake -P "$WORK/predict.cmake" 2>&1); then
echo ">> the helper errored on $rel_proto:"; tail -3 <<<"$predicted"; fail=1; continue
fi
predicted="${predicted#/}" # out_dir is empty above, so the result starts with the separator

if [[ "$mode" == generated ]]; then
mkdir -p "$(dirname "$proto")"
printf 'syntax = "proto3";\nmessage Gen { int32 x = 1; }\n' >"$proto"
fi

out="$WORK/out"
rm -rf "$out"
if ! cli_log=$("$BIN" --arena "${cli_includes[@]}" --out-dir "$out" "$proto" 2>&1); then
echo ">> the CLI failed on $rel_proto:"; tail -3 <<<"$cli_log"; fail=1; continue
fi
# The decoder header, relative to the out-dir. The CLI writes one per file in the closure, so
# every case here uses an entry with no imports; the headers under rapidproto/ are the runtime it
# also drops, never decoders.
written=$(cd "$out" 2>/dev/null && find . -name '*.rp.hpp' -not -path './rapidproto/*' | sed 's|^\./||')
if [[ -z "$written" ]]; then
echo ">> the CLI wrote no decoder header for $rel_proto"; fail=1; continue
fi
if [[ $(wc -l <<<"$written") -ne 1 ]]; then
echo ">> $rel_proto produced more than one decoder header; this check compares exactly one:"
sed 's/^/ /' <<<"$written"; fail=1; continue
fi

if [[ "$written" != "$predicted" ]]; then
echo ">> $rel_proto (-I $rel_import): the CLI wrote '$written', the helper predicted '$predicted'"
echo " A custom command declaring that output would never see it produced."
fail=1
fi
done

[[ $fail -eq 0 ]] || exit 1
echo "generate names: ${#cases[@]} entry shapes, helper prediction matches the CLI"
Loading