Fix Windows-only theme CSS fallback and metadata href separators#374
Merged
Conversation
On a Windows checkout the SCSS sources baked in via include_dir! carry CRLF line endings (git autocrlf). parse_layer's boundary detection uses a $-anchored regex against the raw content; in multiline mode $ matches before the \n but leaves the trailing \r on the marker line, so CRLF-terminated markers like /*-- scss:defaults --*/ fail to match. The gate then reports no boundary markers, theme/bootstrap/highlight layer compilation errors out, and the render silently falls back to base CSS — so on Windows no theme variables, custom-scss markers, or highlight classes reach the output. Normalize CRLF to LF once at parse_layer entry, a single line-ending agnostic chokepoint. SCSS-to-CSS compilation keeps no byte-offset map, so normalizing here is correct (contrast the document-content preserve policy, where byte offsets must survive). The parse loop already strips \r via str::lines(); the only failure point was the boundary-detection gate, but normalizing at entry also guards any future line-anchored scan and keeps a clean LF stream flowing into grass. The CRLF path is covered by an in-source unit test that builds \r\n input inline, so it runs on Linux CI rather than only on a Windows checkout.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Records the design for fixing Windows `@import` resolution in embedded SCSS: the two path domains (real OS paths vs the forward-slash virtual /__quarto_resources__ namespace), the grass PathBuf::join root cause with source anchors, the native-vs-WASM contract comparison, and the chosen fix (a single canonicalization boundary in EmbeddedResources). Also captures the deferred WASM pool-unification and Windows-CI items and the decisions taken with the design's multi-agent + grass-source research.
The canonicalization boundary is already owned by quarto-util (to_forward_slashes), byte-identical to the proposed helper and already tested. Reuse it instead of adding a new function, and note the two existing private copies to consolidate. Records the quarto-util dependency addition to quarto-sass and the WASM-build verification step.
grass resolves `@import`/`@use` by PathBuf::join-ing each load path with the import specifier. Our embedded resources use virtual, forward-slash load paths (/__quarto_resources__/...), but on Windows PathBuf::join inserts a native backslash at the join boundary, and EmbeddedResources' lookup keys are forward-slash (include_dir normalizes at build time). The corrupted key (e.g. "\vendor/_rfs.scss") never matched, so every embedded `@import` inside Bootstrap failed, theme compilation errored, and the render fell back to base CSS on Windows-from-source builds. Normalize to forward slashes at the single embedded-lookup boundary (EmbeddedResources::strip_prefix and the key collectors) using the existing quarto_util::to_forward_slashes helper rather than a new function. RuntimeFs is left untouched so its std::fs fallback keeps genuine native paths intact for real on-disk files. Design and root-cause analysis: claude-notes/designs/embedded-resource-virtual-path-contract.md
adjust_paths_to_document_dir rebases !path metadata values (css, resources, bibliography, ...) relative to the document directory. Two Windows-only defects lived in that one function: - The rebased value is stringified with to_string_lossy, so on Windows pathdiff's native separators leaked into the value. Since it is used verbatim in HTML hrefs (a css: !path <link>), the emitted attribute became href="..\..\shared\styles.css" instead of forward slashes. Normalize at the emission site with quarto_util::to_forward_slashes, the same helper the SCSS and JSON-writer separator fixes reuse. - The relative-path guard used Path::is_relative, which on Windows is false only for drive-prefixed paths; a POSIX-absolute value like /usr/share/base.css is not is_absolute there, so it was wrongly rebased into ../../../usr/share/base.css. Guard on is_rooted (has_root) instead, matching the helper's documented purpose. The suite's dir-metadata-paths fixture and the existing directory_metadata_tests (previously red on Windows, green elsewhere) now pass on all platforms.
cderv
force-pushed
the
bugfix/bd-22rtwdur-windows-smoke-all-suite
branch
from
845bc84 to
4ca6ca4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On a Windows checkout,
cargo nextest run --workspace -E 'binary(integration) & test(smoke_all)'fails on 21 of 90 smoke fixtures: every theme (custom, project, metadata, cascade, bootswatch) fixture missing its custom colors and marker classes, onedir-metadata-pathsfixture with a backslash leaking into an HTMLhref, and Python syntax highlighting missing.hl-keyword. CI runs ubuntu + macos only, so none of this showed up there.Root Cause
Two independent defects, both from path/string handling that behaves differently on Windows:
resources/scss/**/*.scssare checked out with CRLF line endings (gitautocrlf) and baked into the binary viainclude_dir!.parse_layer's boundary-marker regex is$-anchored; in multiline mode$matches before\nbut leaves a trailing\ron CRLF-terminated markers like/*-- scss:defaults --*/, so the regex misses every marker. Layer parsing then reportsNoBoundaryMarkers, theme compilation errors, and the render silently falls back to base CSS — dropping every theme variable, custom-scss layer, and highlight class.grassresolves@import/@usebyPathBuf::join-ing each load path with the import specifier. Embedded resources use forward-slash virtual load paths, but on WindowsPathBuf::joininserts a native backslash at the join boundary, while the embedded-resource lookup keys are forward-slash (include_dirnormalizes at build time). The corrupted key never matched, so embedded@imports inside Bootstrap also failed.Separately,
adjust_paths_to_document_dir(used for!pathmetadata likecss:) stringified the rebased path withto_string_lossy, leaking native backslashes into HTMLhrefattributes, and usedPath::is_relativeas its "already absolute" guard — which isfalsefor POSIX-absolute paths like/usr/share/base.csson Windows, wrongly rebasing them.Fix
parse_layerentry — SCSS→CSS keeps no byte-offset map, so this is a safe single chokepoint (crates/quarto-sass/src/layer.rs).quarto_util::to_forward_slasheshelper rather than adding a new one (crates/quarto-sass/src/resources.rs).!path-adjusted values withquarto_util::to_forward_slashes, and guard onis_rooted(has_root) instead ofPath::is_relative(crates/quarto-core/src/project/mod.rs).Root-cause and design notes:
claude-notes/designs/embedded-resource-virtual-path-contract.md.Test Plan
cargo nextest run -p quarto-sass— 206/206cargo nextest run -p quarto-core -E 'test(directory_metadata)'— 16/16cargo nextest run --workspace -E 'binary(integration) & test(smoke_all)'— passes (was 21 failures)