Conversation
Four places parsed `/etc/os-release` — the driver plan (which also picks the distro for the package installs it approves in `ensure_openmpi_for_vllm` and `ensure_torch_runtime_dep`), `rocm examine`, the OpenMPI install hint, and the `PRETTY_NAME` reader — each with its own `trim_matches`. Against `sh` sourcing the same file, they disagreed with each other: file sh driver plan rocm examine OpenMPI hint ID='ubuntu' ubuntu 'ubuntu' 'ubuntu' ubuntu ID=debian, then ID=ubuntu ubuntu debian ubuntu debian `os-release(5)` allows single quotes, so a spec-valid single-quoted file read as an unsupported distro. And on a file with a duplicated `ID`, `rocm examine` reported Ubuntu while the driver plan was built for Debian 12. All four now read through `rocm_core::os_release`, which keeps one rule: the value `sh` would assign, or nothing. For a file of blank lines, comments and spec-valid assignments it returns `sh`'s value — one matching quote pair removed, `\"`, `\\`, `` \` `` and `\$` decoded inside double quotes, nothing decoded inside single quotes, `\x` meaning `x` in a bare value, a `#` comment after a value, the last assignment of a key winning. Any other line makes the whole file unreadable, not just the key it names. To `sh` such a line is a command or the start of one — `export ID=…` and `X=1; ID=…` assign, `unset ID` removes, an unterminated quote swallows the lines after it — and a command can change any variable, including ones set before it. Reading past it would mean reporting values `sh` does not assign. A CRLF file is unreadable too: `sh` assigns `ubuntu\r`, which matches no distro. An unreadable file is reported, not hidden. `parse` names the first offending line, and the driver plan's reason becomes `/etc/os-release line N is not a plain assignment: "<line>"; no driver commands were planned because the distro cannot be read.` instead of "this distro is not supported", while `rocm examine` records the same line in `probe_failures`. Behaviour changes: - `rocm examine` reports single-quoted and duplicated keys as `sh` reads them, and an unreadable file as a probe failure naming its line. - The driver plan, and the package installs it approves, read the same distro `rocm examine` reports. - The OpenMPI hint no longer accepts `ID =x`; to `sh` that runs a command named `ID`. Real files: run against `dash`, all 50 container images checked and 396 of the 397 files in which-distro/os-release (10dfd5bc) read exactly as `dash` reads them; the one exception is a CRLF file, which reads as unreadable. No file read a value `dash` does not assign. Tests: - Against `/bin/sh` itself (empty environment, a sentinel `HOME`, a `PATH` that does not exist): every spec-valid encoding of every value up to three characters, and every right-hand side up to three characters over a quote-, backslash- and newline-heavy alphabet, checked for the key it sets and a key after it. Each must read as `sh` reads it or not at all, a list of well-formed cases must read as a value, and no earlier value may come back. - Nine mutations of the parser's rules each turn the suite red. - End to end through the driver plan: every supported distro rewritten in single quotes plans exactly as in double quotes; a duplicated `ID` plans from its last assignment; an unreadable file is refused with the message above and no commands. `rocm examine`'s probe failure is asserted with its state. These fail on the code before this change. No e2e scenario here: `/etc/os-release` is read from its real path, and the seam that lets a scenario plant one is the host-root re-rooting proposed in #510. Scenarios for a single-quoted file and a duplicated `ID` are written against that seam and follow once it lands. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #543.
/etc/os-releasewas parsed in four places, each with its owntrim_matches, and againstshsourcing the same file they disagreed with each other:shrocm examineID='ubuntu'ubuntu'ubuntu''ubuntu'ubuntuID=debianthenID=ubuntuubuntudebianubuntudebianSo a spec-valid single-quoted file read as an unsupported distro, and on a file with a duplicated
ID,rocm examinereported Ubuntu while the driver plan was built for Debian 12.The fix
All four readers now go through
rocm_core::os_release, which keeps one rule: the valueshwould assign, or nothing.sh's value: one matching quote pair removed,\"\\\`\$decoded inside double quotes, nothing decoded inside single quotes,\xmeaningxin a bare value, a#comment after a value, the last assignment winning.shsuch a line is a command or the start of one (export ID=…,X=1; ID=…,unset ID, an unterminated quote that swallows the lines after it), and a command can change any variable — so reading past it would report valuesshdoes not assign. CRLF files are unreadable too:shassignsubuntu\r, which matches no distro./etc/os-release line N is not a plain assignment: "<line>"; no driver commands were planned because the distro cannot be read.instead of "this distro is not supported", androcm examinerecords the same line inprobe_failures.Real files
Checked against
dash: all 50 container images, and 396 of the 397 files in which-distro/os-release at10dfd5bc, read exactly asdashreads them. The one exception is a CRLF file, which reads as unreadable. No file read a valuedashdoes not assign.Behaviour changes
rocm examinereports single-quoted and duplicated keys asshreads them, and an unreadable file as a probe failure naming its line.ensure_openmpi_for_vllmandensure_torch_runtime_dep— read the same distrorocm examinereports.ID =x; toshthat runs a command namedID.Tests
/bin/shitself, with an empty environment, a sentinelHOMEand aPATHthat does not exist: every spec-valid encoding of every value up to three characters, and every right-hand side up to three characters over a quote-, backslash- and newline-heavy alphabet, checked for the key it sets and a key after it. Each must read asshreads it or not at all; a list of well-formed cases must read as a value; no earlier value may come back.IDplans from its last assignment; an unreadable file is refused with the message above and no commands.rocm examine's probe failure is asserted together with the state it describes.Scenarios
There is no e2e scenario in this PR.
/etc/os-releaseis read from its real path, and the seam that lets a scenario plant one is the host-root re-rooting in #510. Scenarios for a single-quoted file and a duplicatedID, throughrocm install driver --dkms --dry-run, are written against that seam and follow once it lands.Verification
On this commit:
cargo fmt --check, both CIclippy -D warningsinvocations,cargo test -p rocm-core --lib(474 passed),-p rocm --bin rocm(974 passed),-p xtask(255 passed),xtask manifest --check,tpn --check,check-crate-edges, andcargo xtask e2e(153 scenarios; the only failures are the two already marked expected). The new end-to-end tests were run against the code before this change and fail there for the reasons described above.Locally,
cli_progress::tests::animated_spinner_keeps_ticking_without_progress_calls— which needs three 5 ms ticks inside 60 ms — failed in two full runs on a heavily loaded machine and passed five of five on its own; it is unchanged by this PR. Not run locally: the Windows lane.