Skip to content

Commit 8c8163d

Browse files
zackeesclaude
andauthored
fix(fbuild-python): forward PLATFORMIO_SRC_DIR through build/deploy requests (#274) (#310)
The Rust CLI reads `PLATFORMIO_SRC_DIR` and forwards it into `BuildRequest`/`DeployRequest.src_dir`, but the PyO3 `DaemonConnection` and `AsyncDaemonConnection` request model omitted `src_dir` entirely. FastLED's autoresearch wrapper uses the Python binding, so the caller env override was lost and the daemon fell back to `platformio.ini`'s `src_dir = examples/Sailboat` instead of the requested `examples/AutoResearch`. Add `src_dir: Option<String>` to the shared `OpRequest` and a `platformio_src_dir_from_env()` helper that mirrors the CLI's `.ok().filter(|s| !s.is_empty())` contract. Both the sync and async `build_request`/`deploy_request` constructors now populate `src_dir` from the env var; monitor requests intentionally leave it `None` since the daemon doesn't compile during monitor. Python signatures are unchanged. Tests: - `platformio_src_dir_helper_*` exercise set / unset / empty env values. - `daemon_connection_*` and `async_daemon_connection_*` request constructors verify forwarding when set and omission when unset for both build and deploy. - `op_request_serializes_src_dir_only_when_set` guards the wire format so the daemon's `platformio.ini` fallback still kicks in when the env var is unset. Env-var tests share a `Mutex<()>` and an RAII guard (`PlatformioSrcDirGuard`) that holds the lock across set -> call -> assert -> restore, so cargo's default parallel test runner doesn't race them. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 1a4d661 commit 8c8163d

4 files changed

Lines changed: 269 additions & 7 deletions

File tree

crates/fbuild-python/src/async_daemon_connection.rs

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@
44
use pyo3::prelude::*;
55

66
use crate::outcome::{
7-
build_url, deploy_url, monitor_url, outcome_to_pydict, send_op_async, OpRequest,
7+
build_url, deploy_url, monitor_url, outcome_to_pydict, platformio_src_dir_from_env,
8+
send_op_async, OpRequest,
89
};
910

1011
/// Python-visible AsyncDaemonConnection class.
@@ -183,7 +184,7 @@ impl AsyncDaemonConnection {
183184
}
184185

185186
impl AsyncDaemonConnection {
186-
fn build_request(&self, clean: bool, verbose: bool) -> OpRequest {
187+
pub(crate) fn build_request(&self, clean: bool, verbose: bool) -> OpRequest {
187188
OpRequest {
188189
project_dir: self.project_dir.clone(),
189190
environment: Some(self.environment.clone()),
@@ -193,10 +194,11 @@ impl AsyncDaemonConnection {
193194
monitor_after: false,
194195
skip_build: false,
195196
baud_rate: None,
197+
src_dir: platformio_src_dir_from_env(),
196198
}
197199
}
198200

199-
fn deploy_request(
201+
pub(crate) fn deploy_request(
200202
&self,
201203
port: Option<String>,
202204
clean: bool,
@@ -212,6 +214,7 @@ impl AsyncDaemonConnection {
212214
monitor_after,
213215
skip_build,
214216
baud_rate: None,
217+
src_dir: platformio_src_dir_from_env(),
215218
}
216219
}
217220

@@ -225,6 +228,7 @@ impl AsyncDaemonConnection {
225228
monitor_after: false,
226229
skip_build: false,
227230
baud_rate,
231+
src_dir: None,
228232
}
229233
}
230234
}

crates/fbuild-python/src/daemon_connection.rs

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,10 @@
22
33
use pyo3::prelude::*;
44

5-
use crate::outcome::{build_url, deploy_url, monitor_url, outcome_to_pydict, send_op, OpRequest};
5+
use crate::outcome::{
6+
build_url, deploy_url, monitor_url, outcome_to_pydict, platformio_src_dir_from_env, send_op,
7+
OpRequest,
8+
};
69

710
/// Python-visible DaemonConnection (context manager).
811
#[pyclass]
@@ -121,7 +124,7 @@ impl DaemonConnection {
121124
}
122125

123126
impl DaemonConnection {
124-
fn build_request(&self, clean: bool, verbose: bool) -> OpRequest {
127+
pub(crate) fn build_request(&self, clean: bool, verbose: bool) -> OpRequest {
125128
OpRequest {
126129
project_dir: self.project_dir.clone(),
127130
environment: Some(self.environment.clone()),
@@ -131,10 +134,11 @@ impl DaemonConnection {
131134
monitor_after: false,
132135
skip_build: false,
133136
baud_rate: None,
137+
src_dir: platformio_src_dir_from_env(),
134138
}
135139
}
136140

137-
fn deploy_request(
141+
pub(crate) fn deploy_request(
138142
&self,
139143
port: Option<String>,
140144
clean: bool,
@@ -150,6 +154,7 @@ impl DaemonConnection {
150154
monitor_after,
151155
skip_build,
152156
baud_rate: None,
157+
src_dir: platformio_src_dir_from_env(),
153158
}
154159
}
155160

@@ -163,6 +168,7 @@ impl DaemonConnection {
163168
monitor_after: false,
164169
skip_build: false,
165170
baud_rate,
171+
src_dir: None,
166172
}
167173
}
168174
}

crates/fbuild-python/src/lib.rs

Lines changed: 229 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,10 +83,56 @@ fn _native(m: &Bound<'_, PyModule>) -> PyResult<()> {
8383
#[cfg(test)]
8484
mod tests {
8585
use crate::json_rpc::{extract_remote_json_rpc_response, wait_for_remote_json_rpc_response};
86-
use crate::outcome::{parse_outcome, send_op_async, OpRequest};
86+
use crate::outcome::{parse_outcome, platformio_src_dir_from_env, send_op_async, OpRequest};
8787
use crate::PYTHON_MODULE_VERSION;
88+
use std::sync::Mutex;
8889
use tokio::io::{AsyncReadExt, AsyncWriteExt};
8990

91+
/// Serializes tests that mutate `PLATFORMIO_SRC_DIR`.
92+
///
93+
/// `std::env::set_var` and `remove_var` mutate process-global state, so
94+
/// running env-var tests in parallel (cargo's default) creates races
95+
/// where one test sees another's value and the assertions flake. A
96+
/// single `Mutex` held across set → call → assert → restore keeps the
97+
/// env-mutating tests strictly serial without forcing the whole crate
98+
/// onto `--test-threads=1`.
99+
static PLATFORMIO_SRC_DIR_LOCK: Mutex<()> = Mutex::new(());
100+
101+
/// RAII guard that restores `PLATFORMIO_SRC_DIR` on drop.
102+
///
103+
/// Holds the env-var lock for its lifetime so concurrent env-var tests
104+
/// queue rather than race. The previous value is restored exactly as
105+
/// observed (including "unset") so tests don't leak state into siblings
106+
/// that run after them.
107+
struct PlatformioSrcDirGuard {
108+
_lock: std::sync::MutexGuard<'static, ()>,
109+
previous: Option<String>,
110+
}
111+
112+
impl PlatformioSrcDirGuard {
113+
fn acquire() -> Self {
114+
// PoisonError is fine: the guard exists purely to serialize
115+
// env-var access, and a poisoned mutex still serializes.
116+
let lock = PLATFORMIO_SRC_DIR_LOCK
117+
.lock()
118+
.unwrap_or_else(|e| e.into_inner());
119+
let previous = std::env::var("PLATFORMIO_SRC_DIR").ok();
120+
Self {
121+
_lock: lock,
122+
previous,
123+
}
124+
}
125+
}
126+
127+
impl Drop for PlatformioSrcDirGuard {
128+
fn drop(&mut self) {
129+
match &self.previous {
130+
Some(v) => std::env::set_var("PLATFORMIO_SRC_DIR", v),
131+
None => std::env::remove_var("PLATFORMIO_SRC_DIR"),
132+
}
133+
}
134+
}
135+
90136
/// `parse_outcome` must faithfully extract every field the daemon's
91137
/// `OperationResponse` populates so Python callers can branch on the
92138
/// specific failure mode (see FastLED/fbuild#18). If any field is
@@ -226,6 +272,7 @@ mod tests {
226272
monitor_after: false,
227273
skip_build: false,
228274
baud_rate: None,
275+
src_dir: None,
229276
}
230277
}
231278

@@ -318,4 +365,185 @@ mod tests {
318365
);
319366
});
320367
}
368+
369+
/// When `PLATFORMIO_SRC_DIR` is set, the helper must return its value
370+
/// verbatim. This is the env-read primitive both DaemonConnection
371+
/// surfaces use to populate `OpRequest.src_dir`, so FastLED's
372+
/// autoresearch override survives the Python -> daemon hop. See
373+
/// FastLED/fbuild#274.
374+
#[test]
375+
fn platformio_src_dir_helper_returns_value_when_set() {
376+
let _guard = PlatformioSrcDirGuard::acquire();
377+
std::env::set_var("PLATFORMIO_SRC_DIR", "examples/AutoResearch");
378+
assert_eq!(
379+
platformio_src_dir_from_env().as_deref(),
380+
Some("examples/AutoResearch")
381+
);
382+
}
383+
384+
/// When `PLATFORMIO_SRC_DIR` is unset, the helper must return `None`
385+
/// so `OpRequest.src_dir` stays `None` and the daemon falls back to
386+
/// `platformio.ini`'s configured `src_dir`. Mirrors the CLI's
387+
/// `.ok().filter(|s| !s.is_empty())` contract.
388+
#[test]
389+
fn platformio_src_dir_helper_returns_none_when_unset() {
390+
let _guard = PlatformioSrcDirGuard::acquire();
391+
std::env::remove_var("PLATFORMIO_SRC_DIR");
392+
assert_eq!(platformio_src_dir_from_env(), None);
393+
}
394+
395+
/// An empty `PLATFORMIO_SRC_DIR` (`""`) must be treated as unset, not
396+
/// forwarded as an empty string. The CLI uses the same `filter(|s|
397+
/// !s.is_empty())` rule and a stray empty value would tell the daemon
398+
/// to compile an empty directory.
399+
#[test]
400+
fn platformio_src_dir_helper_returns_none_when_empty() {
401+
let _guard = PlatformioSrcDirGuard::acquire();
402+
std::env::set_var("PLATFORMIO_SRC_DIR", "");
403+
assert_eq!(platformio_src_dir_from_env(), None);
404+
}
405+
406+
/// `DaemonConnection::build_request` must forward `PLATFORMIO_SRC_DIR`
407+
/// into `OpRequest.src_dir` so the daemon receives the override the
408+
/// caller set on the parent env, matching `fbuild-cli`'s `Build`
409+
/// request construction. Regression guard for FastLED/fbuild#274.
410+
#[test]
411+
fn daemon_connection_build_request_forwards_platformio_src_dir() {
412+
let _guard = PlatformioSrcDirGuard::acquire();
413+
std::env::set_var("PLATFORMIO_SRC_DIR", "examples/AutoResearch");
414+
let conn = crate::daemon_connection::DaemonConnection::new(
415+
"tests/platform/uno".into(),
416+
"uno".into(),
417+
);
418+
let req = conn.build_request(false, false);
419+
assert_eq!(req.src_dir.as_deref(), Some("examples/AutoResearch"));
420+
}
421+
422+
/// `DaemonConnection::deploy_request` must forward `PLATFORMIO_SRC_DIR`
423+
/// for the same reason `build_request` does — the issue's "Done"
424+
/// criteria explicitly call out deploy parity with the CLI.
425+
#[test]
426+
fn daemon_connection_deploy_request_forwards_platformio_src_dir() {
427+
let _guard = PlatformioSrcDirGuard::acquire();
428+
std::env::set_var("PLATFORMIO_SRC_DIR", "examples/AutoResearch");
429+
let conn = crate::daemon_connection::DaemonConnection::new(
430+
"tests/platform/uno".into(),
431+
"uno".into(),
432+
);
433+
let req = conn.deploy_request(None, false, false, false);
434+
assert_eq!(req.src_dir.as_deref(), Some("examples/AutoResearch"));
435+
}
436+
437+
/// When the env var is unset, `build_request` must leave `src_dir` as
438+
/// `None`. Omitting the field on the wire is what lets the daemon fall
439+
/// back to `platformio.ini`'s `src_dir`; a forwarded `Some("")` would
440+
/// break that fallback.
441+
#[test]
442+
fn daemon_connection_build_request_omits_src_dir_when_env_unset() {
443+
let _guard = PlatformioSrcDirGuard::acquire();
444+
std::env::remove_var("PLATFORMIO_SRC_DIR");
445+
let conn = crate::daemon_connection::DaemonConnection::new(
446+
"tests/platform/uno".into(),
447+
"uno".into(),
448+
);
449+
let req = conn.build_request(false, false);
450+
assert!(req.src_dir.is_none());
451+
}
452+
453+
/// Same omission guarantee for deploy. The CLI and Python paths must
454+
/// behave identically when the caller has not set
455+
/// `PLATFORMIO_SRC_DIR`.
456+
#[test]
457+
fn daemon_connection_deploy_request_omits_src_dir_when_env_unset() {
458+
let _guard = PlatformioSrcDirGuard::acquire();
459+
std::env::remove_var("PLATFORMIO_SRC_DIR");
460+
let conn = crate::daemon_connection::DaemonConnection::new(
461+
"tests/platform/uno".into(),
462+
"uno".into(),
463+
);
464+
let req = conn.deploy_request(None, false, false, false);
465+
assert!(req.src_dir.is_none());
466+
}
467+
468+
/// Async parity with the sync `build_request` forwarding check. The
469+
/// AsyncDaemonConnection is what FastLED uses under asyncio, so a
470+
/// regression here would surface the same wrong-sketch failure mode
471+
/// even after the sync path is fixed.
472+
#[test]
473+
fn async_daemon_connection_build_request_forwards_platformio_src_dir() {
474+
let _guard = PlatformioSrcDirGuard::acquire();
475+
std::env::set_var("PLATFORMIO_SRC_DIR", "examples/AutoResearch");
476+
let conn = crate::async_daemon_connection::AsyncDaemonConnection::new(
477+
"tests/platform/uno".into(),
478+
"uno".into(),
479+
);
480+
let req = conn.build_request(false, false);
481+
assert_eq!(req.src_dir.as_deref(), Some("examples/AutoResearch"));
482+
}
483+
484+
/// Async parity with the sync `deploy_request` forwarding check.
485+
#[test]
486+
fn async_daemon_connection_deploy_request_forwards_platformio_src_dir() {
487+
let _guard = PlatformioSrcDirGuard::acquire();
488+
std::env::set_var("PLATFORMIO_SRC_DIR", "examples/AutoResearch");
489+
let conn = crate::async_daemon_connection::AsyncDaemonConnection::new(
490+
"tests/platform/uno".into(),
491+
"uno".into(),
492+
);
493+
let req = conn.deploy_request(None, false, false, false);
494+
assert_eq!(req.src_dir.as_deref(), Some("examples/AutoResearch"));
495+
}
496+
497+
/// Async omission parity: with the env var unset, the async
498+
/// surface must also leave `src_dir` as `None` so the daemon's
499+
/// `platformio.ini` fallback still kicks in.
500+
#[test]
501+
fn async_daemon_connection_build_request_omits_src_dir_when_env_unset() {
502+
let _guard = PlatformioSrcDirGuard::acquire();
503+
std::env::remove_var("PLATFORMIO_SRC_DIR");
504+
let conn = crate::async_daemon_connection::AsyncDaemonConnection::new(
505+
"tests/platform/uno".into(),
506+
"uno".into(),
507+
);
508+
let req = conn.build_request(false, false);
509+
assert!(req.src_dir.is_none());
510+
}
511+
512+
/// Async omission parity for deploy.
513+
#[test]
514+
fn async_daemon_connection_deploy_request_omits_src_dir_when_env_unset() {
515+
let _guard = PlatformioSrcDirGuard::acquire();
516+
std::env::remove_var("PLATFORMIO_SRC_DIR");
517+
let conn = crate::async_daemon_connection::AsyncDaemonConnection::new(
518+
"tests/platform/uno".into(),
519+
"uno".into(),
520+
);
521+
let req = conn.deploy_request(None, false, false, false);
522+
assert!(req.src_dir.is_none());
523+
}
524+
525+
/// `OpRequest` serializes `src_dir` with `skip_serializing_if =
526+
/// "Option::is_none"`, so when the env var is unset the field must not
527+
/// appear in the JSON sent to the daemon. The daemon's
528+
/// `BuildRequest.src_dir` is `Option<String>` with `serde(default)`;
529+
/// omitting the field is the only way to get the platformio.ini
530+
/// fallback. A forwarded `null` would be equivalent here, but
531+
/// historical CLI traffic doesn't include the key at all so we keep
532+
/// parity.
533+
#[test]
534+
fn op_request_serializes_src_dir_only_when_set() {
535+
let mut req = sample_op_request();
536+
let json = serde_json::to_string(&req).unwrap();
537+
assert!(
538+
!json.contains("src_dir"),
539+
"src_dir must be omitted when None, got {json}"
540+
);
541+
542+
req.src_dir = Some("examples/AutoResearch".into());
543+
let json = serde_json::to_string(&req).unwrap();
544+
assert!(
545+
json.contains(r#""src_dir":"examples/AutoResearch""#),
546+
"src_dir must serialize verbatim when set, got {json}"
547+
);
548+
}
321549
}

crates/fbuild-python/src/outcome.rs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,30 @@ pub(crate) struct OpRequest {
2121
pub(crate) skip_build: bool,
2222
#[serde(skip_serializing_if = "Option::is_none")]
2323
pub(crate) baud_rate: Option<u32>,
24+
/// Override for `PLATFORMIO_SRC_DIR` — the source directory to compile.
25+
///
26+
/// Mirrors the `fbuild-cli` build/deploy paths which read the env var at
27+
/// request-construction time and forward it to the daemon, so consumers
28+
/// that go through the PyO3 binding (notably FastLED's autoresearch
29+
/// runner) get the same `src_dir` override the CLI provides. The
30+
/// daemon's `BuildRequest`/`DeployRequest` both honor a top-level
31+
/// `src_dir` field. See FastLED/fbuild#274.
32+
#[serde(skip_serializing_if = "Option::is_none")]
33+
pub(crate) src_dir: Option<String>,
34+
}
35+
36+
/// Read `PLATFORMIO_SRC_DIR` from the current process env, returning `None`
37+
/// when unset or empty.
38+
///
39+
/// Centralized so the sync and async `DaemonConnection`s populate
40+
/// `OpRequest.src_dir` identically to `fbuild-cli`
41+
/// (`std::env::var(...).ok().filter(|s| !s.is_empty())`). Keeping this in
42+
/// one place avoids drift between the two surfaces and gives tests a single
43+
/// seam to exercise.
44+
pub(crate) fn platformio_src_dir_from_env() -> Option<String> {
45+
std::env::var("PLATFORMIO_SRC_DIR")
46+
.ok()
47+
.filter(|s| !s.is_empty())
2448
}
2549

2650
pub(crate) fn build_url() -> String {

0 commit comments

Comments
 (0)