FPP Formatting - #336
Conversation
| "gcovr>=6.0", | ||
| "urllib3>=2.5.0", | ||
| "requests>=2.31.0", | ||
| "fprime-fpp-format", |
There was a problem hiding this comment.
[Design] could fix New runtime dependency is unpinned and untracked by version-check.
Every other F Prime pip package is version-tracked via FPRIME_PIP_PACKAGES and pinned in the project requirements, because the FPP tooling must match the FPP grammar version in use. A floating fprime-fpp-format can format sources with a different grammar than the compiler in the same environment, and fprime-util version-check will not report it.
There was a problem hiding this comment.
[Supply Chain] Concur — also in scope for dependency pinning: fprime-fpp-format is the only entry in dependencies with no version specifier at all, so a resolver may pick any of its stable releases back to 2.0.0 (or any future major) on a fresh pip install fprime-tools; a floor such as fprime-fpp-format>=3.3.2 matches this repo's convention for every other dependency; severity from my scope: suggestion.
| for formatter in formatters: | ||
| for allowed_ext in formatter.allowed_extensions: | ||
| for file in dir_path.rglob(f"*{allowed_ext}"): | ||
| formatter.stage_file(file) |
There was a problem hiding this comment.
[Design] must fix --dirs silently changes behavior for existing users; not acknowledged in the description.
fprime-util format --dirs Svc Fw Drv previously touched C/C++ only. It now rewrites every reachable .fpp (plus .fppi via --recursive-includes) in place, and returns 1 when fpp-format is absent — so existing invocations that succeeded now fail, and existing --check gates start failing on FPP files that were never formatted before. Please state the change and the migration in the PR description, or gate FPP directory scanning behind an opt-in flag for this release.
| When '.fpp' files are staged, fpp-format is run with '--recursive-includes' so that '.fppi' include fragments reachable from those files are also formatted with the correct entrypoint rule. Bare '.fppi' fragments are not staged directly, as they carry no context to infer their grammar rule from. | ||
|
|
||
| Note: '{EXECUTABLE} format' requires that the 'clang-format' utility is installed and in the PATH. | ||
| Note: '{EXECUTABLE} format' requires that the 'clang-format' utility is installed and in the PATH to format C/C++ files, and the 'fpp-format' utility (from the 'fprime-fpp-format' package) to format FPP files. Each is only required when files of the corresponding type are staged. |
There was a problem hiding this comment.
[Design] suggestion Document that --pass-through and --allow-extension apply to clang-format only.
run_code_format forwards parsed.pass_through to clang-format and passes ({}, []) to fpp-format, and only clang-format receives --allow-extension values. Users invoking --check --dirs on a mixed tree will otherwise expect their extra flags to reach fpp-format.
| Note: '{EXECUTABLE} format' requires that the 'clang-format' utility is installed and in the PATH to format C/C++ files, and the 'fpp-format' utility (from the 'fprime-fpp-format' package) to format FPP files. Each is only required when files of the corresponding type are staged. | |
| Note: '{EXECUTABLE} format' requires that the 'clang-format' utility is installed and in the PATH to format C/C++ files, and the 'fpp-format' utility (from the 'fprime-fpp-format' package) to format FPP files. Each is only required when files of the corresponding type are staged. The '--pass-through' and '--allow-extension' options apply to clang-format only. |
There was a problem hiding this comment.
[Documentation] Concur — also in scope for documentation currency: --force additionally widens routing (it clears validate_extensions for both formatters), and the trailing More information at ... line still points only at clang-format's docs.
| # Gather explicitly listed files (from --files and, if requested, stdin) and | ||
| # stage each into every formatter. Each formatter's stage_file() filters by | ||
| # its own allowed extensions, so files route themselves to the right tool. |
There was a problem hiding this comment.
[Maintainability] must fix maint-misleading-comment: the routing invariant this comment states does not hold under --force.
--force sets validate_extensions=False for both formatters (line 181), so
stage_file() stops filtering and every explicitly listed file is staged into
both clang-format and fpp-format. A reader trusting this comment will not
expect fprime-util format --force -f model.fpp to also hand the file to
clang-format.
| # Gather explicitly listed files (from --files and, if requested, stdin) and | |
| # stage each into every formatter. Each formatter's stage_file() filters by | |
| # its own allowed extensions, so files route themselves to the right tool. | |
| # Gather explicitly listed files (from --files and, if requested, stdin) and | |
| # stage each into every formatter. Extension validation normally routes each | |
| # file to the right tool; with --force it is disabled, so every listed file | |
| # is staged into both formatters. |
| return status.returncode | ||
|
|
||
|
|
||
| class FppFormatter(ExecutableAction): |
There was a problem hiding this comment.
[Maintainability] could fix maint-code-duplication: FppFormatter duplicates ~60 lines of ClangFormatter verbatim.
__init__ (minus style_file), is_supported, allow_extension,
stage_file, exclude_file and the backup loop are byte-identical apart from
the "C/C++"/"FPP" wording. Any fix to staging or exclusion (e.g. case-insensitive
suffix matching, symlink handling) must now be applied twice, and the copies will
drift. Extracting the shared staging/exclusion logic into a small base class in
this file and leaving only execute() plus the extension list per subclass
removes the duplicate surface.
| def allow_extension(self, file_ext: str) -> None: | ||
| """Add a file extension str to the list of allowed extension""" | ||
| self.allowed_extensions.append(file_ext) |
There was a problem hiding this comment.
[Maintainability] could fix maint-dead-code: FppFormatter.allow_extension() has no caller.
run_code_format routes --allow-extension to the clang formatter only, and the
FPP_ALLOWED_EXTENSIONS comment states the FPP set is deliberately fixed. Leaving
a public mutator that nothing calls invites a future caller to use it and silently
break the .fppi include-following contract. Either drop it or document that it
is intentionally unused.
cc @LeStarch @thomas-bc — low-confidence finding, please confirm.
| if self.verbose: | ||
| print( | ||
| f"[INFO] Skipping {filepath} : unrecognized FPP file extension " | ||
| f"('{filepath.suffix}'). Use --allow-extension or --force." |
There was a problem hiding this comment.
[Maintainability] suggestion maint-misleading-comment: this skip message advertises a flag that has no effect here.
--allow-extension is applied to the clang formatter only (run_code_format),
so a user following this hint on a .fppX file will see the file skipped again
with no explanation.
| f"('{filepath.suffix}'). Use --allow-extension or --force." | |
| f"('{filepath.suffix}'). Use --force to format it anyway." |
There was a problem hiding this comment.
[Documentation] Concur — also in scope for documentation currency: this user-facing message is the only guidance a user gets here, and --force is the sole flag that actually applies to FPP staging.
| returncode |= clang_formatter.execute( | ||
| build, parsed.path, ({}, parsed.pass_through) | ||
| ) | ||
| if fpp_formatter._files_to_format: | ||
| if not fpp_formatter.is_supported(): | ||
| print( | ||
| f"[ERROR] Cannot find executable: {fpp_formatter.executable}. " | ||
| "Install the 'fprime-fpp-format' package to format FPP files.", | ||
| file=sys.stderr, | ||
| ) | ||
| return 1 | ||
| returncode |= fpp_formatter.execute(build, parsed.path, ({}, [])) |
There was a problem hiding this comment.
[Maintainability] suggestion maint-clever-code: bitwise-OR of exit codes fabricates a status nobody returned.
1 | 2 == 3, so a clang failure plus an fpp failure exits with a code neither
tool produced, and codes above 255 wrap when the shell sees them. Keeping the
first non-zero status preserves a meaningful exit code as more formatters are
added.
| returncode |= clang_formatter.execute( | |
| build, parsed.path, ({}, parsed.pass_through) | |
| ) | |
| if fpp_formatter._files_to_format: | |
| if not fpp_formatter.is_supported(): | |
| print( | |
| f"[ERROR] Cannot find executable: {fpp_formatter.executable}. " | |
| "Install the 'fprime-fpp-format' package to format FPP files.", | |
| file=sys.stderr, | |
| ) | |
| return 1 | |
| returncode |= fpp_formatter.execute(build, parsed.path, ({}, [])) | |
| returncode = clang_formatter.execute( | |
| build, parsed.path, ({}, parsed.pass_through) | |
| ) | |
| if fpp_formatter._files_to_format: | |
| if not fpp_formatter.is_supported(): | |
| print( | |
| f"[ERROR] Cannot find executable: {fpp_formatter.executable}. " | |
| "Install the 'fprime-fpp-format' package to format FPP files.", | |
| file=sys.stderr, | |
| ) | |
| return 1 | |
| returncode = returncode or fpp_formatter.execute(build, parsed.path, ({}, [])) |
There was a problem hiding this comment.
[Correctness] Concur — also in scope for functional correctness: a clang-format status of 1 OR'd with an fpp-format status of 2 exits 3, and a signal-killed child's negative returncode propagates through sys.exit() as an unrelated status.
| for filename in parsed.files: | ||
| clang_formatter.stage_file(Path(filename)) | ||
| # Search for files within --dirs and stage them | ||
| explicit_files.extend(Path(name) for name in sys.stdin.read().split()) |
There was a problem hiding this comment.
[Maintainability] suggestion maint-inconsistent-local-convention: explicit_files holds str from --files and Path from stdin.
The mixed element type is re-wrapped by Path(filename) two lines below, so the
conversion here is redundant and leaves the next reader unsure what the list
contains.
| explicit_files.extend(Path(name) for name in sys.stdin.read().split()) | |
| explicit_files.extend(sys.stdin.read().split()) |
| # Run each formatter that has files staged. --pass-through is forwarded only | ||
| # to clang-format (its historical target); fpp-format receives no extra args. | ||
| returncode = 0 | ||
| if clang_formatter._files_to_format: |
There was a problem hiding this comment.
[Maintainability] could fix maint-inconsistent-local-convention: reaching into the formatters' private _files_to_format from commands.py.
The formatter classes expose stage_file/exclude_file/is_supported as their
public interface; poking the underscore attribute from another module means a
rename inside code_formatter.py breaks this caller with no signal. A small
public predicate (e.g. has_staged_files()) on the shared formatter surface keeps
the encapsulation intact at both call sites.
| '{EXECUTABLE} format' formats source files, routing each file to the appropriate formatter by extension: C/C++ files ('cpp', 'c++', 'cxx', 'cc', 'c', and their 'h' equivalents) are formatted with 'clang-format', and FPP files ('.fpp') are formatted with 'fpp-format'. clang-format uses the style specified in the .clang-format file found at the root of the F' framework used by the project (i.e. the 'framework_path' specified in settings.ini). | ||
| Files are specified through stdin or by using the '--files [<path/to/file>]*' flag. When reading from stdin, file paths should be separated by whitespace characters. | ||
| Because clang-format will try to format any text file it is fed, '{EXECUTABLE} format' restricts by default the files that are processed to commonly used C/C++ file extensions (cpp, c++, cxx, cc, c, and their 'h' equivalents). Backup copies of the formatted files are also created by default. | ||
| Because clang-format will try to format any text file it is fed, '{EXECUTABLE} format' restricts by default the files that are processed to commonly used C/C++ file extensions. Backup copies of the formatted files are also created by default. |
There was a problem hiding this comment.
[Documentation] suggestion Rewritten help line still claims backups are made by default.
--backup is action="store_true" (default False) in cli.py, and both formatters gate backups on options.get("backup", False), so no backup is written unless the user passes --backup. The PR touched this line, so the stale claim now ships in the updated FPP-aware help text.
| Because clang-format will try to format any text file it is fed, '{EXECUTABLE} format' restricts by default the files that are processed to commonly used C/C++ file extensions. Backup copies of the formatted files are also created by default. | |
| Because clang-format will try to format any text file it is fed, '{EXECUTABLE} format' restricts by default the files that are processed to commonly used C/C++ file extensions. Backup copies of the formatted files can be created with the '--backup' flag. |
|
|
||
| # Allow requested file extensions (clang-format only; fpp-format has a fixed | ||
| # extension set driven by its include-following behavior) | ||
| for file_ext in parsed.allow_extension: |
There was a problem hiding this comment.
[Documentation] could fix cli.py flag help for the format subcommand is now inaccurate. (Anchored here; src/fprime/util/cli.py is not in this diff.)
--force reads "Force all listed files to be passed to clang-format (no file extension check)", but it now sets validate_extensions=False for both formatters, so listed files also go to fpp-format. --allow-extension ("Add a file extension to the allowed set") and -q/--quiet ("Disable clang-format verbose mode") are now clang-format-only and should say so. Suggested wording: --force -> "Pass all listed files to the formatters (no file extension check)"; --allow-extension -> "Add a file extension to the clang-format allowed set".
| Generate a new F' subtopology. This command prompts for the name of the subtopology, and outputs a folder containing the structure for one. The user can then add the subtopology to their own project depending on where the subtopology is generated. | ||
| """, | ||
| "format": f"""Format C/C++ files using clang-format | ||
| "format": f"""Format C/C++ files using clang-format and FPP files using fpp-format |
There was a problem hiding this comment.
[Documentation] suggestion fprime-util format now formats FPP, which makes nasa/fprime's contributor docs stale.
nasa/fprime CONTRIBUTING.md ("Code Style", ~L84) states the repository "enforces formatting with clang-format" and shows fprime-util format --stdin / --dirs as C++-only workflows; after this PR those same commands rewrite .fpp (and, via --recursive-includes, .fppi) files too. Related: nasa/fprime .github/workflows/format-check.yml runs fprime-util format --check --dirs ... under a job named "C++ Formatting", which will start gating FPP formatting. Cross-repo, so not one-click applicable here; proposed CONTRIBUTING wording:
The F´ repository enforces formatting with `clang-format` for C/C++ and `fpp-format` for FPP models.cc @LeStarch @thomas-bc — low-confidence finding, please confirm.
| returncode |= clang_formatter.execute( | ||
| build, parsed.path, ({}, parsed.pass_through) | ||
| ) | ||
| if fpp_formatter._files_to_format: |
There was a problem hiding this comment.
[Test Quality] must fix The new formatter-routing logic in run_code_format has no test coverage.
The only test for this entry point is test_format_command in test/fprime/util/commands_unit_test.py, whose body is a bare pass. Nothing exercises extension-based routing, the new per-formatter is_supported gate (a project with only C/C++ files must no longer fail when fpp-format is absent, and vice versa), or the combined returncode. Please add a test that drives run_code_format with a stub parsed/build and monkeypatched formatters and asserts which formatter ran and what code was returned.
| "validate_extensions": True, | ||
| } | ||
| formatter = FppFormatter("fpp-format", options) | ||
| assert formatter is not None |
There was a problem hiding this comment.
[Test Quality] must fix test_fpp_init cannot fail: a constructor call never returns None.
The option plumbing it appears to cover (check, backup, the copied extension list) is left unverified.
| assert formatter is not None | |
| assert formatter.executable == "fpp-format" | |
| assert formatter.check is False | |
| assert formatter.backup is True | |
| assert formatter.allowed_extensions == [".fpp"] |
| """Test that execute is a no-op (success) when no files are staged""" | ||
| formatter = FppFormatter("fpp-format", {}) | ||
| result = formatter.execute(mock_build, tmp_path, ({}, [])) | ||
| assert result == 0 |
There was a problem hiding this comment.
[Test Quality] suggestion No test covers --recursive-includes, the behavior that justifies not staging .fppi files.
.fppi fragments are deliberately excluded from staging on the premise that they are reached through include specifiers; nothing verifies that a fragment reachable from a staged .fpp is actually reformatted. Expected values below were checked against fprime-fpp-format 3.3.2.
| assert result == 0 | |
| assert result == 0 | |
| @requires_fpp_format | |
| def test_fpp_execute_recursive_includes(tmp_path, mock_build): | |
| """Test that .fppi fragments reachable from a staged .fpp are formatted""" | |
| fragment = tmp_path / "fragment.fppi" | |
| fragment.write_text(" constant z = 3\n") | |
| root = tmp_path / "model.fpp" | |
| root.write_text('module M {\n include "fragment.fppi"\n}\n') | |
| formatter = FppFormatter("fpp-format", {"check": False, "quiet": True}) | |
| formatter.stage_file(root) | |
| assert formatter.execute(mock_build, tmp_path, ({}, [])) == 0 | |
| assert fragment.read_text() == "constant z = 3\n" |
|
|
||
| # End-to-end tests need the fpp-format executable (from fprime-fpp-format). | ||
| requires_fpp_format = pytest.mark.skipif( | ||
| shutil.which("fpp-format") is None, |
There was a problem hiding this comment.
[Test Quality] could fix This guard can silently skip every behavioral assertion added by the PR.
fprime-fpp-format is now a hard dependency in pyproject.toml (and ships fpp-format on PATH), so the executable is present wherever the package installed successfully. With the skip in place, an environment where it did not install turns the four end-to-end tests into silent no-ops rather than a failure — the remaining tests only cover staging bookkeeping.
| formatter = FppFormatter("fpp-format", options) | ||
| formatter.stage_file(malformed_dst) | ||
|
|
||
| result = formatter.execute(mock_build, tmp_path, ({}, [])) |
There was a problem hiding this comment.
[Test Quality] suggestion WELL_FORMED_FPP depends on config discovered outside the test.
fpp-format reads indent width and line length from the nearest .fpp-format file, searched upward from each file's directory — from tmp_path that search escapes the test tree, so the expected output is environment-dependent. Passing the widths explicitly pins it and incidentally covers pass-through forwarding.
| result = formatter.execute(mock_build, tmp_path, ({}, [])) | |
| result = formatter.execute( | |
| mock_build, tmp_path, ({}, ["--indent", "2", "--line-length", "80"]) | |
| ) |
| self.executable, | ||
| # Follow include specifiers so reachable .fppi fragments are | ||
| # formatted with the correct context-derived entrypoint rule. | ||
| "--recursive-includes", |
There was a problem hiding this comment.
[Security] must fix --recursive-includes lets in-place formatting escape the staged file set.
include specifiers resolve relative to the including file, so a staged .fpp can rewrite fragments outside the requested scope (verified with fpp-format 3.3.2: include "../b/frag.fppi" in a/model.fpp rewrote a/../b/frag.fppi). Those writes bypass --exclude (which only filters _files_to_format) and the backup loop above, so excluded/untargeted files are modified with no .bak. Restrict include-following to fragments under the staged roots, or back up and report every file fpp-format touches.
There was a problem hiding this comment.
interesting finding... do we care? I vote no
| "gcovr>=6.0", | ||
| "urllib3>=2.5.0", | ||
| "requests>=2.31.0", | ||
| "fprime-fpp-format", |
There was a problem hiding this comment.
[Supply Chain] could fix New mandatory install-time dependency ships prebuilt binaries for only three platforms.
fprime-fpp-format publishes wheels for macosx_11_0_arm64, manylinux2014_x86_64 and win_amd64 only; any other target (Linux aarch64, musl, macOS x86_64) falls back to the sdist, a maturin/Rust build that compiles and fetches crates at install time — a new build-time execution surface on every pip install fprime-tools, including CI. Provenance itself checks out (PyPI attestations show a trusted publisher from fprime-community/fpp-tools, workflow pip.yml), but its first non-prerelease release is one day old. An optional extra would match the code's "only required when .fpp files are staged" contract.
cc @LeStarch @thomas-bc — low-confidence finding, please confirm.
| return clang_formatter.execute(build, parsed.path, ({}, parsed.pass_through)) | ||
| # Run each formatter that has files staged. --pass-through is forwarded only | ||
| # to clang-format (its historical target); fpp-format receives no extra args. | ||
| returncode = 0 |
There was a problem hiding this comment.
[Correctness] could fix correctness-other: the "nothing was formatted" message is now unreachable.
Pre-PR, ClangFormatter.execute() always ran and printed [INFO] No files were formatted.. Both execute() calls are now guarded on _files_to_format, so fprime-util format --dirs docs (a tree with no C/C++ or FPP file) prints nothing and exits 0 — the user loses the only signal that their selection matched no files.
| returncode = 0 | |
| returncode = 0 | |
| if not clang_formatter._files_to_format and not fpp_formatter._files_to_format: | |
| print("[INFO] No files were formatted.") |
Co-authored-by: lestarch-autobot <lestarch+autobot@gmail.com>
lestarch-autobot
left a comment
lestarch-autobot
left a comment
There was a problem hiding this comment.
Automated review summary (run 1)
Per-agent results
| Agent | must fix | suggestion | could fix | future work | outstanding | Verdict |
|---|---|---|---|---|---|---|
| Security Vulnerabilities | 1 | 0 | 2 | 0 | 3 | No-Go |
| Supply Chain / Runner Safety | 0 | 1 | 1 | 0 | 2 | Go |
| F Prime C/C++ Design | 0 | 0 | 0 | 0 | 0 | Go |
| Documentation Currency | 0 | 4 | 1 | 0 | 5 | Go |
| Design | 3 | 1 | 1 | 0 | 5 | No-Go |
| Architecture | 0 | 0 | 0 | 0 | 0 | Go |
| Test Quality | 2 | 2 | 2 | 0 | 6 | No-Go |
| Correctness | 1 | 0 | 3 | 0 | 4 | No-Go |
| Maintainability | 1 | 4 | 3 | 0 | 8 | No-Go |
| CI safety | — | — | — | — | — | Go |
| Totals | 8 | 12 | 11 | 0 | 31 | No-Go |
Duplicates consolidated this run: 2 (two threads closed by the de-duplication post-pass; totals count each consolidated finding once, so they are 2 lower than the column sums)
Supply-chain surfaces
| Surface | Outstanding |
|---|---|
| Dependencies | 2 (1 suggestion, 1 could-fix) — 'fprime-fpp-format' added with no version specifier |
| Vendored / submodule | clean |
| Build / test infrastructure | clean |
| Workflows / actions / scripts | clean |
| Generator output | clean |
| Prompt-injection | clean |
| Review-system integrity | clean |
Outstanding must-fix items (7)
Security Vulnerabilities
--recursive-includeslets in-place formatting escape the staged set, bypassing--excludeand the backup loop — also: Correctness — #336 (comment)
Design
- Human design adjudication required. Depends on unmerged nasa/fpp#1062, and published
fprime-fpp-formatdoes not yet ship the assumed tool — #336 (comment) - Under
--force, every listed file is staged into both formatters (.fppreaches clang-format) — also: Correctness — #336 (comment) --dirssilently changes behavior for existing users /--checkgates; not stated in the description — #336 (comment)
Test Quality
- New formatter-routing logic in
run_code_formathas no test coverage (test_format_commandis a barepass) — #336 (comment) test_fpp_initcannot fail; the option plumbing it appears to cover is unverified — #336 (comment)
Maintainability
maint-misleading-comment: the stated--forcerouting invariant does not hold — #336 (comment)
Merge readiness
Merge readiness: No-Go — 8 outstanding must-fix findings across Design (3), Test Quality (2), Security, Correctness, and Maintainability; the Design adjudication item also needs a human decision. Reviewers analyzed 39548cc; the head has since advanced to 5cf2c1a — re-run to re-verify.
Clear skies for FPP formatting once these course corrections are logged.
3 participants
Change Description
Augments
fprime-util formatto support formatting/checking formatting for FPP files using the newfpp-formattool in https://github.com/fprime-community/fpp-tools. We repurposed thefprime-fpp-formatPIP package to hold this new tool.Rationale
FPP Formatting!
Testing/Review Recommendations
This requires a deconflicting of the
fpp-formatexecutable nasa/fpp#1062 and is a tandem withFuture Work
Note any additional work that will be done relating to this issue.