Skip to content

Drop the sys-info dependency#18324

Merged
zanieb merged 1 commit intomainfrom
zb/sys-info
Mar 6, 2026
Merged

Drop the sys-info dependency#18324
zanieb merged 1 commit intomainfrom
zb/sys-info

Conversation

@zanieb
Copy link
Member

@zanieb zanieb commented Mar 5, 2026

Part of #14574 and termux/termux-packages#27547

Avoids the downstream patch at https://github.com/termux/termux-packages/blob/master/packages/uv/0001-sys-info-replace-index-with-strchr.diff

sys-info-rs has not been updated in years https://github.com/FillZpp/sys-info-rs — it includes C code and can't be built on Termux FillZpp/sys-info-rs#116

I considered switching to https://github.com/GuillaumeGomez/sysinfo but our usage is trivial, so we implement our requirements in uv-platform instead

@zanieb zanieb force-pushed the zb/sys-info branch 2 times, most recently from 3208750 to 1b673c0 Compare March 5, 2026 20:03
@zanieb zanieb marked this pull request as ready for review March 5, 2026 21:15
@zanieb
Copy link
Member Author

zanieb commented Mar 5, 2026

Validated on Android in #18333

@zanieb zanieb added the internal A refactor or improvement that is not user-facing label Mar 5, 2026
@zanieb zanieb requested review from EliteTK and konstin March 6, 2026 13:31
@zanieb zanieb force-pushed the zb/sys-info branch 2 times, most recently from bf6d16a to 092bf5c Compare March 6, 2026 16:16
@zanieb zanieb enabled auto-merge (squash) March 6, 2026 16:28
@zanieb zanieb merged commit 363aea2 into main Mar 6, 2026
104 checks passed
@zanieb zanieb deleted the zb/sys-info branch March 6, 2026 16:30
Comment on lines +116 to +119
fs_err::read_to_string("/etc/os-release")
.ok()?
.parse()
.unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically we should fall back to /usr/lib/os-release if we don't find anything in /etc/os-release

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

Copy link
Collaborator

@samypr100 samypr100 Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason (e.g. more distro coverage)? Curious. I can also see falling back to /etc/lsb-release, but not sure if it's needed.

Note, https://github.com/FillZpp/sys-info-rs/blob/master/lib.rs#L483 only checked /etc/os-release

edit (found answer): Per https://www.freedesktop.org/software/systemd/man/latest/os-release.html

The file /etc/os-release takes precedence over /usr/lib/os-release. Applications should check for the former, and exclusively use its data if it exists, and only fall back to /usr/lib/os-release if that is missing

zanieb added a commit that referenced this pull request Mar 6, 2026
Part of #14574

Adds a cross-compiled build for the `aarch64-linux-android` target using
the Android NDK available on the runner.

Requires #18324
Comment on lines +76 to +79
let key = windows_registry::LOCAL_MACHINE
.open(r"SOFTWARE\Microsoft\Windows NT\CurrentVersion")
.ok()?;
Some(Self::Windows(key.get_string("CurrentBuildNumber").ok()?))
Copy link
Collaborator

@samypr100 samypr100 Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not match previous sys-info/linehaul behavior, do we care?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followed up in #18383

match self {
Self::Linux(os_type) => f.write_str(os_type),
Self::Darwin => f.write_str("Darwin"),
Self::WindowsNt => f.write_str("Windows_NT"),
Copy link
Collaborator

@samypr100 samypr100 Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect this to be Windows, to match previous linehaul impl and the output of platform.system() on Windows.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm I should have checked this more carefully. I intended to mirror sys-info.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followed up in #18383

zanieb pushed a commit that referenced this pull request Mar 9, 2026
…dows version (#18383)

## Summary

In #18324, sys-info-rs was dropped
in favor of a more native reimplementation to provide OsType and
OsRelease.

This PR adjusts two areas for windows to match closely previous
behavior:

1. OsType should be `Windows` rather than `Windows_NT`
#18324 (comment) as
seen in
https://github.com/FillZpp/sys-info-rs/blob/60ecf1470a5b7c90242f429934a3bacb6023ec4d/c/windows.c#L12.
This also matches the output of `platform.system()` in CPython.
2. OsRelease previously used `GetVersionEx` in sys-info-rs. Looking
closely, this was used primarily in
`crates/uv-python/src/interpreter.rs` and not in linehaul as linehaul
uses `platform.release()`. The problem with `GetVersionEx` is that it
returns often the wrong version due to legacy reasons (e.g. may be stuck
returning `6.2.9200`). The current implementation only returns the build
number from the registry which is prone to problems across windows older
variants. The implementation should use `RtlGetVersion` system call
which returns the current major, minor, build in the same way as
reported by `sys.getwindowsversion()` in CPython. In order to strike
balance, this switches the implementation to use four octects
`{major}.{minor}.{build}.{revision}` as recent windows versioning relies
on major, build and revision where as older versions rely on major,
minor and service pack. A windows-version crate by the same author as
windows-rs was added as it includes the correct system calls for the
windows versions.

## Test Plan

Tested manually on both Windows desktop (10, 11) and Windows Server
(2016).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal A refactor or improvement that is not user-facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants