Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DiskRefreshKind #1387

Merged
merged 17 commits into from
Nov 28, 2024

Conversation

dlowe
Copy link
Contributor

@dlowe dlowe commented Nov 22, 2024

fixes #1375.

  • add DiskRefreshKind, following the existing pattern (except that kind defaults to true)
  • add _specifics variants of Disk and Disks constructors and refresh methods
  • tweak the linux Disk and Disks to allow precise control over what gets loaded
  • manually tested on the targets I have access to (aarch64-linux-android, aarch64-apple-darwin, aarch64-apple-ios)

Questions / unresolved bits:

  • are there os-specific tests somewhere I can / should add to?
  • as discussed, we'll need to figure out what to do with DiskKind when the user explicitly refreshes .without_kind()
  • anything else I missed?

@dlowe dlowe force-pushed the selectively-refresh-disks branch 3 times, most recently from d47e355 to 4068e14 Compare November 22, 2024 21:41
@dlowe dlowe marked this pull request as ready for review November 22, 2024 21:52
src/common/disk.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Owner

I think it's great as a first step, thanks! For the DiskKind, I'm still unsure that not getting this information is a good idea.

@dlowe
Copy link
Contributor Author

dlowe commented Nov 25, 2024

I think it's great as a first step, thanks! For the DiskKind, I'm still unsure that not getting this information is a good idea.

For my specific use case, I need to be able to opt out of DiskKind to avoid the problematic canonicalize() call. I also don't need DiskKind at all, so it's an easy approach that works well for me.

Having the DiskKind default to true in DiskRefreshKind makes it so the only way to opt out of DiskKind is with an explicit call to without_kind()... at which point, disk.kind has the same behavior as every other skippable field, i.e. it has a value with no reliable semantics.

Can you elaborate on what your concern is?

@GuillaumeGomez
Copy link
Owner

Just personal taste but it's not really a valid argument. 😅 Then let's do it.

So: please add the possibility to opt out of the DiskKind. And also: please adds tests to check that even if the information is available when we retrieve it, we don't set it into the Disk. So for example if a platform retrieves everything with one call (how wonderful 🤩), then only set the information the user asked into Disk.

Finally, please add a test checking that each state of the Disk is (not) set according to RefreshDiskKind.

@dlowe
Copy link
Contributor Author

dlowe commented Nov 25, 2024

So: please add the possibility to opt out of the DiskKind. And also: please adds tests to check that even if the information is available when we retrieve it, we don't set it into the Disk. So for example if a platform retrieves everything with one call (how wonderful 🤩), then only set the information the user asked into Disk.

I'm okay with this, but I think it contradicts the existing sysinfo docs, which say:

⚠️ Just like all other refresh types, ruling out a refresh doesn’t assure you that the information won’t be retrieved if the information is accessible without needing extra computation.

... so just want to confirm: you want to test that we respect DiskRefreshKind even if it doesn't require additional system calls?

Finally, please add a test checking that each state of the Disk is (not) set according to RefreshDiskKind.

On it.

@dlowe dlowe force-pushed the selectively-refresh-disks branch 2 times, most recently from f0b6c7f to 9624d5a Compare November 25, 2024 19:33
@GuillaumeGomez
Copy link
Owner

I'm okay with this, but I think it contradicts the existing sysinfo docs, which say:

Yep. We should also remove this sentence.

src/common/disk.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Owner

And once done with linux, will remain apple, windows and freebsd. If needed, I can update the ones you can't.

@dlowe dlowe force-pushed the selectively-refresh-disks branch 5 times, most recently from 621e11b to 89404de Compare November 25, 2024 23:11
@dlowe
Copy link
Contributor Author

dlowe commented Nov 25, 2024

And once done with linux, will remain apple, windows and freebsd. If needed, I can update the ones you can't.

@GuillaumeGomez I've done the linux, apple and freebsd implementations, but I don't have easy access to a windows system.

The osx tests are now failing on a seemingly-unrelated libc problem that I don't think I could have caused (unless it's somehow by touching Cargo.toml?) Any ideas on those?

@GuillaumeGomez
Copy link
Owner

For the libc issue, it was fixed in #1393. So just a rebase will fix the CI. Gonna review the code to see if anything else is missing. Very excited about this change!

src/common/disk.rs Outdated Show resolved Hide resolved
src/common/disk.rs Outdated Show resolved Hide resolved
src/common/disk.rs Outdated Show resolved Hide resolved
src/common/disk.rs Outdated Show resolved Hide resolved
src/common/disk.rs Outdated Show resolved Hide resolved
src/common/disk.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Owner

Reminder that this PR still includes the switch to libc 0.2.164.

It's fine.

Time for a final review then. Finishing something else then review time!

src/windows/disk.rs Outdated Show resolved Hide resolved
src/windows/disk.rs Outdated Show resolved Hide resolved
src/windows/disk.rs Outdated Show resolved Hide resolved
src/windows/disk.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Owner

Ok I think it's all I found.

@dlowe dlowe force-pushed the selectively-refresh-disks branch from c98dedc to 78f0362 Compare November 27, 2024 19:22
@GuillaumeGomez
Copy link
Owner

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit 08dee51 into GuillaumeGomez:master Nov 28, 2024
67 checks passed
@dlowe dlowe deleted the selectively-refresh-disks branch November 28, 2024 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Selectively refreshing disks
2 participants