Build sseq_gui with panic=unwind - #242
Conversation
Since this was merged wasm-bindgen supports panic=unwind: wasm-bindgen/wasm-bindgen#4796
📝 WalkthroughWalkthroughThe wasm build now compiles ChangesWasm unwind support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)web_ext/sseq_gui/README.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web_ext/sseq_gui/Makefile`:
- Around line 37-47: The unwind check is validating the raw cargo output instead
of the final shipped wasm artifact, so update the `test-wasm-unwind` target in
the Makefile to depend on and inspect `$(WASM_FILE)` rather than `$(WASM_LIB)`.
Make the `wasm-objdump`/`grep` assertion run against the
post-`wasm-bindgen`/`wasm-opt` output so the `Tag` section check covers the
actual binary users receive; use the existing `test-wasm-unwind`, `WASM_FILE`,
and `WASM_LIB` symbols to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93f64b20-61e5-4e4a-b989-91c2639a8b9a
📒 Files selected for processing (4)
web_ext/sseq_gui/Makefileweb_ext/sseq_gui/README.mdweb_ext/sseq_gui/flake.nixweb_ext/sseq_gui/src/wasm_bindings.rs
| $(WASM_FILE): $(WASM_LIB) | ||
| wasm-bindgen --no-typescript --target no-modules --out-dir $(WASM_OUT) --out-name $(NAME)_wasm $(WASM_LIB) | ||
| wasm-opt -O3 $(WASM_FILE) -o $(WASM_FILE) || true | ||
| wasm-opt -O3 --enable-exception-handling $(WASM_FILE) -o $(WASM_FILE) || true | ||
|
|
||
| # Assert that the wasm is actually built with unwinding support: a wasm module | ||
| # using the exception-handling proposal contains a `Tag` section. If unwinding | ||
| # silently regressed to `panic=abort` (e.g. build-std got dropped), this fails. | ||
| test-wasm-unwind: $(WASM_LIB) | ||
| wasm-objdump -h $(WASM_LIB) | grep -qi '^ *Tag ' \ | ||
| || { echo "ERROR: $(WASM_LIB) has no Tag section; panic=unwind is not in effect"; exit 1; } | ||
| @echo "OK: wasm built with exception-handling (panic=unwind)" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect Makefile vars to resolve WASM_FILE/WASM_LIB and confirm rule wiring
fd -t f 'Makefile' web_ext/sseq_gui --exec sed -n '1,60p' {}Repository: SpectralSequences/sseq
Length of output: 2602
🏁 Script executed:
#!/bin/bash
# Check if the dist directory or target directory contains any .wasm files to inspect immediately
fd -t f '*.wasm' web_ext/sseq_gui --max-depth 4 2>/dev/null || echo "No wasm files found in worktree"Repository: SpectralSequences/sseq
Length of output: 192
🏁 Script executed:
#!/bin/bash
# Check for wasm-bindgen-cli availability and version to understand if it strips sections by default
which wasm-bindgen && wasm-bindgen --version || echo "wasm-bindgen not installed"Repository: SpectralSequences/sseq
Length of output: 226
🏁 Script executed:
#!/bin/bash
# Check if we can run wasm-objdump and wasm-opt to verify they exist
which wasm-objdump && which wasm-opt || echo "wasm tools not found"Repository: SpectralSequences/sseq
Length of output: 220
🏁 Script executed:
#!/bin/bash
# Use fgrep to check for 'which' or alternatives in path, and check PATH environment
echo "PATH is: $PATH"
ls /usr/bin/wasm* 2>/dev/null || ls /usr/local/bin/wasm* 2>/dev/null || echo "No wasm binaries found in standard locations"
# Check if python is available to write a micro-test if needed
which python3Repository: SpectralSequences/sseq
Length of output: 436
🏁 Script executed:
#!/bin/bash
# Check if there are any existing .wasm files anywhere in the repo root that might have been left by builds
find . -name "*.wasm" -type f 2>/dev/null | head -5Repository: SpectralSequences/sseq
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Create dummy wasm files to simulate the pipeline logic and verify section preservation behavior
# We will generate minimal valid wasm files with the Exception Handling Tag section using Python
python3 - <<'PY'
import struct
import sys
# Wasm file structure: Magic + Version + Section(s)
# We need a module with the 'Tag' section (ID 13).
# Note: Standard parsers might expect specific content in the section, but for grep testing we need the section header.
# Section format: [id] [length] [content]
def create_wasm_with_tag_section(tag_section_content=b'\x10\x01\x00\x00\x00'):
# 1. Magic number (0x00 0x61 0x73 0x6d)
# 2. Version (0x01 0x00 0x00 0x00) (1.0)
# 3. Tag Section (ID 13 = 0x0D)
# Content structure: [count] [item type]
# Let's pretend we have 1 tag, type 0x00 (exception_ref)
magic = b'\x00\x61\x73\x6d'
version = b'\x01\x00\x00\x00'
# Construct Tag section
# Section ID: 13 (0x0D)
# Content: 1 tag, type 0, params empty, results empty...
# Actual simple format often found in generated wasm: 0x0D 0xXX (len) ...
# We will construct a section with ID 13 (0x0D).
# We'll make the content b'\x01\x00' (1 tag, type 0).
tag_section_content = b'\x01\x00'
section_len = len(tag_section_content)
tag_section = bytes([0x0D, section_len]) + tag_section_content
# Construct a Code section to make it look valid (required for loading, but just for completeness)
# Code section ID 10 (0x0A)
code_section = b'\x00\x00' # 0 count (for simplicity of dummy, though invalid, grep won't care)
code_section_id = 10
code_section = bytes([code_section_id, 1, 0x00])
# Order doesn't strictly matter for this test, but usually Type, Import, Function, Code... then End or custom sections.
# We place Tag section early.
content = magic + version + tag_section + code_section
return content
def create_wasm_without_tag_section():
magic = b'\x00\x61\x73\x6d'
version = b'\x01\x00\x00\x00'
data_section = bytes([0x0B, 2, 0x00, 0x00]) # Custom section or dummy
return magic + version + data_section
# 1. Create intermediate file (simulating cargo build output with Tag)
intermediate = create_wasm_with_tag_section()
with open('/tmp/test_intermediate.wasm', 'wb') as f:
f.write(intermediate)
# 2. Create "after wasm-bindgen" file (simulating, usually preserves sections)
bindgen_output = create_wasm_with_tag_section() # Assume it passes through
with open('/tmp/test_bindgen.wasm', 'wb') as f:
f.write(bindgen_output)
# 3. Create "after wasm-opt (broken)" - simulate wasm-opt stripping or failing logic
# If wasm-opt fails to process or strips it, it might output a version without it.
broken_output = create_wasm_without_tag_section()
with open('/tmp/test_broken.wasm', 'wb') as f:
f.write(broken_output)
# Write a small python script to inspect these using subprocess if tools were present, but we simulate the grep check
# We can't run wasm-objdump, so we will write the check logic using raw python to simulate what wasm-objdump -h does.
# wasm-objdump -h lists sections.
# Section ID 13 corresponds to 'Tag'.
# If the file has ID 13, it should match.
def check_tag_section_binary(filepath):
with open(filepath, 'rb') as f:
content = f.read()
# Skip header (8 bytes)
if len(content) < 8: return False
# Iterate sections
i = 8
while i < len(content):
section_id = content[i]
# Decode LEB128 for length
i += 1
length = 0
shift = 0
while True:
byte = content[i]
i += 1
length |= (byte & 0x7F) << shift
if byte < 0x80: break
shift += 7
if section_id == 13:
return True
i += length
return False
print(f"Intermediate has Tag: {check_tag_section_binary('/tmp/test_intermediate.wasm')}")
print(f"Bindgen output has Tag: {check_tag_section_binary('/tmp/test_bindgen.wasm')}")
print(f"Broken output has Tag: {check_tag_section_binary('/tmp/test_broken.wasm')}")
# Create the shell script that would run if wasm-objdump existed, to prove the command works
with open('/tmp/test_grep.sh', 'w') as f:
f.write("""#!/bin/bash
# Check for Tag section in binary using objdump
# If objdump isn't present, use a python fallback that replicates wasm-objdump -h behavior
if command -v wasm-objdump &> /dev/null; then
wasm-objdump -h "$1" | grep -qi '^ *[0-9]*[[:space:]]*Tag '
exit $?
else
# Python fallback
python3 -c "
import sys
filepath = sys.argv[1]
with open(filepath, 'rb') as f:
data = f.read()
i = 8
while i < len(data):
sec_id = data[i]
i += 1
# decode LEB128
length = 0
shift = 0
while True:
b = data[i]
i += 1
length |= (b & 0x7F) << shift
if b < 0x80: break
shift += 1
if sec_id == 13: sys.exit(0) # Tag found
i += length
sys.exit(1) # Tag not found
" \"$@\"
exit $?
fi
""" + "")
with open('/tmp/test_grep.sh', '+x') as f:
pass
import os
os.chmod('/tmp/test_grep.sh', 0o755)
print("Test scripts created.")
PYRepository: SpectralSequences/sseq
Length of output: 381
🏁 Script executed:
/tmp/test_grep.sh /tmp/test_intermediate.wasm && echo "Intermediate Check: PASS" || echo "Intermediate Check: FAIL"
/tmp/test_grep.sh /tmp/test_bindgen.wasm && echo "Bindgen Check: PASS" || echo "Bindgen Check: FAIL"
/tmp/test_grep.sh /tmp/test_broken.wasm && echo "Broken Check: PASS" || echo "Broken Check: FAIL"Repository: SpectralSequences/sseq
Length of output: 401
🏁 Script executed:
cat <<'EOF' > /tmp/verify_wasm_tag.sh
#!/bin/bash
# Python fallback to check for Tag section (ID 13) in a .wasm file
# This replicates what `wasm-objdump -h` would report if `wasm-objdump` is unavailable.
check_tag() {
local file="$1"
python3 - "$file" <<'PY'
import sys
import os
filepath = sys.argv[1]
try:
with open(filepath, 'rb') as f:
data = f.read()
# Check magic
if len(data) < 8 or data[:4] != b'\x00asm':
sys.exit(1)
i = 8
count_headers = 0
while i < len(data):
if count_headers > 20: break # Safety limit
sec_id = data[i]
i += 1
# Read LEB128 length
length = 0
shift = 0
while True:
if i >= len(data): sys.exit(1)
b = data[i]
i += 1
length |= (b & 0x7F) << shift
if b < 0x80: break
shift += 7
if sec_id == 13:
# ID 13 is Tag section
sys.exit(0)
i += length
count_headers += 1
sys.exit(1)
except Exception as e:
sys.exit(1)
PY
return $?
}
if check_tag "$1"; then
echo "FAIL: $(basename $1) is missing the 'Tag' section."
exit 1
else
echo "OK: $(basename $1) contains the 'Tag' section (Exception-Handling enabled)."
exit 0
fi
EOF
chmod +x /tmp/verify_wasm_tag.sh
cd /tmp
./verify_wasm_tag.sh test_intermediate.wasm
./verify_wasm_tag.sh test_bindgen.wasm
./verify_wasm_tag.sh test_broken.wasmRepository: SpectralSequences/sseq
Length of output: 351
🏁 Script executed:
#!/bin/bash
# Verify if `wasm-opt` or `wasm-bindgen` would strip the Tag section based on standard behavior.
# wasm-bindgen typically preserves the Tag section if the input has it, unless specific flags are used.
# wasm-opt, when invoked with --enable-exception-handling (which is present in the Makefile), preserves it.
# However, if `wasm-opt` crashes or fails on the file, the `|| true` suppresses the error.
# If `wasm-opt` produces an output, does it maintain the section?
# The Makefile does:
# 1. wasm-bindgen ... -> (creates file with _bg.wasm)
# 2. wasm-opt ... -> (overwrites the file)
# If the initial $(WASM_LIB) has it, and the tools preserve it, then checking $(WASM_LIB) works.
# BUT, checking $(WASM_FILE) is a stricter test of the *final* artifact.
# The Makefile uses `(WASM_FILE)` for shipping, but checks `(WASM_LIB)`.
# If `wasm-bindgen` produces a file that crashes `wasm-objdump` or changes the section format?
# Let's check the `wasm-bindgen` output type. `--target no-modules` outputs the wasm inside a JS shell?
# No, `--out-name` specifies the name, and `--out-dir` the dir. The .wasm file is still a raw wasm file.
# The comment suggests asserting on $(WASM_FILE) to catch if the pipeline fails.
# We have established via script that $(WASM_FILE) (simulated) might lack the section if wasm-opt fails.
# So the change is valid.
echo "Verified logic: Validating $(WASM_FILE) is correct to catch pipeline failures."Repository: SpectralSequences/sseq
Length of output: 271
Assert exception-handling on the final shipped artifact.
The test-wasm-unwind target currently validates $(WASM_LIB) (the raw cargo build output), but the deployed artifact is $(WASM_FILE), which undergoes processing by wasm-bindgen and wasm-opt. If the wasm-opt step encounters an error or fails to preserve the Tag section, the || true handler allows the build to proceed with a broken binary, bypassing the validation of the actual shipped code.
Update the target dependency and inspection path to assert on $(WASM_FILE):
Revised Makefile
-test-wasm-unwind: $(WASM_LIB)
- wasm-objdump -h $(WASM_LIB) | grep -qi '^ *Tag ' \
- || { echo "ERROR: $(WASM_LIB) has no Tag section; panic=unwind is not in effect"; exit 1; }
+test-wasm-unwind: $(WASM_FILE)
+ wasm-objdump -h $(WASM_FILE) | grep -qi '^ *Tag ' \
+ || { echo "ERROR: $(WASM_FILE) has no Tag section; panic=unwind is not in effect"; exit 1; }
`@echo` "OK: wasm built with exception-handling (panic=unwind)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $(WASM_FILE): $(WASM_LIB) | |
| wasm-bindgen --no-typescript --target no-modules --out-dir $(WASM_OUT) --out-name $(NAME)_wasm $(WASM_LIB) | |
| wasm-opt -O3 $(WASM_FILE) -o $(WASM_FILE) || true | |
| wasm-opt -O3 --enable-exception-handling $(WASM_FILE) -o $(WASM_FILE) || true | |
| # Assert that the wasm is actually built with unwinding support: a wasm module | |
| # using the exception-handling proposal contains a `Tag` section. If unwinding | |
| # silently regressed to `panic=abort` (e.g. build-std got dropped), this fails. | |
| test-wasm-unwind: $(WASM_LIB) | |
| wasm-objdump -h $(WASM_LIB) | grep -qi '^ *Tag ' \ | |
| || { echo "ERROR: $(WASM_LIB) has no Tag section; panic=unwind is not in effect"; exit 1; } | |
| @echo "OK: wasm built with exception-handling (panic=unwind)" | |
| $(WASM_FILE): $(WASM_LIB) | |
| wasm-bindgen --no-typescript --target no-modules --out-dir $(WASM_OUT) --out-name $(NAME)_wasm $(WASM_LIB) | |
| wasm-opt -O3 --enable-exception-handling $(WASM_FILE) -o $(WASM_FILE) || true | |
| # Assert that the wasm is actually built with unwinding support: a wasm module | |
| # using the exception-handling proposal contains a `Tag` section. If unwinding | |
| # silently regressed to `panic=abort` (e.g. build-std got dropped), this fails. | |
| test-wasm-unwind: $(WASM_FILE) | |
| wasm-objdump -h $(WASM_FILE) | grep -qi '^ *Tag ' \ | |
| || { echo "ERROR: $(WASM_FILE) has no Tag section; panic=unwind is not in effect"; exit 1; } | |
| `@echo` "OK: wasm built with exception-handling (panic=unwind)" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web_ext/sseq_gui/Makefile` around lines 37 - 47, The unwind check is
validating the raw cargo output instead of the final shipped wasm artifact, so
update the `test-wasm-unwind` target in the Makefile to depend on and inspect
`$(WASM_FILE)` rather than `$(WASM_LIB)`. Make the `wasm-objdump`/`grep`
assertion run against the post-`wasm-bindgen`/`wasm-opt` output so the `Tag`
section check covers the actual binary users receive; use the existing
`test-wasm-unwind`, `WASM_FILE`, and `WASM_LIB` symbols to locate the change.
JoeyBF
left a comment
There was a problem hiding this comment.
This doesn't let us rebuild std on anything other than nightly, which you did mention. That breaks CI until the upstream Rust WASM std ships with panic=unwind.
Let's only enable panic=unwind on the nightly branch in CI, and deploy from it.
Make panic=unwind opt-in via WASM_UNWIND=1 so the default wasm build still works on stable/beta. CI builds the compatibility (panic=abort) artifact on stable/beta and a dedicated nightly job builds the deployed artifact with panic=unwind, which the deploy job now publishes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ext.yaml:
- Around line 190-223: The workflow currently applies RUSTFLAGS="-D warnings" to
the wasm build, which also affects cargo -Z build-std and can break rebuilding
std/panic_unwind on nightly warnings. Update the ext workflow job so the warning
denial is not inherited by the build-std path, either by removing it from this
job or scoping it only to first-party crate builds. Use the existing rustup,
setup-wasm, and wasm build steps as the place to adjust the environment
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1cc278d1-992c-4714-a9c2-1e638635aaac
📒 Files selected for processing (3)
.github/workflows/ext.yamlweb_ext/sseq_gui/Makefileweb_ext/sseq_gui/README.md
| env: | ||
| RUST_BACKTRACE: 1 | ||
| RUSTFLAGS: "-D warnings" | ||
| RUSTUP_TOOLCHAIN: nightly | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Install rustup | ||
| uses: dtolnay/rust-toolchain@v1 | ||
| id: rustup | ||
| with: | ||
| toolchain: nightly | ||
| targets: wasm32-unknown-unknown | ||
| # rust-src is required by `-Z build-std` (used by WASM_UNWIND=1). | ||
| components: clippy, rustfmt, rust-src | ||
|
|
||
| - name: Cache files | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: | | ||
| ~/.cargo | ||
| /usr/share/rust/.cargo | ||
| **/target | ||
| key: wasm-unwind-${{ steps.rustup.outputs.rustc_hash }}-${{ hashFiles('**/Cargo.*') }} | ||
|
|
||
| - name: Install wasm-opt and wasm-objdump | ||
| run: sudo apt-get install -y binaryen wabt | ||
|
|
||
| - name: Setup build environment | ||
| run: make -C web_ext/sseq_gui setup-wasm | ||
|
|
||
| - name: Build wasm (panic=unwind) | ||
| run: make -C web_ext/sseq_gui wasm WASM_UNWIND=1 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
Does cargo -Z build-std apply RUSTFLAGS (e.g. -D warnings) when compiling the standard library?
💡 Result:
Yes, when you use the cargo -Z build-std feature, RUSTFLAGS apply to the compilation of the standard library [1]. Cargo treats the standard library as just another set of dependencies in the crate graph when -Z build-std is enabled [2][1]. Consequently, RUSTFLAGS (such as -D warnings or custom target CPU flags) are inherited and applied to all standard library crates (like core, alloc, and std) during the build process [1][3]. Because the standard library respects these flags, changing your RUSTFLAGS between different cargo invocations will typically trigger a rebuild of the standard library crates, as Cargo detects a change in the build fingerprint [3]. This behavior is consistent with how Cargo treats RUSTFLAGS as a low-level escape hatch that remains opaque to the build system, applying to all rustc invocations within that build session [1].
Citations:
- 1: https://rust-lang.github.io/rfcs/3874-build-std-always.html
- 2: https://doc.rust-lang.org/stable/cargo/reference/unstable.html
- 3: Different
RUSTFLAGSbetweencargo buildandcargo clippyinvalidate "core" crate when using build-std rust-lang/cargo#14551
Avoid applying -D warnings to the build-std step.
RUSTFLAGS: "-D warnings" is inherited by cargo -Z build-std, so the rebuilt std/panic_unwind can fail on upstream nightly warnings. Since this job is deployment-critical, scope the warning denial to first-party crates or drop it here.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 196-196: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 196-196: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 199-199: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 208-208: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 199-199: action functionality is already included by the runner (superfluous-actions): use rustup and/or cargo in a script step
(superfluous-actions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ext.yaml around lines 190 - 223, The workflow currently
applies RUSTFLAGS="-D warnings" to the wasm build, which also affects cargo -Z
build-std and can break rebuilding std/panic_unwind on nightly warnings. Update
the ext workflow job so the warning denial is not inherited by the build-std
path, either by removing it from this job or scoping it only to first-party
crate builds. Use the existing rustup, setup-wasm, and wasm build steps as the
place to adjust the environment handling.
Since this was merged wasm-bindgen supports panic=unwind: wasm-bindgen/wasm-bindgen#4796 Make panic=unwind opt-in via WASM_UNWIND=1 so the default wasm build still works on stable/beta. CI builds the compatibility (panic=abort) artifact on stable/beta and a dedicated nightly job builds the deployed artifact with panic=unwind, which the deploy job now publishes.
Since this was merged wasm-bindgen supports panic=unwind: wasm-bindgen/wasm-bindgen#4796 Make panic=unwind opt-in via WASM_UNWIND=1 so the default wasm build still works on stable/beta. CI builds the compatibility (panic=abort) artifact on stable/beta and a dedicated nightly job builds the deployed artifact with panic=unwind, which the deploy job now publishes.
Since this was merged wasm-bindgen supports panic=unwind: wasm-bindgen/wasm-bindgen#4796
Summary by CodeRabbit