Skip to content

Consolidate markdown formatting onto rumdl/flint, drop spotless prettier#19183

Merged
trask merged 9 commits into
open-telemetry:mainfrom
zeitlinger:rumdl-md060-and-revert-spotless-prettier
Jul 16, 2026
Merged

Consolidate markdown formatting onto rumdl/flint, drop spotless prettier#19183
trask merged 9 commits into
open-telemetry:mainfrom
zeitlinger:rumdl-md060-and-revert-spotless-prettier

Conversation

@zeitlinger

Copy link
Copy Markdown
Member

Summary

  1. Enables rumdl's MD060 (table-cell-alignment) via extend-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)). Uses extend-enable, not enableenable is an allowlist ("only these rules") and would have silently disabled every other rumdl rule.
  2. Reverts the format("markdown") spotless block added in Add prettier to spotless #18952, which used prettier for the same purpose.

Rationale:

  • flint/rumdl already runs in CI on every PR (build / lint / lint-check), so this consolidates onto a toolchain already enforced rather than introducing a new one.
  • The full markdown corpus (207 files, excluding CHANGELOG.md which flint.toml already excludes from all lint checks) passes cleanly under rumdl/flint with MD060 enabled and the existing .rumdl.toml disable list otherwise unchanged — no cleanup backlog.
  • rumdl --fix produces output byte-identical to prettier's canonical formatting for the motivating misaligned-table case.
  • The other 7 rules currently disabled in .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 run on the full repo — 0 violations
  • rumdl check --fix on a known-misaligned table — output matches prettier's canonical form exactly
  • ./gradlew spotlessCheck — passes (no markdown target left to fail)
  • ./gradlew tasks --all — confirms spotlessMarkdown* tasks are gone, spotlessMisc* unaffected

@zeitlinger
zeitlinger marked this pull request as ready for review July 10, 2026 17:14
@zeitlinger
zeitlinger requested a review from a team as a code owner July 10, 2026 17:14
Copilot AI review requested due to automatic review settings July 10, 2026 17:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) via extend-enable in .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.

@zeitlinger

Copy link
Copy Markdown
Member Author

@trask was quite surprising that rumdl (via flint) did not fix markdown

https://rumdl.dev/rules/ has many options BTW

@trask

trask commented Jul 10, 2026

Copy link
Copy Markdown
Member

not working on Windows:

$ 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)
[lint:fix] ERROR task failed

@zeitlinger

Copy link
Copy Markdown
Member Author

not working on Windows:

$ 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)
[lint:fix] ERROR task failed

Thanks for reporting! I'll fix it in flint and then report here.

@zeitlinger
zeitlinger marked this pull request as draft July 11, 2026 09:00
@laurit

laurit commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

The only reason I'd be supportive of this is because currently markdown formatting with spotless is super slow. For me ./gradlew spotlessApply takes ~16m, when the markdown part is removed it runs much faster. If I had to guess the problem is probably in target("**/*.md"). Of course it would be ideal if we could make spotless do this faster.

zeitlinger added a commit to grafana/flint that referenced this pull request Jul 13, 2026
…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]>
@zeitlinger

Copy link
Copy Markdown
Member Author

The only reason I'd be supportive of this is because currently markdown formatting with spotless is super slow. For me ./gradlew spotlessApply takes ~16m, when the markdown part is removed it runs much faster. If I had to guess the problem is probably in target("**/*.md"). Of course it would be ideal if we could make spotless do this faster.

Being fast is the main goal of flint 😄

Comment thread .github/config/lychee.toml Outdated
@opentelemetry-pr-dashboard

Copy link
Copy Markdown

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.

@zeitlinger

Copy link
Copy Markdown
Member Author

not working on Windows:

$ 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)
[lint:fix] ERROR task failed

Thanks for reporting! I'll fix it in flint and then report here.

@trask this is fixed in flint - and the new flint is included in this PR. Can you try again?

@zeitlinger
zeitlinger marked this pull request as ready for review July 13, 2026 11:27
@trask

This comment was marked as outdated.

Comment thread .github/config/.rumdl.toml
@zeitlinger
zeitlinger marked this pull request as draft July 14, 2026 12:06
zeitlinger added a commit to grafana/flint that referenced this pull request Jul 14, 2026
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]>
@zeitlinger
zeitlinger marked this pull request as ready for review July 14, 2026 16:42
Comment thread .github/config/.rumdl.toml
@zeitlinger
zeitlinger force-pushed the rumdl-md060-and-revert-spotless-prettier branch from 5880e72 to 8a120b4 Compare July 15, 2026 09:19
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]>
@zeitlinger
zeitlinger force-pushed the rumdl-md060-and-revert-spotless-prettier branch from 8a120b4 to 1ab6a42 Compare July 15, 2026 09:21
Also note flint checks links, not just formatting.

Signed-off-by: Gregor Zeitlinger <[email protected]>
Comment on lines +6 to +7
# MD060 (table-cell-alignment)
extend-enable = ["MD060"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(pending grafana/flint#404 to improve perf issue on Windows)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to have a release right away?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

[flint](https://github.com/grafana/flint), run via [mise](https://mise.jdx.dev/):

```bash
mise run lint:fix

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it only checks links in modified files

ah probably takes a long time in this PR branch because it updates flint itself?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - that causes a full run

Removed expired SSL certificate check for GWT project.

@trask trask left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's give it a spin!

@trask
trask merged commit e71bd17 into open-telemetry:main Jul 16, 2026
185 of 187 checks passed
@zeitlinger
zeitlinger deleted the rumdl-md060-and-revert-spotless-prettier branch July 17, 2026 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants