Skip to content

Introduce ariadne to provide better error messages + chmod support - #13785

Open
sylvestre wants to merge 4 commits into
uutils:mainfrom
sylvestre:ariadne-support
Open

Introduce ariadne to provide better error messages + chmod support#13785
sylvestre wants to merge 4 commits into
uutils:mainfrom
sylvestre:ariadne-support

Conversation

@sylvestre

Copy link
Copy Markdown
Contributor

example:

╰╴❯ LANG=C ./target/debug/coreutils chmod "g+rw?x" a  
Error: invalid operator (expected +, -, or =, but found ?)
   ╭─[ chmod:1:5 ]
   │
 1 │ g+rw?x a
   │     ┬
   │     ╰── expected +, - or = here
   │
   │ Help: a mode is either octal, as in 644, or clauses such as u+rwx,go-w
───╯

screenshot with color:
image

more programs will follow

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

Binary size comparison:

Individual binary size comparison VS main (threshold: >=5% AND >=4 KB).

Total size of compared binaries: 147.30 MB (+380 KB, +0.25%)

Significant per-binary changes:
  [         1.09 MB ->    1.17 MB  (+80 KB, +7.17%)
  test      1.09 MB ->    1.17 MB  (+80 KB, +7.17%)
  chmod     1.14 MB ->    1.21 MB  (+76 KB, +6.53%)

@codspeed-hq

codspeed-hq Bot commented Aug 7, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 0.24%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 3 improved benchmarks
❌ 2 regressed benchmarks
✅ 342 untouched benchmarks
⏩ 46 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation numfmt_large_numbers_si[10000] 93.2 ms 102.4 ms -9.06%
Simulation numfmt_padding[(10000, 50)] 93.5 ms 97.4 ms -4.02%
Simulation ls_recursive_deep_tree[(200, 2)] 3.6 ms 3.4 ms +5.33%
Simulation cksum_default 41.4 ms 39.8 ms +4.02%
Simulation mv_multiple_to_dir 25.3 ms 24.5 ms +3.3%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing sylvestre:ariadne-support (9502c25) with main (c33da23)

Open in CodSpeed

Footnotes

  1. 46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@sylvestre
sylvestre force-pushed the ariadne-support branch 3 times, most recently from e86738c to 120499c Compare August 7, 2026 11:36
@sylvestre

Copy link
Copy Markdown
Contributor Author

@codspeedbot fix this regression

@codspeed-hq

codspeed-hq Bot commented Aug 7, 2026

Copy link
Copy Markdown

@codspeedbot fix this regression

I looked into the 6 reported regressions, and there is nothing to fix here — none of them are caused by this PR.

Why

Every single flagged benchmark was compared across different CPU models. From the run comparison metadata:

Benchmark BASE runner CPU HEAD runner CPU
true_consecutive_calls / false_consecutive_calls AMD EPYC 7763 Intel Xeon Platinum 8370C
cksum_crc, cksum_multiple_files AMD EPYC 9V74 AMD EPYC 7763
numfmt_from_si[10000] Intel Xeon 6973P-C AMD EPYC 7763
du_max_depth_balanced_tree AMD EPYC 7763 AMD EPYC 9V74
ls_recursive_* (the "improvements") AMD EPYC 9V74 AMD EPYC 7763

Simulation mode derives its cache model from the physical CPU, so a runner swap shifts the estimate on identical source. The fact that ls moved in the opposite direction (+3.4% to +7.5%) with the mirrored CPU swap is the giveaway: this is runner variability, not a code effect.

The PR cannot reach that code anyway

The benchmark workflow builds one package per matrix job (cargo codspeed build -p uu_<util>), so cargo feature unification does not leak across packages. ariadne is behind the new optional diagnostics feature of uucore, enabled only by uu_chmod; none of uu_true, uu_false, uu_cksum, uu_numfmt, uu_du or uu_ls enable diagnostics or even the mode feature, so neither ariadne nor the modified mode.rs is compiled into their bench binaries.

Verified locally

I built and ran the worst offender on both revisions on the same machine:

Benchmark main ariadne-support
true_consecutive_calls 170.28 ns 170.28 ns
false_consecutive_calls 170.28 ns 170.28 ns

Byte-for-byte identical measurements — the reported -17.38% does not reproduce.

Recommendation

No changes were made and nothing was pushed. These can be safely acknowledged on CodSpeed. Worth noting separately: du_max_depth_balanced_tree is independently known to swing ~3% on its own (the du printing thread sends one message per directory over an mpsc channel, and the futex wake-up count is non-deterministic), so it will keep appearing in reports until that is batched.

The one real, measurable cost of this PR is the binary size already reported by the size bot: chmod grows +76 KB (+6.5%) from pulling in ariadne. That is a size trade-off rather than a runtime one, and worth keeping an eye on as more utils adopt it.

Summary
  • Warming up the performance wizard...
  • Investigating the reported regressions — All flagged benchmarks were compared across different CPU models; none of them exercise code touched by this PR
  • Reproducing the flagged benchmarks locally — true/false benchmarks measure 170.28 ns on both base and PR head on identical hardware: no real regression
  • Conclusion — All 6 flagged regressions are cross-runner artifacts; no code change is required
  • Handling comment — Comment handling finished successfully

Learnings updated. See what CodSpeed remembers about this repository.

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/cp/sparse-to-pipe. tests/cp/sparse-to-pipe is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/rm/isatty (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Note: The gnu test tests/printf/printf-surprise is now being skipped but was previously passing.
Skip an intermittent issue tests/tail/tail-n0f (was skipped on 'main', now failing)

@sylvestre
sylvestre requested review from cakebaker and a lite review from Copilot August 7, 2026 17:52

Copilot AI left a comment

Copy link
Copy Markdown

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 introduces an optional diagnostics rendering facility (powered by ariadne) to produce richer, caret-pointing error reports on terminals, and wires it into chmod by upgrading mode parsing to return structured errors with precise spans.

Changes:

  • Add uucore::diagnostics (feature-gated) to render terminal-only error reports against argv snapshots.
  • Replace stringly-typed mode parse errors with ModeError { message, span, kind } and propagate spans across comma-separated mode clauses.
  • Integrate diagnostics into chmod and add tests (including a test-harness helper to simulate stderr as a terminal while disabling color).

Reviewed changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/uutests/src/lib/util.rs Adds terminal_sim_stderr() helper for unix tests to simulate terminal stderr and disable color via NO_COLOR.
tests/by-util/test_chmod.rs Adds diagnostic-focused integration tests validating caret placement and terminal-vs-pipe behavior.
src/uucore/src/lib/lib.rs Re-exports uucore::diagnostics behind the diagnostics feature.
src/uucore/src/lib/features/mode.rs Introduces ModeError/ModeErrorKind with spans; updates chmod-mode parsing APIs to return structured errors.
src/uucore/src/lib/features/diagnostics.rs New diagnostics renderer built on ariadne, including snapshotting/locating operands inside argv text.
src/uucore/src/lib/features.rs Adds the diagnostics module behind a feature flag.
src/uucore/Cargo.toml Adds optional ariadne dependency and diagnostics feature.
src/uu/mknod/src/mknod.rs Adjusts error mapping for new mode parse error type.
src/uu/mkfifo/src/mkfifo.rs Converts ModeError to String for existing function signature.
src/uu/mkdir/src/mkdir.rs Converts ModeError to String for existing function signature.
src/uu/install/src/install.rs Adjusts translation formatting for new mode parse error type.
src/uu/chmod/src/diagnostics.rs New mapping layer from ModeErrorKind to localized labels/help and span alignment within the mode operand.
src/uu/chmod/src/chmod.rs Captures argv for rendering diagnostics and renders rich reports on parse failures (when stderr is a terminal).
src/uu/chmod/locales/fr-FR.ftl Adds localized diagnostic labels/help strings (French).
src/uu/chmod/locales/en-US.ftl Adds localized diagnostic labels/help strings (English).
src/uu/chmod/Cargo.toml Enables uucore’s diagnostics feature for chmod.
deny.toml Updates cargo-deny duplicate-version skip list for hashbrown.
Cargo.toml Adds workspace dependency entry for ariadne.
Cargo.lock Locks new dependency (ariadne) and associated transitive updates.
.vscode/cspell.dictionaries/workspace.wordlist.txt Adds spelling dictionary entries for new identifiers/deps.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/uu/chmod/src/chmod.rs
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.

2 participants