feat: Better OS Detection, starting with windows #6046
Conversation
|
It's possible to add WSL detection ? I make my code in plaform.rs, but it's better to centralize in your |
|
@Waxime64 sure, and the OnceLock is a good call as well |
dylan-hurd-oai
force-pushed
the
dh--env-context-windows
branch
from
ce04fe5 to
78aafa4
Compare
| let info = info.expect("expected WSL operating system info"); | ||
| assert_eq!(info.name, "Windows Subsystem for Linux"); | ||
| assert_eq!(info.version, ""); | ||
| assert_eq!(info.is_likely_windows_subsystem_for_linux, Some(true)); |
There was a problem hiding this comment.
why do we need both name and is_likely_windows_subsystem_for_linux ?
There was a problem hiding this comment.
Updated this - should be os_info.name, not "Windows Subsystem for Linux" 🤦
When running inside WSL, the os_info will return a linux distribution, e.g. Ubuntu. But it's still helpful to know that this is running inside WSL and the actual machine you're on is a Windows machine.
| if has_wsl_env_markers() { | ||
| let info = info.expect("expected WSL operating system info"); | ||
| assert_eq!(info.name, "Windows Subsystem for Linux"); | ||
| assert_eq!(info.version, ""); |
There was a problem hiding this comment.
what decisions to do we expect model to make better when knowing the version?
dylan-hurd-oai
force-pushed
the
dh--env-context-windows
branch
from
704a9a6 to
66858e4
Compare
| // Diff messages should only include fields that changed between turns. | ||
| // Operating system is a static property of the host and should not be | ||
| // emitted as part of a per-turn diff. | ||
| let mut ec = EnvironmentContext::new(cwd, approval_policy, sandbox_policy, None); |
There was a problem hiding this comment.
nit: this feels a bit awkward. Something like EnvironmentContext::init_without_os(...) would be nice
| Some(OperatingSystemInfo { | ||
| name: info.os_type().to_string(), | ||
| version: info.version().to_string(), | ||
| is_likely_windows_subsystem_for_linux: Some(has_wsl_env_markers()), |
There was a problem hiding this comment.
is this right? If they are running WSL, wouldn't target_os not be windows?
There was a problem hiding this comment.
You can technically execute codex.exe from within WSL, since WSL supports interop for executables. I think we should generally discourage this, since many things won't work, but it is possible.
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| fn has_wsl_env_markers() -> bool { |
There was a problem hiding this comment.
would be nice to have one version of this (also this one doesn't cache the output?)
Summary
Adds more information to
<environment_context>on Windows platforms and when WSL env vars are set. I think we're almost certainly add this to env context for all sessions, but want to test those cases further.Testing