Hacktoberfest 2026: le issue che i maintainer hanno segnato per ottobre, aperte e adatte ai principianti. Sfoglia le issue Hacktoberfest

Collapse the bare/_at/_read three-entry-point split in openmpi.rs and driver_install.rs

Chiusa Adatta ai principianti
#690 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Una pull request collegata è già stata integrata.

  • #705 di @jussielo-amd — integrata

Valutazione

Difficoltà
2/5
Tempo stimato
1-3 ore
Idoneità per principianti
66/100
Tipo di issue
Refactoring
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
rust
Ambito
cli

Direzione di ricerca

Inizia cercando build_driver_install_plan e install_hint_for in apps/rocm/src/driver_install.rs e crates/rocm-core/src/openmpi.rs, e conferma che esistano le controparti _read. Il lavoro consiste principalmente in modifiche meccaniche nel modulo di test di driver_install.rs, racchiudendo ogni input di testo in OsRelease::found(text). È completato quando le funzioni semplici vengono eliminate, non rimane alcun riferimento ad esse e i test esistenti passano con asserzioni invariate.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

Background

#675 added an OsRelease-aware _read seam (plan_for_os_release_read, install_hint_for_read, build_driver_install_plan_read) alongside the existing bare-text and _at functions in crates/rocm-core/src/openmpi.rs and apps/rocm/src/driver_install.rs, so call sites that actually read the file (which may have come from the /usr/lib/os-release fallback) can name the real path in any error message.

That left three entry points per builder: a bare-text function (test-only), an _at function (path-aware), and a _read function (OsRelease-aware). openmpi::plan_for_os_release — the one non-test pub fn among these with no production caller — was dead and has been deleted in #675. install_hint_for and build_driver_install_plan remain as bare-text, #[cfg(test)]-only helpers.

Proposal

Now that tests can construct OsRelease::found(text) directly and call the _read seam, install_hint_for and build_driver_install_plan are redundant with it — tests could call install_hint_for_read/build_driver_install_plan_read instead and drop the bare-text middle layer entirely.

This wasn't done as part of #675 because the blast radius is large and unrelated to that PR's actual fix: a workspace grep found 34 call sites for build_driver_install_plan (all in driver_install.rs's test module, almost all exercising unrelated behavior — privilege escalation, WSL plans, AMDGPU version overrides, quote-style parsing — that merely need some os-release text as input) and 13 for install_hint_for. Rewriting all ~47 to construct OsRelease::found(text) wrappers is mechanical but inflates whatever PR does it several-fold for a purely cosmetic change, so it's better scoped as its own standalone cleanup.

Scope

  • Migrate the ~47 test call sites of build_driver_install_plan/install_hint_for to build_driver_install_plan_read/install_hint_for_read with OsRelease::found(text).
  • Delete the bare build_driver_install_plan/install_hint_for functions.
  • Keep the _at functions — they remain the real internal implementation detail both _read and (if any) remaining path-aware callers route through.
  • No behavior change; this is a pure test/internal-API cleanup.

Noted by @rominf in review on #675 (non-blocking "decision for the author").

Lingua principale
Rust
Stelle
45
Fork
11
Merge medio
3g 18h
PR unite (30g)
143

Preparare l'ambiente

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di ROCm/rocm-cli

Tutte le issue di ROCm/rocm-cli

Issue simili

Altre issue su Rust

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.