-
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.
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'm a bit confused. What do you mean by "fixing the environment"?
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.
having the separate containers have the proper builds as defaults as well as not have dual version builds at once
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 script is not building same version at once, but for development needs. #572 (comment)
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.
If PROFILE isn't expressed by an environment variable or a flag, we should make this fail and print a "NO PROFILE SET" error message instead of just picking one randomly. That way we don't run into bugs and have no idea whats happening.
If theres a problem in the CI stemming from that we have to set up the CI env properly in another PR.
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.
Profile is not an environment variable anymore. It's only an "option" determined by user input
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.
Either way defaulting to "use the one that exists" is bad practice. Now that I've taken a second look I'm not sure removing the ENV variable is best practice, will probably make things harder to configure globally and is more of a bandaid then addressing the underlying issue.
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.
Profile is not used elsewhere previous
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
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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: