Page MenuHomeDevCentral
Paste P401

Claude code review for D4022
ActivePublic
Actions

Authored by dereckson on Sun, Jun 7, 12:57.
Tags
None
Referenced Files
F32204369: Claude code review for D4022
Sun, Jun 7, 12:57
Subscribers
None
Tokens
"Y So Serious" token, awarded by dereckson.
## Review of D4022 — Guess by heuristics Rust platform triplet
### Summary
The change is well-motivated and the overall approach is sound. Removing the hard `__virtual__` gate on `rustc` presence so the module can still function — falling back to heuristic detection — is a clean solution to a genuine bootstrapping problem.
---
### What works well
**Graceful degradation design** — replacing the `__virtual__` hard dependency with a `can_call_rustc` flag and branching inside `get_rustc_triplet()` is the right pattern. The module stays loadable and functional for the common cases.
**`_get_arch()` normalisations** — mapping `amd64 → x86_64` and `arm64 → aarch64` handles the BSD/Linux naming discrepancy that would otherwise trip up most servers.
**ARM 32-bit ABI logic** — the musl vs. glibc detection via `ldd --version` is a pragmatic standard approach, and appending `hf` based on `/proc/cpuinfo` VFP flags is correct for Rust's target naming.
---
### Issues and suggestions
**1. `__virtual__` removed entirely — module now always loads**
The old `__virtual__` ensured the module only loaded where it made sense. Now it loads unconditionally on all minions. If you want to keep that guard for systems where neither `rustc` nor a guessable platform is relevant, consider keeping `__virtual__` but making it return `True` always (or only excluding platforms you actively don't support). Not a blocker, but worth a conscious decision.
**2. `_get_os()` only normalises Linux — other OSes fall through as-is**
```python
def _get_os():
uname_os = os.uname().sysname.lower()
if uname_os == "linux":
return "linux-gnu"
return uname_os
```
On FreeBSD, `os.uname().sysname` returns `"FreeBSD"`, lowercased to `"freebsd"`. The Rust triplet vendor component for FreeBSD is typically `freebsd`, not `linux-gnu`, so the OS part is fine — but the **vendor** for non-Linux systems will still be `"unknown"`, which is correct for Linux but not always for macOS (`apple`) or FreeBSD. For your devserver use case this is probably fine if you only target Linux, but it's worth a comment noting the assumption.
**3. ARM non-Linux path falls back to `armv6` regardless of actual arch**
```python
def _guess_arm32_rust_triplet(os_part):
if os_part == "linux-gnu":
return ["arm", "unknown", _get_linux_arm_ebi()]
return ["armv6", "unknown", os_part]
```
For non-Linux ARM (e.g. FreeBSD on ARMv7), this hardcodes `armv6` as the arch. Since `_get_arch()` returns `"arm"` for any `armv6*` input, you've already lost the original variant string. Consider either retaining the raw `uname().machine` for the fallback or documenting that non-Linux ARM is unsupported.
**4. `ldd --version` output parsing is fragile on some systems**
On Alpine Linux, `ldd` from musl outputs to **stderr**, not stdout — `result.stdout` will be empty and the `"musl"` check will miss it. You should check both:
```python
ldd_output = (result.stdout + result.stderr).lower()
if "musl" in ldd_output:
...
elif "glibc" in ldd_output or "gnu" in ldd_output:
...
```
**5. `_guess_arm32_is_hard_float()` raises on a missing `features`/`flags` line**
If `/proc/cpuinfo` is readable but has no line starting with `features` or `flags` (possible on some embedded kernels), the function raises `ValueError` rather than returning a default. A safe fallback to `False` (soft-float) with a log warning might be more robust than crashing the triplet detection entirely.
**6. `subprocess` import vs. Salt's `cmd` execution module**
The rest of the module uses `__salt__["cmd.shell"](...)` for subprocess calls. `_get_linux_arm_ebi()` introduces a direct `subprocess.run()` call instead. This is inconsistent and also means the call bypasses Salt's logging and timeout infrastructure. Consider using `__salt__["cmd.run_all"](["ldd", "--version"], ...)` for consistency — though you'd need to handle `can_call_rustc` carefully to avoid circular dependency if `cmd` module isn't loaded yet. Alternatively, document why `subprocess` is used directly here.
---
### Minor nits
- The `hard_float_flags` list includes `"fpa"` — FPA is the old pre-VFP floating point that is actually *soft-float* at the ABI level in Rust's terms. Including it may produce an incorrect `hf` triplet on very old ARMv4/5 hardware. It's unlikely to matter in practice but worth removing or commenting.
- A docstring on `guess_rust_triplet()` and the private helpers would help future readers understand the supported/unsupported matrix.
---
### Overall verdict
The change is a solid step forward and unblocks the bootstrapping problem cleanly. The main issue worth fixing before landing is the **musl `ldd` stderr** case (point 4), as Alpine-based devservers would silently fail. The ARM non-Linux vendor/arch assumptions (points 2 & 3) are acceptable if your fleet is Linux-only, but should be documented. Everything else is low priority.

Event Timeline

dereckson created this paste.Sun, Jun 7, 12:57

Markdown content for code review for D4022

dereckson awarded a token.Sun, Jun 7, 12:57
Nasqueron DevCentral · If it had been much bigger the moon would have had a core of ice. · Powered by Phabricator