Skip to content

build(bottlecap): allow cargo build on Windows#1193

Closed
lucaspimentel wants to merge 13 commits into
mainfrom
lpimentel/allow-build-on-windows
Closed

build(bottlecap): allow cargo build on Windows#1193
lucaspimentel wants to merge 13 commits into
mainfrom
lpimentel/allow-build-on-windows

Conversation

@lucaspimentel
Copy link
Copy Markdown
Member

@lucaspimentel lucaspimentel commented Apr 21, 2026

Summary

Enables cargo build, cargo clippy, and cargo test to succeed on x86_64-pc-windows-msvc for local development, with zero change to Linux build artifacts or runtime behavior. Useful for editing bottlecap from Windows without spinning up WSL or a VM.

What changed

Build system:

  • nix and libddwaf dependencies are now target-conditional (Unix-only). Both have Unix-only build scripts that panic on Windows.
  • On Windows, appsec resolves to a no-op stub (bottlecap/src/appsec_stub.rs) that mirrors the public API used outside the module, so callers in main.rs, traces/trace_processor.rs, traces/trace_agent.rs, and proxy/interceptor.rs compile unchanged. The stub closely mirrors the real module: matching derives on Processor/Context/Error, pub(crate) visibility on hold_trace, and Processor::new returns Error::Unavailable rather than a phantom processor.
  • *.sh / *.bash pinned to LF line endings so scripts checked out on Windows still execute inside Linux Docker containers.

Code cleanups surfaced by Windows builds:

  • nix imports in proc/clock.rs and metrics/enhanced/statfs.rs are #[cfg]-gated to match already-gated function bodies.
  • Body::reader marked #[cfg_attr(target_os = "windows", allow(dead_code))] since its only callers are inside the stubbed-out appsec module.
  • statfs_info Windows stub: prefix unused parameter with _ and switch to io::Error::other per clippy::io_other_error.

Test fixes:

  • tests/appsec_processor_test.rs gated off Windows since it drives the real libddwaf-backed API which is not present in the stub.
  • The three tags::lambda::tags::tests::test_resolve_* tests switched from hard-coded Linux temp paths to std::env::temp_dir() to avoid a bootstrap race on Windows. Safer on Linux too (temp path no longer shared via filesystem side effects between tests). Paths are rendered with to_string_lossy to tolerate non-UTF-8 temp directories, and test_resolve_runtime now cleans up the file it creates.
  • proc::tests::test_get_cpu_data gated off Windows since it assumes SC_CLK_TCK = 100, which is Linux-specific.

What did not change

  • No runtime behavior on Linux/Lambda (the only supported deployment target).
  • No CI jobs added. Windows remains unsupported for CI.
  • Go extension, build scripts, publishing pipelines: untouched.

Testing

Manually validated on x86_64-pc-windows-msvc from a clean state:

  • cargo build — success

  • cargo clippy --workspace — success, no warnings

  • cargo fmt --all -- --check — success

  • cargo test --lib511 passed, 0 failed

  • cargo test --no-run — all integration test binaries compile

  • validate cargo build / cargo clippy / cargo test on Linux before merging. No regressions expected because every gate is #[cfg(not(target_os = "windows"))] or #[cfg_attr(target_os = "windows", ...)], so Linux code paths and the dependency graph should be byte-identical to before, but this has not been verified by me directly.

Out of scope

  • Runtime behavior on Windows is undefined. appsec and filesystem metrics are no-ops. This is explicitly acceptable, the goal is "build for local development."
  • No Windows CI job added.

"Turns out making a Linux-only Lambda extension build on Windows is mostly a matter of politely asking libddwaf to sit this one out." — Claude 🤖

Ensures *.sh and *.bash files are checked out with LF line endings on
Windows, so they execute correctly inside Linux Docker containers.
nix is a Unix-only crate; gate its use statements and move it to a
target-conditional dependency so cargo build can resolve on Windows.
Linux build output is unchanged.
Substitute a no-op stub for the appsec module on Windows and gate
libddwaf behind a target predicate so the Unix-only libddwaf-sys build
script no longer runs on Windows. Linux build, clippy output, and
runtime behavior are unchanged.
- metrics/enhanced/statfs.rs: prefix unused param with _ and use
  io::Error::other per the clippy::io_other_error lint.
- lifecycle/invocation/triggers/body.rs: allow dead_code on Body::reader
  when targeting Windows (its only callers are inside the stubbed-out
  appsec module).
The integration test drives the real libddwaf-backed Processor API
(with_capacity, libddwaf::get_version, etc.) which is not present in
the Windows stub. Gate the file so cargo test --no-run compiles on
Windows.
The three resolve_runtime / resolve_provided tests hard-coded a Linux
temp path that does not exist by default on Windows, causing them to
race on a fresh machine depending on which test triggered the directory
creation first. std::env::temp_dir returns a directory that always
exists on every supported platform, removing the bootstrap race.
get_cpu_data parses Linux's /proc/stat and converts ticks to
milliseconds using sysconf(SC_CLK_TCK), which is meaningless on
Windows. The test's expected values assume CLK_TCK=100 (Linux) and
cannot pass on Windows without turning the conversion into nonsense.
@lucaspimentel lucaspimentel marked this pull request as ready for review April 21, 2026 02:01
@lucaspimentel lucaspimentel requested a review from a team as a code owner April 21, 2026 02:01
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Enables the bottlecap crate to compile (and run most tests) on Windows by making Unix-only dependencies and AppSec functionality target-conditional, while keeping Linux behavior unchanged.

Changes:

  • Makes nix/libddwaf Unix-only dependencies and introduces a Windows appsec no-op stub module.
  • Adjusts a few Unix-specific imports and Windows-incompatible tests via #[cfg(...)] gates.
  • Improves cross-platform test path handling by using std::env::temp_dir() and pins shell scripts to LF line endings via .gitattributes.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
bottlecap/Cargo.toml Moves nix and libddwaf behind a non-Windows target dependency section.
bottlecap/src/lib.rs Selects real appsec vs appsec_stub.rs based on target OS.
bottlecap/src/appsec_stub.rs Adds Windows no-op appsec API surface so callers compile unchanged.
bottlecap/tests/appsec_processor_test.rs Disables AppSec integration test on Windows where libddwaf-backed API isn’t present.
bottlecap/src/tags/lambda/tags.rs Updates tests to use temp_dir() instead of hard-coded /tmp/... paths.
bottlecap/src/proc/mod.rs Gates a Linux-specific proc test off Windows.
bottlecap/src/proc/clock.rs Gates nix import to match existing non-Windows code.
bottlecap/src/metrics/enhanced/statfs.rs Gates Unix-only imports and implements a Windows stub error path for statfs_info.
bottlecap/src/lifecycle/invocation/triggers/body.rs Allows Body::reader to be dead-code on Windows (only used by AppSec).
.gitattributes Forces LF endings for *.sh/*.bash so Docker execution works with Windows checkouts.

Comment thread bottlecap/src/appsec_stub.rs Outdated
Comment thread bottlecap/src/appsec_stub.rs Outdated
Comment thread bottlecap/src/metrics/enhanced/statfs.rs
Comment thread bottlecap/src/tags/lambda/tags.rs Outdated
Comment thread bottlecap/src/tags/lambda/tags.rs Outdated
Comment thread bottlecap/src/appsec_stub.rs Outdated
lucaspimentel and others added 6 commits April 20, 2026 22:41
Remove Copy/Clone from stub Processor, Context, and Error, and change
hold_trace visibility to pub(crate) to match the real appsec module.

🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
The function is only used internally; removing pub aligns visibility
across platforms.

🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
Avoids a potential panic when the temp directory path contains
non-UTF-8 characters.

🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
- Scope clippy::needless_pass_by_value to hold_trace only (was file-wide)
- Move assert_eq! before cleanup in test_resolve_runtime for clearer failure messages

🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
@duncanista
Copy link
Copy Markdown
Contributor

duncanista commented Apr 23, 2026

Lucas, my brother in Christ, this is a Lambda extension. It runs on Linux. In Linux containers. On Linux servers. In the Linux cloud. The fact that you needed 500+ lines of cfg-gating and a whole appsec stub module just to get cargo build to stop crying on Windows is not a PR — it's a cry for help. 🍎 is right there man.

- sent by @claude

Copy link
Copy Markdown
Contributor

@duncanista duncanista left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Thanks for the thorough PR description, Lucas — the level of detail here is appreciated. A few of the changes in this PR are genuinely good improvements on their own (test fixes, .gitattributes). However, we have concerns about the overall approach of adding Windows compilation support through cfg gates and a hand-written stub module. Requesting changes so we can discuss the tradeoffs before this merges.


What works well (should merge independently)

  • .gitattributes LF enforcement for *.sh / *.bash — This is useful for everyone regardless of Windows support. Shell scripts with CRLF break inside Docker/CI.
  • Test path improvements — Replacing hardcoded /tmp paths with std::env::temp_dir() and adding cleanup is a real improvement on Linux too. Eliminates shared filesystem side effects between tests.
  • statfs_info visibility tightening (pub fnfn) and io::Error::other() modernization — straightforward cleanups.

We'd happily take these as a separate PR.


Concerns with the Windows cfg-gating approach

1. The stub is a maintenance hazard with no CI safety net

appsec_stub.rs manually replicates 87 lines of API surface: Processor, Context, HoldArguments (14 fields), and Error — all hand-matched to the real module. Since CI runs exclusively on Linux, the stub is never compiled or tested in CI. When someone modifies the real appsec module (adds a field to HoldArguments, changes a method signature, adds a public type), Linux CI passes and the stub silently drifts. Nobody finds out until someone tries building on Windows again, which could be months later. At that point, the fix requires archaeology to figure out what diverged.

2. cfg(target_os = "windows") gates hurt readability for all developers

This is a Linux-only Lambda extension. Every developer reading this code will encounter #[cfg(not(target_os = "windows"))] annotations on imports, functions, and modules, and will have to mentally filter them out. The #[cfg_attr(target_os = "windows", allow(dead_code))] on Body::reader is a good example — it's meaningless noise for the 100% of production and CI builds that target Linux. These gates are also viral: every future Unix-only addition (new nix usage, new /proc parsing, new Linux syscall) needs a corresponding Windows gate, and forgetting to add one breaks the Windows build that nobody's CI checks.

3. The Cargo.toml change is subtly fragile

libddwaf now falls under [target.'cfg(not(target_os = "windows"))'.dependencies] because the new section header was inserted above it. This is a TOML positional dependency — anyone adding a new dep between datadog-fips and libddwaf could accidentally move libddwaf back into unconditional [dependencies], or someone reordering deps could break the section boundary. The fips feature's "libddwaf/fips" reference silently becomes a no-op on Windows, which is correct but non-obvious.


Tradeoffs

Windows cfg gates (this PR) WSL2 / Dev Container
Code changes 10 files, 123 additions 0 production files (devcontainer: 1 config file)
CI coverage Stub is never tested in CI N/A — no stub to test
Ongoing maintenance Every Unix-only addition needs a Windows gate; stub must be manually kept in sync None
Readability cfg annotations scattered across imports, functions, modules No noise in production code
Dev setup cost Zero for the Windows developer WSL2: ~10 min one-time setup; devcontainer: single click
Drift risk High — silent divergence between stub and real module None
Benefit scope Anyone building natively on Windows Anyone on any OS (devcontainers)

The core question is whether the convenience of native Windows cargo build justifies modifying production code, dependency resolution, and module structure for a platform that is explicitly out of scope for both runtime and CI. WSL2 is the standard answer for "I need Linux tooling on Windows" — it gives a real Linux kernel with near-native performance and full toolchain support. A .devcontainer/devcontainer.json would solve this for all non-Linux developers with a single config file and zero production code impact.


Recommendation

  1. Split out the universally-good changes (.gitattributes, test path fixes, statfs_info visibility, io::Error::other()) into their own PR — these should merge now.
  2. Reconsider the Windows compilation approach. If native Windows builds are truly needed beyond what WSL2/devcontainers provide, a Cargo feature flag (#[cfg(feature = "appsec")]) would be preferable to target_os gates — it can be tested on Linux in CI, is explicit, and doesn't scatter platform annotations throughout the codebase.

@lucaspimentel
Copy link
Copy Markdown
Member Author

  • Split out the universally-good changes
  • Reconsider the Windows compilation approach.

@duncanista tell your AI 😅 that I think this makes sense, and thanks for taking the time to review this.

@lucaspimentel
Copy link
Copy Markdown
Member Author

I split out the general cleanup into #1211. We can come up with a better design for a Windows build later. Thanks!

lucaspimentel added a commit that referenced this pull request Apr 24, 2026
## Overview

Portable test and code cleanups in bottlecap. No behavior change on
Linux.

- `.gitattributes`: force LF for `*.sh` / `*.bash`. Shell scripts now
check out with LF on every platform, so a CRLF-prone checkout (e.g.
Windows) still executes cleanly inside Linux Docker containers.
- `bottlecap/src/tags/lambda/tags.rs`, the three
`tags::lambda::tags::tests::test_resolve_*` tests:
- Switch from a hard-coded `/tmp/...` path to `std::env::temp_dir()`.
Removes a bootstrap race between tests that were implicitly sharing a
filesystem side effect through the same path.
- Render paths with `to_string_lossy()` so a non-UTF-8 temp dir does not
panic the test.
  - `test_resolve_runtime` now cleans up the file it created.
- `bottlecap/src/metrics/enhanced/statfs.rs`:
- `statfs_info` is `fn` on both targets. It's an internal helper; `pub`
was only present on the non-Windows branch.
- The existing `#[cfg(target_os = "windows")]` stub now uses
`io::Error::other(..)` (per `clippy::io_other_error`) and prefixes its
unused `path` with `_`.

> Origin: these commits were split out of #1193 after review feedback,
so the portable cleanups can land independently of the broader
Windows-build discussion.

## Testing

- `cargo check` on Linux: clean.
- `cargo test --lib tags::lambda::tags::`: 8 passed, 0 failed.

> *"Turns out 'make the tests not race on Windows' is also 'make the
tests not race on Linux'."*, Claude 🤖
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants