Skip to content

Fix optional extensions with inaccessible PATH entries - #3187

Open
tianrking wants to merge 1 commit into
rust-lang:mainfrom
tianrking:fix/optional-renderer-path-errors
Open

tianrking wants to merge 1 commit into
rust-lang:mainfrom
tianrking:fix/optional-renderer-path-errors

Conversation

@tianrking

@tianrking tianrking commented Aug 10, 2026

Copy link
Copy Markdown

Fixes #3170.

Problem

A missing optional renderer or preprocessor can still fail a build when the
platform reports command lookup differently from ErrorKind::NotFound, or when
the child exits before mdBook writes its JSON input. This regressed the expected
optional = true behavior on Linux.

Change

  • Preflight optional extension commands on both Unix and Windows before spawning
    them, so the optional-command decision is not dependent on child-process timing.
  • Preserve the child command's working directory when resolving relative command
    paths and relative PATH entries.
  • On Unix, treat an inaccessible PATH entry with no usable command as absent,
    while direct command paths and existing/non-executable files still go through
    spawn so their real errors are retained.
  • On Windows, mirror std::process::Command's documented bare-command search:
    the executable directory, system directory, Windows directory, then PATH;
    an omitted .exe extension is also considered.
  • Apply the common preflight to renderer execution and both preprocessor paths
    (run and supports).

Regression coverage

  • Renderer and preprocessor: a missing optional command with an inaccessible
    PATH entry is skipped on Unix.
  • Unix: an existing non-executable direct command is not misclassified as
    missing.
  • Windows: an extensionless command with an existing .exe is not
    misclassified as missing.

Validation

Local Windows validation with Rust 1.88.0:

  • cargo test --workspace --locked
  • cargo test --workspace --no-default-features --locked
  • cargo test -p mdbook-driver
  • cargo test --test testsuite missing_optional -- --nocapture
  • cargo clippy -p mdbook-driver --all-targets --no-deps -- -D warnings
  • cargo clippy -p mdbook-driver --all-targets --no-default-features --no-deps -- -D warnings
  • cargo fmt --all -- --check
  • git diff --check

@rustbot rustbot added the S-waiting-on-review Status: waiting on a review label Aug 10, 2026
@GuillaumeGomez

Copy link
Copy Markdown
Member

Would be better to handle unix and windows the same.

@GuillaumeGomez GuillaumeGomez added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: waiting on a review labels Aug 17, 2026
@tianrking
tianrking force-pushed the fix/optional-renderer-path-errors branch 2 times, most recently from e322957 to 915ae72 Compare September 8, 2026 16:25
@rustbot

This comment has been minimized.

1 similar comment
@rustbot

rustbot commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@tianrking

Copy link
Copy Markdown
Author

@GuillaumeGomez I updated this in 915ae72 to use the same optional-command preflight on Unix and Windows.

  • Windows now follows std::process::Command's documented bare-command search locations and .exe elision.
  • Direct paths and existing/non-executable files still go through spawn, so their real errors are preserved.
  • The Unix inaccessible-PATH renderer and preprocessor regressions remain covered; I also added Unix direct-command and Windows extensionless-command coverage.

The branch has been rebased onto current main. Both fresh CI runs for 915ae72 are green (38 successful checks total). Could you please take another look?

@GuillaumeGomez GuillaumeGomez 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.

I don't think this is the right approach. Why not fixing the existing error in spawn and crate::handle_command_error instead?

View changes since this review

@rustbot

rustbot commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

optional = true renderer fails with Broken pipe mdBook 0.5.4

3 participants