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

Use custom traits instead of libstd's MetadataExt and PermissionsExt. #343

Merged
merged 6 commits into from
Jan 10, 2024

Conversation

sunfishcode
Copy link
Member

Define our own copy of MetadataExt and PermissionsExt and change the code to use them instead of std::os::*::fs::MetadataExt and `std::os::*::fs::PermissionsExt'.

In general, we should be moving away from using std's Ext traits like this, as they weren't meant to be used in this way.

Fixes #342.

…xt`.

Define our own copy of `MetadataExt` and `PermissionsExt` and change the
code to use them instead of `std::os::*::fs::MetadataExt` and
`std::os::*::fs::PermissionsExt'.

In general, we should be moving away from using std's `Ext` traits like
this, as they weren't meant to be used in this way.

Fixes #342.
@sunfishcode sunfishcode added the semver bump Issues that will require a semver-incompatible fix label Jan 3, 2024
Copy link
Contributor

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Seems sane to me

fn blksize(&self) -> u64;
/// Returns the number of blocks allocated to the file, in 512-byte units.
fn blocks(&self) -> u64;
#[cfg(target_os = "vxworks")]
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems easy for us to somehow miss future extensions to these traits in std, especially slightly more obscure OSes. This seems to be the main downside of this. But eh, in the end if there's some obscure new API and we don't notice, it can just be added.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. All of cap-std already faces this challenge; it's just the nature of what cap-std being what it is.

Remove more uses of the std::os traits, and update the build.rs scripts
to fix Rust feature detection.
@sunfishcode sunfishcode merged commit 6849a94 into main Jan 10, 2024
22 checks passed
@sunfishcode sunfishcode deleted the sunfishcode/dont-use-sealed-traits branch January 10, 2024 22:52
cgwalters added a commit to cgwalters/cap-std-ext that referenced this pull request Jan 18, 2024
Adapt to the changes in that crate, xref
bytecodealliance/cap-std#343
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver bump Issues that will require a semver-incompatible fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stop implementing std's MetadataExt
2 participants