-
Notifications
You must be signed in to change notification settings - Fork 15
ci/cd: Fix lind script and docker file #572
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
End-to-End Test ReportTest PreviewTest Report Deterministic TestsSummary
Test Results by Category
Non Deterministic TestsSummary
Test Results by Category
Fail TestsSummary
Test Results by Category
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
End-to-End Test ReportTest PreviewTest Report Deterministic TestsSummary
Test Results by Category
Non Deterministic TestsSummary
Test Results by Category
Fail TestsSummary
Test Results by Category
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Policy: | ||
| # - If user explicitly specifies a profile, honor it strictly. | ||
| # - Otherwise: | ||
| # * Use the only available profile if exactly one exists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this isn't a great way to go about this since we can easily be confused that were using one while actually using the other. Id rather there be some sort of warning/error if there's a mismatch and we fix the environments properly.
| ENV CLANG="/home/${USERNAME}/lind-wasm/${CLANG_PACKAGE}" | ||
|
|
||
| # --- Prebuild --- | ||
| ENV WASMTIME_PROFILE=debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the implicit WASMTIME_PROFILE=debug avoids accidental debug/runtime mismatches. This should help ensure local dev matches CI behaviour more closely.
| # --- tool paths & quick checks (anchored to repo) --- | ||
| CLANG_BIN="clang" # must be on PATH | ||
| WASM_OPT_BIN="${REPO_ROOT}/tools/binaryen/bin/wasm-opt" | ||
| WASMTIME_PROFILE="${WASMTIME_PROFILE:-release}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping implicit selection via WASMTIME_PROFILE makes execution more deterministic and avoids masked local issues. This sounds like it directly addresses the issue in issue #560
| WASMTIME_PROFILE="debug" | ||
| WASMTIME_BIN="${DBG_BIN}" | ||
| elif [[ ${rel_ok} -eq 1 && ${dbg_ok} -eq 1 ]]; then | ||
| echo "error: both release and debug wasmtime binaries exist:" >&2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes ambiguity explicit, which is good. One thing to think about. Do you plan to change any CI jobs or workflows to build both in the same workspace? If you ever do, you'll need to be mindful of this. Should be fine as it is now, though
| Options: | ||
| --wasmtime-profile=release|debug | ||
| Explicitly select which wasmtime build to use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is clear in the help output. It might also be worth adding a short note to the developer docs/README that WASMTIME_PROFILE is no longer used for binary selection.
This PR resolves the WASMTIME_PROFILE–related issue described in #560.
Previously, our development environment implicitly relied on
WASMTIME_PROFILEto decide which wasmtime binary to execute. In particular:WASMTIME_PROFILE=debug.As a result, both debug and release binaries could coexist, but
lind_runwould always execute the debug binary.This behavior masked issues locally and caused discrepancies between server-side development and CI, which runs in a clean environment.
This PR makes two related changes to eliminate the ambiguity:
In Dockerfile.dev:
In lind_run script:
--wasmtime-profile={release|debug}.In lind_compile script: