diff --git a/CHANGELOG.md b/CHANGELOG.md index d5b9273..c6aca1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/check.sh b/check.sh index 2c918fd..4953fae 100755 --- a/check.sh +++ b/check.sh @@ -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 @@ -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")" @@ -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++ @@ -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: `, 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. @@ -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 } @@ -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 @@ -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 diff --git a/cmake/rapidproto-generate.cmake b/cmake/rapidproto-generate.cmake index a425d09..92a24a1 100644 --- a/cmake/rapidproto-generate.cmake +++ b/cmake/rapidproto-generate.cmake @@ -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) @@ -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) diff --git a/tests/check_generate_names.sh b/tests/check_generate_names.sh new file mode 100755 index 0000000..e51496b --- /dev/null +++ b/tests/check_generate_names.sh @@ -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 +# +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 = |[|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" <&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"