Fix Web GUI local assets - #11166
Draft
jorge-ferreira-pii wants to merge 15 commits into
Draft
Conversation
The report concatenates the widget sources into one <script type="module">, with their import/export statements stripped by regex. Two forms are not covered: `export async function` (tile-merge.js) and an import spread over several lines (websocket-tile-layer.js). Either one is a syntax error, and a syntax error there kills every widget in the report at once -- today the report opens with "Uncaught SyntaxError: Unexpected token 'export'" and nothing but a console message to say so. Cover both, and stop being silent about the rest: what the patterns miss now fails the build, as does content a raw string literal cannot carry verbatim. The import match is anchored on the quoted specifier rather than on the semicolon, which may be absent, so it can no longer swallow the statement that follows an import. embed_report_assets_test pins every shape. asset_list_parity_test holds the report's JS list -- ordered, because the files share one scope -- and the embedded asset list equal between the two build systems, which is what the CMake list being out of order showed was worth checking. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
check_no_module_syntax stops the build when a form the regexes miss survives; this is the same rule on the artefact, so the two layers do not depend on each other being right. The generator marks every source it concatenates, which is the boundary the test walks: before the first marker are the imports web.cpp writes for the libraries the report loads, after it code that must carry no module syntax at all. Anchoring there rather than on the specifiers keeps the test true however those libraries come to be resolved. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
The viewer pulled leaflet, golden-layout, three, elkjs and netlistsvg from CDNs, and the netlistsvg host redirects to plain http, so the browser executed code fetched over an unencrypted connection (issue The-OpenROAD-Project#11065). Check the libraries in instead, with update_vendor.py to reproduce the tree from the npm registry and vendor.lock.json to pin every byte. Nothing serves these files yet. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
index.html loaded leaflet, golden-layout, elk and netlistsvg from CDNs, and main.js and 3d-viewer-widget.js imported three and golden-layout from esm.sh. Point them all at the vendored copies, which the binary now serves: the two ES modules through an import map, so the specifiers stay bare and the saved report can remap them to inlined copies. Serving them takes two changes to embed_web_assets.py. Assets are now given as <served path>=<file path> instead of being keyed by basename, because the stylesheets reach their icons through relative urls, and content is read as bytes, with anything a raw string literal cannot hold verbatim -- images, CRLF, NUL -- written as octal escapes. Fixes The-OpenROAD-Project#11065. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
web_save_report repeated index.html's CDN tags, so a report opened later fetched leaflet, golden-layout and three over the network -- and, once the viewer stopped doing that, from hosts the design owner may not reach at all. Inline the vendored copies as data: URIs instead. The stylesheets need their url() references inlined too, otherwise the icons resolve against whatever directory the report was saved in, and the two ES modules go through an import map because a data: URL cannot resolve a relative import. base64Encode now takes a pointer and a size, which is what lets an embedded asset be encoded without copying it into a vector first. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
Serving the libraries locally fixes today's viewer; the policy is what keeps it fixed. Every response now carries a Content-Security-Policy that names no remote origin, so a reintroduced CDN reference fails visibly in the browser instead of quietly fetching remote code, plus nosniff. The saved report carries the same intent in a <meta>, since a file:// document has no headers. The one inline block, the import map, is allowed by hash rather than by 'unsafe-inline'; embed_web_assets.py computes it from the markup so the two cannot drift. TestWebAssets holds every embedded asset to the rule, and the vendored tree to vendor.lock.json. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
A sweep over the branch for anything not strictly required by The-OpenROAD-Project#11065, plus two defects in the vendoring it added. The policy loses worker-src and font-src: removing each one and re-rendering shows the viewer does not use them, and default-src 'none' names the missing directive in the console if a future dependency does. nosniff goes with them, being unrelated hardening, and the unused .mjs MIME entry too. update_vendor.py recorded the esbuild digest only for the platform it ran on, so the first run on any other one executed a binary with nothing to verify it against; --rewrite-lock now records all four and a missing one is an error. It also ran esbuild on an absolute path, which esbuild writes into the bundle's comments, so the vendored golden-layout differed on every run and the tree could not be reproduced from the lock. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
WEB-0044 was warning from two places, which fails //:dup_id_test and the messages() POST_BUILD of the web target; the lookup and the warning move into one reportAsset() instead of the id being duplicated. update_vendor.py had no execute bit, so the web_vendor_lock ctest could not launch it. In the stylesheet inliner, a padded url( x.png ) resolved to a path with the spaces in it and a root-absolute url(/x.png) had base_dir prepended; both ended as an empty url() plus a warning. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
Nothing here changes behaviour. resolveAssetPath was a hand-rolled path normaliser; std::filesystem does it, including the root-absolute case operator/ already handles. stylesheetDataUri derives its base directory from the path it is given, which retires the three directory constants the report head carried, and base64Encode is one function over a string_view rather than three overloads. The report tests decode with utl::base64_decode, assert on every stylesheet rather than the first, and anchor on the <script type="module"> boundary instead of a literal import line. embed_report_assets.py now fails the build when module syntax survives its regexes, where the C++ test only caught it downstream, and it names its raw string delimiters once. embed_web_assets.py refuses a second HTML asset, whose hashes would silently widen the one policy every page is served with. Both build systems keep their own asset list, as everything in this repo does, but asset_list_parity_test now holds them equal -- they have already drifted once, and for the report the order is what runs. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
InlinedScriptHasNoModuleSyntaxLeft anchored on the last "import " in the module, which lands on a comment: it scanned 1082 of 13101 lines. It now walks every line with the rule the generator applies, and holds the two the import map resolves to a count. The import regex was anchored on the semicolon, so an import without one, or a top-level import(), deleted every line up to the next semicolon -- and check_no_module_syntax could not see what was already gone. Anchoring on the quoted specifier bounds the match: it still spans the newlines a multi-line import needs, but crosses neither a semicolon nor a quote. The new embed_report_assets_test pins both, and the export forms, and the guards. style.css went into the report's <style> block without inlineStylesheetUrls, so it was the one stylesheet outside the mechanism; it goes through it now, and StylesheetIconsAreInlined covers that block too. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
update_vendor.py refused a tarball whose digest differed from the lock but not one the lock had never seen, so a version bumped in PACKAGES pinned whatever arrived; the guard the previous commit's message described was lost before it was committed, and this is it. --check now compares the pinned versions with the ones the script asks for, which is what catches a bump that never ran the vendoring. web_save_report warned about a missing embedded asset and then announced success, leaving a report with src="" that opens to a blank page; the assets it inlines are now checked before the file is opened, and a miss is WEB-0045. The head's comment claimed the report inlines what index.html loads -- it leaves out elk and netlistsvg, 2.8 MB for a panel that needs the server, which the README now says too. The stylesheet inliner ended a url( token at the first ')', so a quoted reference containing one would have been truncated. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
The seven path constants added for the pre-flight check were a list to keep in step with the head that writes them, and two of them had already fallen out of step: the golden-layout stylesheet and module were still written as literals, so the check guarded paths the writer no longer used. The inliners now look assets up through a ReportAssets, which remembers a miss; saveReport removes the file and errors if anything was missed. The list goes away, the literals exist once again, and the icons a stylesheet reaches through url() are covered too -- those were never in the list, so a missing one still shipped a report with an empty url(). update_vendor.py decided a version was unpinned only after downloading every package before it, leaving the tree half-rewritten; it now decides before the first byte moves, through the same comparison --check makes, and the pinned and mismatched cases are one guard with one message instead of two. The stylesheet scanner knows the span of a quoted reference once it has found the closing quote, so it no longer throws that away and re-derives it by trimming and unquoting further down. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
The stylesheet inliner looked for a lowercase url( token, so an upgrade that emitted URL( would have left a relative icon reference in a report that opens from file://, and the test looked for it the same way, so CI would have stayed green. Both are case-insensitive now. The token also has to be a token: a miss refuses to save the report, so matching the tail of an identifier -- as in myurl( -- would turn a stylesheet the browser reads fine into a refusal. update_vendor.py extracted a bundled tree by joining member names from the tarball onto a temporary directory, which a name reaching up out of it would have escaped; extract_member now takes the root it is writing under and refuses anything that resolves outside it. The digest is no help here on a --rewrite-lock run, which is precisely the run where the tarball is new. embed_web_assets.py hashed the inline scripts while the output file was open, so a refusal left a truncated .cpp newer than its inputs, which the next build does not regenerate. It hashes before opening, as update_vendor.py already validates before downloading. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
The two rules the previous commit gave the url( scanner -- case-insensitive,
and it has to be a token -- had no test in the repo, and the report test had
grown its own half-copy of the rule: it looked for the token in any case but
ignored the boundary, so a stylesheet carrying foo-url(x.png), which the scanner
skips on purpose, would have failed it with a message about an unresolved
reference.
css_inliner.h exports what the scanner is, TestCssInliner pins it -- both rules,
the reference forms, the quoted parenthesis, and the miss the report is refused
over -- and the report test now finds tokens through the production scanner, so
there is one rule. findUrlToken drives off the '(' with find() and compares
with sta::stringBeginEqual, which is both shorter than the byte loop it replaces
and faster than the plain find("url(") that came before it.
extract_member returns the destination it validated, so esbuild_binary stops
spelling that path twice and chmods the file it wrote. embed_web_assets.py
writes its output aside and renames, so no failure can leave a truncated .cpp
newer than its inputs, and two served paths that fold into one C identifier are
now refused by name rather than by a redefinition error in generated code.
Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request vendors browser libraries (Leaflet, GoldenLayout, Three.js, elkjs, and netlistsvg) directly into the OpenROAD binary, allowing the web viewer and saved reports to function entirely offline without loading code from CDNs. It updates the build systems (Bazel and CMake), adds scripts to embed these assets, implements CSS URL inlining to handle relative paths in stylesheets, and adds extensive tests to verify that no external resources are requested. The review feedback highlights several robustness improvements, including using generic_string() for cross-platform path consistency on Windows, ignoring external URLs when rewriting stylesheet references, handling potential exceptions in std::filesystem::remove, and enhancing regex patterns in the Python asset-embedding scripts to robustly match src= attributes and strip import statements with trailing comments.
| { | ||
| return (std::filesystem::path(base_dir) / std::filesystem::path(reference)) | ||
| .lexically_normal() | ||
| .string(); |
Contributor
There was a problem hiding this comment.
On Windows, std::filesystem::path::string() returns paths with backslashes (\\), which will fail to match the forward-slash paths used in the embedded asset lookup table (kAssetTable). Using generic_string() ensures forward slashes are used consistently across all platforms, preventing failures when saving reports on Windows.
Suggested change
| .string(); | |
| .generic_string(); |
Comment on lines
+1162
to
+1163
| if (reference.empty() || reference.front() == '#' | ||
| || reference.starts_with("data:")) { |
Contributor
There was a problem hiding this comment.
If a stylesheet contains references to external URLs (e.g., Google Fonts or external images starting with http://, https://, or //), they should be left alone. Otherwise, resolveAssetPath will treat them as local paths, fail to find them in the embedded assets, set missing_ = true, and ultimately cause the report saving to fail entirely.
Suggested change
| if (reference.empty() || reference.front() == '#' | |
| || reference.starts_with("data:")) { | |
| if (reference.empty() || reference.front() == '#' | |
| || reference.starts_with("data:") | |
| || reference.starts_with("http://") | |
| || reference.starts_with("https://") | |
| || reference.starts_with("//")) { |
| if (assets.missing()) { | ||
| // The warnings above name what was missed; what matters here is that no | ||
| // one is told a blank report was saved. | ||
| std::filesystem::remove(filename); |
Contributor
There was a problem hiding this comment.
std::filesystem::remove can throw a std::filesystem::filesystem_error exception if the file removal fails (e.g., due to permissions or file locks). To prevent potential crashes, use the std::error_code overload to handle failures gracefully.
std::error_code ec;
std::filesystem::remove(filename, ec);
Comment on lines
+98
to
+99
| if "src=" in attributes: | ||
| continue |
Contributor
There was a problem hiding this comment.
Checking for "src=" in attributes is case-sensitive and does not handle spaces around the = sign (e.g., src = "..."). Using a regular expression is more robust and ensures inline scripts are correctly identified.
Suggested change
| if "src=" in attributes: | |
| continue | |
| if re.search(r"\bsrc\s*=", attributes, re.IGNORECASE): | |
| continue |
Comment on lines
+27
to
+32
| content = re.sub( | ||
| r"^import\b[^'\";]*(['\"])[^'\"\n]*\1[ \t]*;?[ \t]*$", | ||
| "", | ||
| content, | ||
| flags=re.MULTILINE, | ||
| ) |
Contributor
There was a problem hiding this comment.
If an import statement has a trailing single-line comment (e.g., import { X } from './x.js'; // comment), the regex will fail to match and strip it. This will leave the import statement in the concatenated script, causing a syntax error in the browser. Allowing optional trailing comments makes the stripping process more robust.
content = re.sub(
r"^import\b[^'";]*(['"])[^'"\n]*\1[ ]*;?[ ]*(?://.*)?$",
"",
content,
flags=re.MULTILINE,
)Only comments change: the rationale and the measured numbers go, the invariants stay. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
jorge-ferreira-pii
changed the title
Contributor
|
@jorge-ferreira-pii is there a way to set it up so bazel / cmake fetches the files instead of vendoring them? (not sure it really matters, but this makes it hard to tell what "version" we have of each) |
Contributor
Author
Yes, both can. Bazel handles it easily. CMake is the harder half: it would need the network at configure time, plus Node.js in the C++ build to bundle golden-layout, which ships no ready-to-serve browser build. I am not sure how much the network part really costs, though. Regarding your concern about versions: they are already pinned and CI-checked with hashes per file/package; it just doesn't show in the diff. |
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
2 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.
Fix #11065