feat(worker): support workers to run natively on windows [WIP] #4446
Conversation
Deploying windmill with
|
| Latest commit: |
2f80ba1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0a4ad7a6.windmill.pages.dev |
| Branch Preview URL: | https://alp-build-windows.windmill.pages.dev |
rubenfiszel
force-pushed
the
main
branch
3 times, most recently
from
ec6b974
to
8e0eb3d
Compare
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to e81555a in 1 minute and 0 seconds
More details
- Looked at
1134lines of code in13files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/windmill-worker/src/rust_executor.rs:325
- Draft comment:
On Windows,std::os::windows::fs::symlink_diris used for creating symlinks, but this is intended for directories. Ifbin_pathis a file, consider usingstd::os::windows::fs::symlink_fileinstead. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_In6YX0ACdBVkNh5t
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()); | ||
|
|
||
| build_rust_cmd.args(vec!["build", "--release"]); |
There was a problem hiding this comment.
looks like it could be kept above but not important
There was a problem hiding this comment.
❌ Changes requested. Incremental review on fb81321 in 49 seconds
More details
- Looked at
326lines of code in5files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. backend/windmill-worker/src/go_executor.rs:231
- Draft comment:
On Windows, usestd::os::windows::fs::symlink_fileinstead ofsymlink_dirfor file symlinks.
let symlink = std::os::windows::fs::symlink_file(&bin_path, &target);
- Reason this comment was not posted:
Marked as duplicate.
2. backend/windmill-worker/src/rust_executor.rs:322
- Draft comment:
On Windows, usestd::os::windows::fs::symlink_fileinstead ofsymlink_dirfor file symlinks.
let symlink = std::os::windows::fs::symlink_file(&bin_path, &target);
- Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_fG6vWInrj6hnlau8
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| #[cfg(windows)] | ||
| { | ||
| target = format!("{job_dir}\\main.js"); | ||
| symlink = std::os::windows::fs::symlink_dir(&local_path, &target); |
There was a problem hiding this comment.
On Windows, use std::os::windows::fs::symlink_file instead of symlink_dir for file symlinks.
| symlink = std::os::windows::fs::symlink_dir(&local_path, &target); | |
| symlink = std::os::windows::fs::symlink_file(&local_path, &target); |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on ceb221a in 39 seconds
More details
- Looked at
54lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. .github/workflows/build_windows_worker.yml:1
- Draft comment:
Consider renaming the workflow to something more descriptive, like 'Build Windows Worker', to better reflect its purpose. - Reason this comment was not posted:
Confidence changes required:50%
The workflow is named 'Backend only integration tests', but it seems to be focused on building for Windows. The name should reflect its purpose more accurately.
2. .github/workflows/build_windows_worker.yml:18
- Draft comment:
Verify if 'ubicloud-standard-8' is the intended runner. If not, consider using a standard GitHub runner like 'windows-latest'. - Reason this comment was not posted:
Confidence changes required:50%
The 'runs-on' key specifies 'ubicloud-standard-8', which is not a standard GitHub runner. This might be a custom runner, but it's worth verifying if this is intentional.
3. .github/workflows/build_windows_worker.yml:44
- Draft comment:
Verify if 'mkdir frontend/build' is necessary for the backend build process. If not, consider removing it. - Reason this comment was not posted:
Confidence changes required:50%
The 'cargo build windows' step uses 'mkdir frontend/build', but this seems unrelated to the backend build process. Verify if this is necessary.
Workflow ID: wflow_JsucE3BSawNqEpzR
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| types: [opened, synchronize, reopened] | ||
| paths: | ||
| - "backend/**" | ||
| - ".github/workflows/backend-test.yml" |
There was a problem hiding this comment.
The path should be '.github/workflows/build_windows_worker.yml' to match the file name.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b6f8d46 in 26 seconds
More details
- Looked at
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build_windows_worker.yml:44
- Draft comment:
The workflow uses a Linux-based container image (ghcr.io/windmill-labs/backend-tests). Consider using a Windows-based image for building Windows workers. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_53WrywaplBKfzOBz
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 3b02fbb in 30 seconds
More details
- Looked at
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_XzhGeFehRGtTL66W
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| timeout-minutes: 15 | ||
| run: | ||
| mkdir frontend/build && cd backend && touch | ||
| windmill-api/openapi-deref.yaml && apt-get install gcc-mingw-w64 && rustup target add x86_64-pc-windows-gnu && |
There was a problem hiding this comment.
The use of apt-get install is incorrect here for a Windows build. Consider using a Windows-compatible package manager like choco or ensure the environment is set up correctly for Windows builds.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on b3dad9e in 38 seconds
More details
- Looked at
22lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_8Bv0W8yypIAb6tcK
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| - ".github/workflows/backend-test.yml" | ||
|
|
||
| jobs: | ||
| cargo_build_windos: |
There was a problem hiding this comment.
There's a typo in the job name cargo_build_windos. It should be cargo_build_windows.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 9b84cb9 in 26 seconds
More details
- Looked at
22lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build_windows_worker.yml:17
- Draft comment:
The job name contains a typo. It should becargo_build_windowsinstead ofcargo_build_windos. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment points out a typo that has already been corrected in the diff. Since the change has been made, the comment is no longer necessary. The comment is about a change that has already been resolved.
I might be missing the context of whether the comment was made before or after the change. However, the diff clearly shows the correction, indicating the comment is outdated.
The diff provides clear evidence that the typo has been corrected, so the comment is no longer relevant.
The comment should be deleted because the typo it points out has already been corrected in the diff.
Workflow ID: wflow_K6Izc4zjA008dJBu
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 11e8ca7 in 24 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build_windows_worker.yml:45
- Draft comment:
Usingapt-get installin a Windows build workflow might not be appropriate. Consider using a Windows-compatible package manager or ensure the container supports this command. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_SqQQLrEsLgSeImXE
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5102e0a in 20 seconds
More details
- Looked at
45lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/windmill-worker/src/bash_executor.rs:456
- Draft comment:
The default value forTMPshould be a Windows-style path, such asC:\Temp, instead of/tmp. This issue is also present inhandle_bash_job. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_Z4GvrsKCA9OIlYpW
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 01d7c27 in 19 seconds
More details
- Looked at
57lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build_windows_worker.yml:26
- Draft comment:
Thecontainerkey is not supported on Windows runners in GitHub Actions. Consider removing it or using a different approach for Windows builds. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_SnUEFK4EKp3NlnFK
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e49ba39 in 17 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build_windows_worker.yml:23
- Draft comment:
The removal of the container section is appropriate for running natively on Windows. - Reason this comment was not posted:
Confidence changes required:0%
The removal of the container section is correct since the goal is to run natively on Windows without Docker.
Workflow ID: wflow_6b4xvv0OaOmX04fO
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d5f3e75 in 19 seconds
More details
- Looked at
21lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build_windows_worker.yml:23
- Draft comment:
TheDATABASE_URLenvironment variable references a PostgreSQL service that has been removed from the workflow. Ensure this variable is necessary for the build process or update it accordingly. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_3LcQCQjOMoAL6U3j
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d1b5abd in 17 seconds
More details
- Looked at
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build_windows_worker.yml:30
- Draft comment:
Ensuretype NUL > windmill-api/openapi-deref.yamlis intended for Windows environments only, as it is a Windows-specific command. - Reason this comment was not posted:
Confidence changes required:33%
The command to create a file usingtype NULis a Windows-specific command. Ensure that this is intended for Windows environments only.
Workflow ID: wflow_4itKqPDQx9KtNWl6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2f80ba1 in 16 seconds
More details
- Looked at
11lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build_windows_worker.yml:30
- Draft comment:
Ensure the directory exists before creating the file to avoid errors. Consider usingNew-Item -ItemType Directory -Forcefor the directory. - Reason this comment was not posted:
Confidence changes required:50%
The use ofNew-Itemin the PowerShell command is correct for creating a file, but the command should ensure the directory exists before creating the file.
Workflow ID: wflow_oncwQWEWXvqXBYdr
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
The goal was to be able to build and run workers on windows without the need of docker or WSL.
As a first step the worker should support:
Trying to keep code changes to a minimum but includes some minor automatic
rustfmtNotes:
Important
Add Windows support for workers by updating executors and environment configurations for compatibility.
ansible_executor.rs,bash_executor.rs,bun_executor.rs,go_executor.rs,python_executor.rs, andrust_executor.rs.cfg(windows)to conditionally compile Windows-specific code.SystemRootand other environment variables for Windows in executors.handle_powershell_job()inbash_executor.rsto support Windows paths and environment.handle_bun_job()inbun_executor.rsto handle Windows-specific paths and environment variables.handle_python_job()inpython_executor.rsfor Windows compatibility.handle_rust_job()inrust_executor.rsto set Windows-specific environment variables.SYSTEM_ROOTconstant for Windows inworker.rs.build_windows_worker.ymlGitHub Actions workflow for building Windows workers.This description was created by
for 2f80ba1. It will automatically update as commits are pushed.