feat(worker): support workers to run natively on windows #4446
Conversation
Deploying windmill with
|
| Latest commit: |
bbe6b0b
|
| Status: | ⚡️ Build in progress... |
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.
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.
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.
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.
👍 Looks good to me! Incremental review on 4c002d2 in 21 seconds
More details
- Looked at
16lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build_windows_worker.yml:46
- Draft comment:
Usingbashas the shell on a Windows runner may cause compatibility issues. Consider usingpwshorcmdfor better compatibility with Windows environments. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The change from a PowerShell script to a bash script on a Windows runner is significant and could indeed cause compatibility issues. The comment is directly related to the change made in the diff, and it provides a clear suggestion to use a more compatible shell. This aligns with the rules for actionable and clear code quality refactors.
I might be overestimating the compatibility issues with usingbashon Windows, as GitHub Actions does support bash on Windows runners. The comment might not be necessary if the change is intentional and tested.
Even ifbashis supported, the comment highlights a potential issue that could arise from this change, which is valuable information for the author to consider.
Keep the comment as it provides a valid concern about compatibility issues with usingbashon Windows, which is directly related to the change made in the diff.
Workflow ID: wflow_Au5GMYdcWJ9CtwDO
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 a673a96 in 20 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:54
- Draft comment:
Consider separating themkdirandcdcommands to ensurecdis executed only ifmkdirsucceeds. This can prevent potential issues ifmkdirfails. - Reason this comment was not posted:
Confidence changes required:50%
The use ofmkdir frontend/build && cd backendin a single line can be problematic ifmkdirfails. It would be better to separate these commands to ensurecdis executed only ifmkdirsucceeds.
Workflow ID: wflow_IZfqRs7wjMIpIzls
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 e536dd7 in 27 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:48
- Draft comment:
Consider enabling thepull_requesttrigger to ensure that changes are tested in PRs before merging. This helps catch issues early. - Reason this comment was not posted:
Confidence changes required:50%
The commented-out pull_request trigger should be enabled for better CI/CD practices.
Workflow ID: wflow_vDXoafjYrHSnNbNq
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 45dc9cd in 41 seconds
More details
- Looked at
40lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build_windows_worker.yml:52
- Draft comment:
Remove commented-out sections if they are not needed to keep the workflow clean and maintainable. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about a change made in the diff, specifically the commented-out sections. Removing unnecessary commented-out code can improve code readability and maintainability. The comment is actionable and clear, suggesting a code quality improvement.
The comment assumes that the commented-out sections are not needed without knowing the author's intention. There might be a reason for keeping them commented out temporarily.
The comment is about code quality and maintainability, which is generally a good practice. If the sections are not needed, removing them is beneficial.
The comment is valid as it addresses a change made in the diff and suggests a clear code quality improvement. It should be kept.
Workflow ID: wflow_Y5jaIla3MBKBaX7U
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 9eeea89 in 25 seconds
More details
- Looked at
49lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build_windows_worker.yml:5
- Draft comment:
Ensure that triggering on tags only is the intended behavior, as this will not trigger on branch pushes or pull requests. - Reason this comment was not posted:
Confidence changes required:50%
The workflow is triggered on any push with a tag matching 'v*'. This is a common pattern for release workflows, but it's important to ensure that this is the intended behavior, as it will not trigger on branch pushes or pull requests.
Workflow ID: wflow_Tb9MoMasBbNFuULZ
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.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 493e149 in 25 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_IciH1Qx7ZPMSs126
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 2a3dd20 in 20 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/windmill-worker/src/handle_child.rs:9
- Draft comment:
Apply#[cfg(windows)]to theuse std::process::Stdio;line for consistency.
#[cfg(windows)]
use std::process::Stdio;
- Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_KMBgb44Z9YVQUP6O
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 965c8b7 in 21 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_jmbBrgeOrEHdhExt
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 7a0da5e in 24 seconds
More details
- Looked at
29lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/windmill-worker/src/rust_executor.rs:35
- Draft comment:
TheCARGO_PATHis constructed using Unix-style paths. Consider usingCARGO_PATH_DEFAULTfor Windows to ensure compatibility. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment seems to be addressing a concern that was part of the previous implementation, whereCARGO_PATH_DEFAULTwas used. However, the diff shows thatCARGO_PATH_DEFAULTwas removed, andCARGO_PATHis now constructed usingCARGO_HOME. This suggests a deliberate change in how paths are managed, possibly to unify the approach across platforms. The comment may not be applicable to the new implementation.
I might be missing the specific reason whyCARGO_PATH_DEFAULTwas removed and whether the new approach indeed covers all compatibility concerns. The comment could be highlighting a potential oversight in the new implementation.
The removal ofCARGO_PATH_DEFAULTand the new construction ofCARGO_PATHusingCARGO_HOMEindicates a conscious decision to change the path handling. Without evidence of an issue in the new approach, the comment seems outdated.
The comment is likely outdated due to the changes in path handling. The new implementation suggests a unified approach, making the comment aboutCARGO_PATH_DEFAULTunnecessary. The comment should be deleted.
Workflow ID: wflow_qu3ZLc11jq1qPdC7
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
rubenfiszel
changed the title
| @@ -540,6 +557,7 @@ pub async fn pull_codebase(w_id: &str, id: &str, job_dir: &str) -> Result<()> { | |||
| if is_tar { | |||
| extract_tar(fs::read(bun_cache_path)?.into(), job_dir).await?; | |||
| } else { | |||
| #[cfg(unix)] | |||
There was a problem hiding this comment.
missing a cp on windows
| #[cfg(unix)] | ||
| tokio::fs::symlink(bun_cache_path, dst).await?; |
There was a problem hiding this comment.
missing a cp on windows
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on fdf0b01 in 36 seconds
More details
- Looked at
61lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/windmill-worker/src/bun_executor.rs:564
- Draft comment:
On Windows, usestd::os::windows::fs::symlink_filefor file symlinks instead ofsymlink_dir. This is also applicable at line 581. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests a change fromsymlink_dirtosymlink_filefor file symlinks on Windows. The code usessymlink_dir, which is typically for directories, not files. The context of the code indicates that the symlink is for a cache path, which might be a directory. The comment might be suggesting a change that doesn't align with the intended use of the symlink.
I might be missing the context of whetherbun_cache_pathis a file or a directory. If it's a directory, the comment is incorrect. If it's a file, the comment is correct.
The naming and typical usage of cache paths suggest they are directories. Without explicit evidence thatbun_cache_pathis a file, the comment seems speculative.
The comment should be deleted because it lacks strong evidence and seems speculative about the nature ofbun_cache_path.
Workflow ID: wflow_zDxDlUI5oSBTanDT
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
@rubenfiszel addressed your comments and carefully reviewed impact on windows. The biggest risk are the simplifications of the RUST env vars but they look good but (most of) the rest is behind feature flags. also we can merge this PR first #4448 |
Co-authored-by: Ruben Fiszel <ruben@windmill.dev>
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.
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 fdf0b01. It will automatically update as commits are pushed.