fix: util.shell(), util.sleep() and util.spawn() do not work in preflight
#5425
Conversation
|
Thanks for opening this pull request! 🎉
|
|
Console preview environment is available at https://wing-console-pr-5425.fly.dev 🚀 Last Updated (UTC) 2024-01-05 03:51 |
BenchmarksComparison to Baseline ⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜
⬜ Within 1.5 standard deviations Benchmarks may vary outside of normal expectations, especially when running in GitHub Actions CI. Results
Last Updated (UTC) 2024-01-18 22:03 |
Signed-off-by: monada-bot[bot] <monabot@monada.co>
monadabot
added
the
⚠️ pr/review-mutation
| @@ -135,32 +141,37 @@ export class Util { | |||
| * @param opts `ShellOptions`, such as the working directory and environment variables. | |||
| * @returns The standard output of the shell command. | |||
| * @throws An error if the shell command execution fails or returns a non-zero exit code. | |||
| * @inflightImpl __shell__inflight | |||
There was a problem hiding this comment.
nit: this reminded me of python so it looked kinda weird 🌚 Doesn't really matter but maybe a more direct convention?
| * @inflightImpl __shell__inflight | |
| * @inflightImpl shell$$inflight |
I assume a leading underscore was needed because it's marked as @internal, but I would also suggest just making it private instead. Thanks to the magic of JS, it's still accessible so it should still work.
Signed-off-by: monada-bot[bot] <monabot@monada.co>
| @@ -801,6 +801,8 @@ pub struct FunctionSignature { | |||
| /// - `$args$`: the arguments passed to this function call | |||
| /// - `$args_text$`: the original source text of the arguments passed to this function call, escaped | |||
| pub js_override: Option<String>, | |||
| /// During jsify, calls to this function will be replaced with this other function name | |||
| pub inflight_impl: Option<String>, | |||
There was a problem hiding this comment.
I guess in winglang this would just be two functions with the same name, correct?
class Foo {
bar() { log("preflight");
inflight bar() { log("inflight"); }
}In that vane, this makes me wonder if it makes more sense to model and implement this as two function members with the same name and have the compiler select the phase-specific version (might break like a million assumptions).
Then, in the JSII side, we can decide that foo$inflight is imported as the inflight version of foo.
There was a problem hiding this comment.
I think you're onto something...
eladb
removed
the
⚠️ pr/review-mutation
|
@Chriscbr let's get this one merged :-) |
| /// During jsify, calls to this function will be replaced with this other function name | ||
| pub inflight_impl: Option<String>, |
There was a problem hiding this comment.
I'd like to try avoiding this new machinery by reusing the _toInflightType methods we already support
|
Hi, This PR has not seen activity in 20 days. Therefore, we are marking the PR as stale for now. It will be closed after 7 days. |
| public static sleep(delay: Duration): void { | ||
| Atomics.wait( | ||
| new Int32Array(new SharedArrayBuffer(4)), | ||
| 0, | ||
| 0, | ||
| delay.seconds * 1000 | ||
| ); | ||
| } |
There was a problem hiding this comment.
With #6157, this approach will no longer work (preflight runs in a child process instead of a worker).
I think the only "reasonable" way left would be doing it via a shell.
garysassano
changed the title
util.shell(), util.sleep() and util.spawn() do not work in preflight
Currently,
util.shelldoes not behave as expected when called in preflight code because the underlying implementation returns a promise. There were a couple possible solutions for this:Neither of these address the issue fully. Instead, I've opted to allow unphased functions in the SDK to have both preflight and inflight implementations. The compiler is then able to use its existing phase information to emit the name of either the preflight or inflight method implementation when producing JavaScript code.
In the process of writing this, I also:
fsmoduleutil.sleepthat relies on the fact that we currently execute preflight code inside a worker threadI'm not sure whether the second change is one that we want to keep but it's cool that it works.
Fixes #5418
Fixes #5420
Checklist
pr/e2e-fulllabel if this feature requires end-to-end testingBy submitting this pull request, I confirm that my contribution is made under the terms of the Wing Cloud Contribution License.