Consolidate markdown formatting onto rumdl/flint, drop spotless prettier#19183
Conversation
There was a problem hiding this comment.
Pull request overview
This PR consolidates Markdown linting/format enforcement onto the existing CI-enforced flint/rumdl toolchain by enabling rumdl’s table cell alignment rule and removing the Gradle Spotless+prettier Markdown formatting block.
Changes:
- Enable rumdl rule MD060 (
table-cell-alignment) viaextend-enablein.github/config/.rumdl.toml. - Remove the root-project Spotless
format("markdown")configuration that applied prettier to**/*.md.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| conventions/src/main/kotlin/otel.spotless-conventions.gradle.kts | Removes Spotless Markdown formatting (prettier) so Markdown is no longer formatted via Gradle Spotless. |
| .github/config/.rumdl.toml | Explicitly enables rumdl MD060 to catch misaligned Markdown table columns via the flint/rumdl lint pipeline. |
|
@trask was quite surprising that rumdl (via flint) did not fix markdown https://rumdl.dev/rules/ has many options BTW |
|
not working on Windows: |
Thanks for reporting! I'll fix it in flint and then report here. |
|
The only reason I'd be supportive of this is because currently markdown formatting with spotless is super slow. For me |
…es checks (#390) ## Summary Found by @trask on Windows: open-telemetry/opentelemetry-java-instrumentation#19183 (comment) ``` $ mise run lint:fix [lint:fix] $ flint run --fix Error: git check-attr Caused by: The filename or extension is too long. (os error 206) ``` Two fixes for the same underlying bug class — passing many/long file paths as CLI args with no (or an inadequate) length cap, overflowing Windows' command-line length limits. ### 1. `git check-attr` (internal git-plumbing helper) - `generated_paths()` passed candidate paths as CLI args to `git check-attr`, batched at 256 per invocation. On repos with many/long paths (e.g. `opentelemetry-java-instrumentation`, a deeply-nested Java monorepo) a batch can total tens of KB, exceeding Windows' ~32KB `CreateProcess` command-line limit (os error 206, `ERROR_FILENAME_EXCED_RANGE` — yes, that's the real Win32 constant's actual spelling; added a `_typos.toml` entry so `typos` stops "fixing" it to "EXCEED"). - Switches to `git check-attr --stdin -z`, writing paths to the child's stdin instead of argv — stdin has no OS command-line-length ceiling, so batching is no longer needed. Paths are written on a separate thread concurrently with reading stdout/stderr, since output can fill the OS pipe buffer before all paths are written, which would deadlock if done sequentially. ### 2. `Scope::Files` check invocations (general check-execution path) While investigating, found the same bug class in `build_invocations()` — worse, actually, since it had **no** length cap at all (not even a fixed batch count): - `{FILES}`/`{RELFILES}` was substituted with every matched file joined into one command. Many `Scope::Files` checks are mise shims routed through `cmd.exe /C` by `spawn_command`, and `cmd.exe`'s own command-line buffer caps at **~8191 chars** — tighter than `CreateProcessW`'s ~32KB. - `google-java-format` (registered via `Check::files`, no `full_cmd` fallback) is directly exposed: `opentelemetry-java-instrumentation` has thousands of `.java` files with long nested paths, so a PR touching as few as ~50 matching files could already overflow the limit. `ryl`, `zizmor`, `xmllint`, `typos`, and `editorconfig-checker` are similarly exposed. `ktlint`/`dotnet-format` have a `full_cmd` fallback, but it only applies to whole-project ("full") runs, not the common PR-scoped changed-files case. - Adds `chunk_files_by_length()`, splitting matched files into groups that keep the rendered `{FILES}`/`{RELFILES}` argument under a conservative 6000-char cap, issuing multiple invocations instead of one unbounded one. `run_invocations()` already loops over a list of invocations and combines their output, so no changes were needed there. ## Test plan - [x] `check-attr`: regression test with long-enough paths that a single 256-path argv batch would total ~56KB (verified mathematically, comfortable margin over the 32KB limit) - [x] `Scope::Files`: regression test constructing a check with enough long-named files that a single invocation would exceed the cap, verifying `build_invocations` splits into multiple invocations that together cover every matched file exactly once - [x] Both: this crate's own CI matrix (`.github/workflows/test.yml`) already runs `cargo test` on `windows-2025`, so these tests exercise the real failure mode there — can't reproduce the literal Windows OS error from a Linux sandbox, so margins were verified by calculation rather than direct repro - [x] `cargo build`, `cargo test` (272/273 pass; the one failure is a pre-existing environment gap — missing linter binaries on PATH in this sandbox, unrelated) - [x] `cargo fmt --check`, `cargo clippy --all-targets -- -D warnings` - [x] `mise exec -- typos .` clean --------- Signed-off-by: Gregor Zeitlinger <[email protected]>
Being fast is the main goal of flint 😄 |
|
This PR has review comments. Review suggestions, whether from maintainers or automated reviewers, aren't always correct or required. Please evaluate each comment on its merits, then make sure each thread has a clear outcome. For example, link to the commit if you applied a suggestion, explain why it wasn't applied, or ask a follow-up question. Automation flags a PR for human review once every review thread has a reply or is marked as resolved. Status across open PRs is visible on the pull request dashboard. |
@trask this is fixed in flint - and the new flint is included in this PR. Can you try again? |
This comment was marked as outdated.
This comment was marked as outdated.
Pass Lychee input paths through `--files-from -` and stream them over stdin instead of appending every path to the process command line. This avoids Windows `os error 206` in repositories with large file lists while preserving the existing filtering, remapping, and cache behavior. Adds regression coverage proving an input list larger than the Windows command-line limit remains outside argv. Found while testing [open-telemetry/opentelemetry-java-instrumentation#19183](open-telemetry/opentelemetry-java-instrumentation#19183 (comment)). @zeitlinger heads up this this was entirely AI written, I didn't even look at the code, but it supposedly built and tested the code with your PR and is working on Windows now. --------- Signed-off-by: Trask Stalnaker <[email protected]> Signed-off-by: Gregor Zeitlinger <[email protected]> Co-authored-by: Gregor Zeitlinger <[email protected]>
5880e72 to
8a120b4
Compare
MD060 is off by default in rumdl. Enable it via extend-enable so misaligned markdown table columns are caught by the existing flint/ rumdl CI lint job, not just by spotless's prettier markdown formatting. Signed-off-by: Gregor Zeitlinger <[email protected]>
Rumdl now covers the same gap (misaligned markdown tables) via MD060, enabled in open-telemetry#19181 — see that PR for the analysis showing the full markdown corpus already passes cleanly under rumdl/flint with MD060 enabled, with fixer output identical to prettier's for the motivating case. Drops the Node/prettier dependency from the Gradle build in favor of the single flint/rumdl toolchain already enforced in CI via the lint-check job. This reverts the otel.spotless-conventions.gradle.kts change from 611e2e5. It deliberately does NOT revert the markdown file content changes from that PR — those stay, since spotless's prettier formatting already brought them in line with what rumdl/flint expects. Signed-off-by: Gregor Zeitlinger <[email protected]>
Signed-off-by: Gregor Zeitlinger <[email protected]>
Signed-off-by: Gregor Zeitlinger <[email protected]>
Signed-off-by: Gregor Zeitlinger <[email protected]>
Signed-off-by: Gregor Zeitlinger <[email protected]>
8a120b4 to
1ab6a42
Compare
Also note flint checks links, not just formatting. Signed-off-by: Gregor Zeitlinger <[email protected]>
| # MD060 (table-cell-alignment) | ||
| extend-enable = ["MD060"] |
There was a problem hiding this comment.
(pending grafana/flint#404 to improve perf issue on Windows)
There was a problem hiding this comment.
do you want to have a release right away?
There was a problem hiding this comment.
maybe it's just bad for me in this PR because this PR branch because it updates flint itself and so doesn't use modified files?
| [flint](https://github.com/grafana/flint), run via [mise](https://mise.jdx.dev/): | ||
|
|
||
| ```bash | ||
| mise run lint:fix |
There was a problem hiding this comment.
I think I noticed this runs link check, even though non-fixable, which also makes it slow?
if that can't be changed, could we have a command that only does markdown formatting, which is what is generally needed for contributors
There was a problem hiding this comment.
it only checks links in modified files (and also uses the lychee cache in local mode) - so it should not be an issue.
That being said flint run --fix rumdl would only run markdown check on modified files.
There was a problem hiding this comment.
it only checks links in modified files
ah probably takes a long time in this PR branch because it updates flint itself?
There was a problem hiding this comment.
Yes - that causes a full run
Removed expired SSL certificate check for GWT project.
Summary
MD060(table-cell-alignment) viaextend-enable— off by default in rumdl, and the specific gap that motivated Add prettier to spotless #18952 (a misaligned table nobody caught: Add Couchbase 3.x stable db semconv support #18926 (comment)). Usesextend-enable, notenable—enableis an allowlist ("only these rules") and would have silently disabled every other rumdl rule.format("markdown")spotless block added in Add prettier to spotless #18952, which used prettier for the same purpose.Rationale:
flint/rumdlalready runs in CI on every PR (build / lint / lint-check), so this consolidates onto a toolchain already enforced rather than introducing a new one.CHANGELOG.mdwhichflint.tomlalready excludes from all lint checks) passes cleanly under rumdl/flint withMD060enabled and the existing.rumdl.tomldisable list otherwise unchanged — no cleanup backlog.rumdl --fixproduces output byte-identical to prettier's canonical formatting for the motivating misaligned-table case..rumdl.toml(line-length, no-inline-html, etc.) are unrelated stylistic opt-outs (intentional long prose lines, intentional<details>blocks, etc.) — not things prettier was silently covering for.Does not revert the markdown content changes from #18952 (the ~60 files it reformatted) — those stay, since they already satisfy rumdl/flint's rules and reverting them would just reintroduce inconsistent formatting with no corresponding tooling change.
Net effect: drops the Node/prettier dependency from the Gradle build in favor of the single flint/rumdl toolchain already enforced in CI.
Test plan
flint runon the full repo — 0 violationsrumdl check --fixon a known-misaligned table — output matches prettier's canonical form exactly./gradlew spotlessCheck— passes (no markdown target left to fail)./gradlew tasks --all— confirmsspotlessMarkdown*tasks are gone,spotlessMisc*unaffected