feat(rust): shared build directory

[pyranota]

Important

Introduces a shared build directory for Rust projects, enhancing caching and build efficiency, with updates to nsjail configurations and Dockerfiles.

• Behavior:
  • Introduces shared build directory for Rust in rust_executor.rs using get_build_dir().
  • Adds download.rust.config.proto for nsjail configuration.
  • Updates build_rust_crate() to use shared build directory.
  • Installs cargo-sweep in Dockerfiles to manage cache size.
• Configuration:
  • Adds CARGO_SWEEP_PATH to flake.nix and Dockerfiles.
  • Updates run.rust.config.proto and download.rust.config.proto for nsjail.
• Misc:
  • Adds DEV_CONF_NSJAIL for development-specific nsjail mounts.
  • Updates DockerfileFull, DockerfileFullEe, and DockerfileNsjail to include cargo-sweep.

^{This description was created by for 893878d. You can customize this summary. It will automatically update as commits are pushed.}

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	b7e5781
Status:	✅  Deploy successful!
Preview URL:	https://6d0834ee.windmill.pages.dev
Branch Preview URL:	https://rust-minor-update.windmill.pages.dev

View logs

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to 967ba80 in 2 minutes and 40 seconds. Click for details.

• Reviewed 552 lines of code in 7 files
• Skipped 0 files when reviewing.
• Skipped posting 10 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. backend/windmill-worker/nsjail/download.rust.config.proto:1

• Draft comment:
  Consider adding file-level documentation to explain the purpose of each mount and expected placeholder substitutions.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

2. backend/windmill-worker/nsjail/download.rust.config.proto:117

• Draft comment:
  Ensure the {DEV} placeholder is properly substituted in production to prevent accidental inclusion of development mounts.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the author to ensure that a placeholder is properly substituted in production. It is not a specific code suggestion or a request for a test, but rather a general reminder to check something. This violates the rule against asking the author to ensure behavior is intended or tested.

3. backend/windmill-worker/nsjail/run.rust.config.proto:108

• Draft comment:
  Verify that including the {DEV} placeholder in the run config is intentional and that it won’t alter production behavior.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify their intention regarding the inclusion of a placeholder in the run configuration. This falls under the rule of not asking the author to confirm their intention or ensure behavior is intended. Therefore, this comment should be removed.

4. backend/windmill-worker/src/rust_executor.rs:43

• Draft comment:
  The default SWEEP_MAXSIZE is set to '10GB'. Confirm that this default is appropriate for your resource constraints.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

5. backend/windmill-worker/src/rust_executor.rs:211

• Draft comment:
  The get_build_dir function creates a shared build directory. Ensure proper permissions and cleanup strategies are in place.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

6. backend/windmill-worker/src/rust_executor.rs:245

• Draft comment:
  Invoking 'cargo sweep' via the cargo binary assumes the tool is available. Consider using an explicit CARGO_SWEEP_PATH for clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% Using cargo sweep as a subcommand is actually the standard way to run cargo extensions. The cargo binary will look for executables named cargo-sweep in the PATH. The commented out CARGO_SWEEP_PATH suggests this was considered but rejected. Using CARGO_PATH is consistent with how other cargo commands are handled in this codebase. The comment raises a valid point about being explicit, but may not understand how cargo subcommands typically work. Using a separate path could actually be more confusing than the standard approach. While being explicit has merits, following cargo's standard subcommand pattern is more idiomatic and requires less configuration. The current approach is actually clearer to Rust developers. The comment should be deleted as it suggests a non-standard approach that would add complexity without clear benefits. The current implementation using CARGO_PATH follows cargo's conventions.

7. docker/DockerfileNsjail:46

• Draft comment:
  Rust version inconsistency: this Dockerfile uses rust:1.80.1 whereas others use rust:1.86.0. Consider aligning the Rust versions for consistency.
• Reason this comment was not posted:
  Comment was on unchanged code.

8. flake.nix:84

• Draft comment:
  Ensure that the cargo-sweep dependency in the flake remains in sync with the version installed in Docker images to prevent mismatches.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a speculative comment asking to "ensure" something. It's asking the author to verify something about Docker images that aren't even shown in this diff. The comment is making assumptions about Docker usage that we can't verify. This falls under the rule about not making "ensure that..." comments. Maybe version mismatches between environments could cause real problems that should be addressed. Maybe there's important context about Docker that I'm missing. Even if version syncing is important, this comment is still speculative and asks for verification rather than pointing out a concrete issue. If there's a specific version requirement, that should be stated directly. This comment should be deleted as it violates the rule against making speculative comments and asking authors to verify/ensure things.

9. flake.nix:220

• Draft comment:
  A hardcoded gcc version ('14.2.1') is used in BINDGEN_EXTRA_CLANG_ARGS. Consider retrieving the version dynamically to ensure compatibility.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

10. backend/windmill-worker/src/rust_executor.rs:56

• Draft comment:
  Typographical suggestion: Consider capitalizing 'nixos' to 'NixOS' in the comment (# Mount nix store for nixos to work properly) to properly reflect the proper noun.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While technically correct that NixOS is the proper capitalization, this is just a comment in code and doesn't affect functionality. The comment is clear and understandable either way. This seems like an extremely minor stylistic suggestion that doesn't meaningfully improve code quality. The proper capitalization could be considered part of maintaining professional code quality and attention to detail. Some would argue that even comments should maintain proper naming conventions. While proper capitalization is good, this level of nitpicking on internal comments doesn't provide enough value to justify the overhead of a PR comment and the subsequent change. Delete this comment as it's too minor and doesn't meaningfully improve code quality. The comment is perfectly understandable as is.

Workflow ID: wflow_8amovAye9zKppWu4

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 0655a5c in 1 minute and 55 seconds. Click for details.

• Reviewed 175 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 7 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. backend/windmill-worker/src/rust_executor.rs:23

• Draft comment:
  Reordering constants in the use block; no functional changes but consider grouping related items for clarity.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. backend/windmill-worker/src/rust_executor.rs:41

• Draft comment:
  SWEEP_MAXSIZE default increased from '10GB' to '25GB'; confirm this change aligns with resource management expectations.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

3. backend/windmill-worker/src/rust_executor.rs:201

• Draft comment:
  Removed mem_peak, canceled_by, and occupancy_metrics parameters from get_build_dir; ensure omitting these metrics does not impact later monitoring.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

4. backend/windmill-worker/src/rust_executor.rs:209

• Draft comment:
  When nsjail is disabled, a shared build directory based on worker_name is used; verify this doesn’t lead to cache collisions across jobs.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

5. backend/windmill-worker/src/rust_executor.rs:265

• Draft comment:
  Cargo sweep is now spawned asynchronously with dummy metric values; ensure that ignoring mem_peak and canceled_by here is acceptable.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. backend/windmill-worker/src/rust_executor.rs:310

• Draft comment:
  build_rust_crate now calls get_build_dir with fewer arguments; verify that the removal of occupancy metrics does not affect downstream processing.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

7. backend/windmill-worker/src/rust_executor.rs:216

• Draft comment:
  There is a typographical error in the comment on this line. "mainining" should be corrected to "maintaining".
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While this is a real typo that was introduced in the changed code, comments about typos in documentation are generally not important enough to warrant a PR comment. The typo doesn't affect functionality and is a very minor issue. Additionally, the PR rules specifically say to "NOT make purely informative comments" and to "NOT comment unless there is clearly a code change required." The typo does make the code slightly less professional and could be worth fixing. Documentation quality does matter. While documentation quality matters, this is too minor of an issue to warrant a PR comment. The author can fix it if they notice it, but it's not worth calling out specifically. Delete this comment as it's too minor of an issue and violates the rule about making purely informative comments.

Workflow ID: wflow_mtBVudXDaCRpuIcX

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 893878d in 1 minute and 34 seconds. Click for details.

• Reviewed 54 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. backend/windmill-worker/src/rust_executor.rs:209

• Draft comment:
  The refactored logic now nests the nsjail check inside job.runnable_path.as_ref(), which means if job.runnable_path is None the shared build directory (used when nsjail is disabled) is never selected. Previously, if *DISABLE_NSJAIL was true, it always returned a shared directory. Confirm that this change is intentional.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment correctly identifies a change in behavior, but is it a problem? The code comments indicate that shared build directories are meant for caching and security. If runnable_path is None, it means we don't have path information to properly scope the cache, so defaulting to a random build dir seems appropriate from a security perspective. The code appears to be working as intended. I could be wrong about the intended behavior. Maybe there's a valid use case for having a shared build directory even when runnable_path is None that I'm not considering. The code comments clearly explain the security implications and caching tradeoffs. Without a runnable_path, we can't properly scope the cache, so falling back to a random build dir aligns with the documented security model. Delete the comment. While it correctly identifies a behavior change, the new behavior appears intentional and aligns with the documented security model. The comment is asking for confirmation rather than identifying a clear issue.

2. backend/windmill-worker/src/rust_executor.rs:223

• Draft comment:
  Typo: 'mainining' should be corrected to 'maintaining' for clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_rE1sp6HL5TyPQij4

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}