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

std: Stabilize a number of new fs features #25844

Merged
merged 1 commit into from Jun 12, 2015

Conversation

Projects
None yet
10 participants
@alexcrichton
Copy link
Member

alexcrichton commented May 27, 2015

This commit stabilizes the following APIs, slating them all to be cherry-picked
into the 1.1 release.

  • fs::FileType (and transitively the derived trait implementations)
  • fs::Metadata::file_type
  • fs::FileType::is_dir
  • fs::FileType::is_file
  • fs::FileType::is_symlink
  • fs::DirEntry::metadata
  • fs::DirEntry::file_type
  • fs::DirEntry::file_name
  • fs::set_permissions
  • fs::symlink_metadata
  • os::raw::{self, *}
  • os::{android, bitrig, linux, ...}::raw::{self, *}
  • os::{android, bitrig, linux, ...}::fs::MetadataExt
  • os::{android, bitrig, linux, ...}::fs::MetadataExt::as_raw_stat
  • os::unix::fs::PermissionsExt
  • os::unix::fs::PermissionsExt::mode
  • os::unix::fs::PermissionsExt::set_mode
  • os::unix::fs::PermissionsExt::from_mode
  • os::unix::fs::OpenOptionsExt
  • os::unix::fs::OpenOptionsExt::mode
  • os::unix::fs::DirEntryExt
  • os::unix::fs::DirEntryExt::ino
  • os::windows::fs::MetadataExt
  • os::windows::fs::MetadataExt::file_attributes
  • os::windows::fs::MetadataExt::creation_time
  • os::windows::fs::MetadataExt::last_access_time
  • os::windows::fs::MetadataExt::last_write_time
  • os::windows::fs::MetadataExt::file_size

The os::unix::fs::Metadata structure was also removed entirely, moving all of
its associated methods into the os::unix::fs::MetadataExt trait instead. The
methods are all marked as #[stable] still.

As some minor cleanup, some deprecated and unstable fs apis were also removed:

  • File::path
  • Metadata::accessed
  • Metadata::modified

Features that were explicitly left unstable include:

  • fs::WalkDir - the semantics of this were not considered in the recent fs
    expansion RFC.
  • fs::DirBuilder - it's still not 100% clear if the naming is right here and if
    the set of functionality exposed is appropriate.
  • fs::canonicalize - the implementation on Windows here is specifically in
    question as it always returns a verbatim path. Additionally the Unix
    implementation is susceptible to buffer overflows on long paths unfortunately.
  • fs::PathExt - as this is just a convenience trait, it is not stabilized at
    this time.
  • fs::set_file_times - this funciton is still waiting on a time abstraction.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented May 27, 2015

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented May 27, 2015

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented May 27, 2015

Does this let Cargo build on stable? IIRC, it was only missing some FS labels.

I'm 👍 to backporting this to beta because it's 1.1. If it were 1.2, I'd say just wait.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented May 27, 2015

It does indeed! Cargo also needs the catch_panic feature to build 100% on stable, but it can just not use that feature of git2-rs for now and have panics turn into aborts (Cargo rarely panics anyway)

@retep998

This comment has been minimized.

Copy link
Member

retep998 commented May 28, 2015

With this winapi will be able to drop its dependence on libc!

@alexcrichton alexcrichton force-pushed the alexcrichton:stabilize-fs-features branch 3 times, most recently from 1bd9143 to ab0e4d7 May 28, 2015

@BurntSushi

This comment has been minimized.

Copy link
Member

BurntSushi commented May 28, 2015

Yay! I think this also lets xsv compile on Rust stable too.

👍

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Jun 2, 2015

This PR is now entering the final comment period for stabilizing these APIs!

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jun 4, 2015

Should os::windows::fs::MetadataExt::{creation_time, last_access_time, last_write_time} be left unstable until a suitable time type exists, or perhaps renamed in the same style as wait_timeout_ms?

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Jun 4, 2015

I personally view os::*::fs::MetadataExt as accessors of the raw underlying fields, so even if we had an abstraction for time I'd probably still opt to not return the abstraction but rather the u64 itself. There definitely does seem to be a use case, however, for a middle layer of abstraction sometimes, and this may not be the best way to draw the line in the end.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jun 4, 2015

Works for me.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jun 5, 2015

☔️ The latest upstream changes (presumably #26028) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -663,15 +623,17 @@ impl Permissions {
}
}

#[unstable(feature = "file_type", reason = "recently added API")]
impl FileType {

This comment has been minimized.

@Stebalien

Stebalien Jun 5, 2015

Contributor

I probably missed the discussion but why is this not an (extensible) enum? This seems like a really good candidate. Yes, some of the variants won't make sense on certain platforms but I don't think that's really an issue (we'll just have extra variants) and being able to match on filetype would be really nice.

This comment has been minimized.

@Stebalien

Stebalien Jun 5, 2015

Contributor

Variants:

enum FileType {
    // Common
    Directory, File, Symlink,
    // Windows Only
    ReparsePoint,
    // Unix Only
    CharDev, BlockDev, NamedPipe, Socket,
    #[doc(hidden)]
    __non_exhaustive
}

This comment has been minimized.

@alexcrichton

alexcrichton Jun 5, 2015

Author Member

We're currently not necessarily considering a hidden/unstable variant to be an "unstable enum", it was mostly just how ErrorKind worked out. The set of file types also varies quite a bit across platforms, and the extra types made somewhat more sense to provide through extension traits instead of via this enum.

Finally, using an opaque structure allows one day accessing the raw bits used to determine the file type which may contain some unknown information.

This comment has been minimized.

@Stebalien

Stebalien Jun 5, 2015

Contributor

This actually makes it easier to write cross-platform code because the coder doesn't need to care about the platform: they just handle all cases on all platforms (when possible).

This comment has been minimized.

@alexcrichton

alexcrichton Jun 5, 2015

Author Member

It's somewhat of a fine line between being able to write cross-platform code while still understand what is indeed cross platform. If we were to have an exhaustive enum which is the union of all file types on all platforms, it's suddenly not clear what file types come up on which platforms. Which is to say that it's not clear to me that having a union is indeed more cross platform as you're probably still going to have platform-specific code handling various file types on various platforms.

This comment has been minimized.

@Stebalien

Stebalien Jun 5, 2015

Contributor

We're currently not necessarily considering a hidden/unstable variant to be an "unstable enum", it was mostly just how ErrorKind worked out.

Given that we're already doing this, I don't see why we shouldn't do it here.

The set of file types also varies quite a bit across platforms, and the extra types made somewhat more sense to provide through extension traits instead of via this enum.

The problem with extension traits is that they force the user to care about the platform. With an enum, I could write:

fn format_filetype(s: &mut String, entry: &DirEntry) {
   match entry.file_type() {
        File => s.push('f'),
        NamedPipe => s.push('p'),
        ReparsePoint => s.push('r'),
        Other => s.push('?'),
        // ...
   }
}

With extension traits, I need one function per platform (each one importing a different extension trait). Extension traits are useful when some set of functionality is nonsensical on a platform but in this case, the extra file types make sense, they just aren't used.

Finally, using an opaque structure allows one day accessing the raw bits used to determine the file type which may contain some unknown information.

This unknown information is stored in the Metadata structure. Are you worried about unknown file types or something? Pretty much by definition, a file should only have one type so an enum should always be able to describe it.

This comment has been minimized.

@Stebalien

Stebalien Jun 6, 2015

Contributor

Sorry, I'm not re-loading before posting... For now, I feel that the enum is small enough that this isn't really an issue however, I can see this becoming an issue in the future as more platforms are added and this enum grows. So I see your point.

@BurntSushi BurntSushi referenced this pull request Jun 8, 2015

Closed

Remake PollWatcher #12

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Jun 9, 2015

triage: beta-accepted

@alexcrichton alexcrichton force-pushed the alexcrichton:stabilize-fs-features branch from ab0e4d7 to c72cf72 Jun 10, 2015

@alexcrichton

This comment has been minimized.

Copy link
Member Author

alexcrichton commented Jun 10, 2015

re-r? @aturon

I've rebased and updated this PR to include a snippet in RELEASES.md to talk about the new features.

pub type c_float = f32;
pub type c_double = f64;
#[cfg(target_arch = "aarch64")]
#[stable(feature = "raw_os", since = "1.0.0")] pub type c_char = u8;

This comment has been minimized.

@retep998

retep998 Jun 10, 2015

Member

Uh, shouldn't all these be stable for 1.1.0 and not 1.0.0?

This comment has been minimized.

@alexcrichton

alexcrichton Jun 10, 2015

Author Member

Ah oops, thanks!

std: Stabilize a number of new fs features
This commit stabilizes the following APIs, slating them all to be cherry-picked
into the 1.1 release.

* fs::FileType (and transitively the derived trait implementations)
* fs::Metadata::file_type
* fs::FileType::is_dir
* fs::FileType::is_file
* fs::FileType::is_symlink
* fs::DirEntry::metadata
* fs::DirEntry::file_type
* fs::DirEntry::file_name
* fs::set_permissions
* fs::symlink_metadata
* os::raw::{self, *}
* os::{android, bitrig, linux, ...}::raw::{self, *}
* os::{android, bitrig, linux, ...}::fs::MetadataExt
* os::{android, bitrig, linux, ...}::fs::MetadataExt::as_raw_stat
* os::unix::fs::PermissionsExt
* os::unix::fs::PermissionsExt::mode
* os::unix::fs::PermissionsExt::set_mode
* os::unix::fs::PermissionsExt::from_mode
* os::unix::fs::OpenOptionsExt
* os::unix::fs::OpenOptionsExt::mode
* os::unix::fs::DirEntryExt
* os::unix::fs::DirEntryExt::ino
* os::windows::fs::MetadataExt
* os::windows::fs::MetadataExt::file_attributes
* os::windows::fs::MetadataExt::creation_time
* os::windows::fs::MetadataExt::last_access_time
* os::windows::fs::MetadataExt::last_write_time
* os::windows::fs::MetadataExt::file_size

The `os::unix::fs::Metadata` structure was also removed entirely, moving all of
its associated methods into the `os::unix::fs::MetadataExt` trait instead. The
methods are all marked as `#[stable]` still.

As some minor cleanup, some deprecated and unstable fs apis were also removed:

* File::path
* Metadata::accessed
* Metadata::modified

Features that were explicitly left unstable include:

* fs::WalkDir - the semantics of this were not considered in the recent fs
  expansion RFC.
* fs::DirBuilder - it's still not 100% clear if the naming is right here and if
  the set of functionality exposed is appropriate.
* fs::canonicalize - the implementation on Windows here is specifically in
  question as it always returns a verbatim path. Additionally the Unix
  implementation is susceptible to buffer overflows on long paths unfortunately.
* fs::PathExt - as this is just a convenience trait, it is not stabilized at
  this time.
* fs::set_file_times - this funciton is still waiting on a time abstraction.

@alexcrichton alexcrichton force-pushed the alexcrichton:stabilize-fs-features branch from c72cf72 to ec68c4a Jun 10, 2015

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jun 12, 2015

This has gone through FCP and been reviewed by the core team, and we're ready to land the stabilization!

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jun 12, 2015

📌 Commit ec68c4a has been approved by aturon

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jun 12, 2015

⌛️ Testing commit ec68c4a with merge 50ab23d...

bors added a commit that referenced this pull request Jun 12, 2015

Auto merge of #25844 - alexcrichton:stabilize-fs-features, r=aturon
This commit stabilizes the following APIs, slating them all to be cherry-picked
into the 1.1 release.

* fs::FileType (and transitively the derived trait implementations)
* fs::Metadata::file_type
* fs::FileType::is_dir
* fs::FileType::is_file
* fs::FileType::is_symlink
* fs::DirEntry::metadata
* fs::DirEntry::file_type
* fs::DirEntry::file_name
* fs::set_permissions
* fs::symlink_metadata
* os::raw::{self, *}
* os::{android, bitrig, linux, ...}::raw::{self, *}
* os::{android, bitrig, linux, ...}::fs::MetadataExt
* os::{android, bitrig, linux, ...}::fs::MetadataExt::as_raw_stat
* os::unix::fs::PermissionsExt
* os::unix::fs::PermissionsExt::mode
* os::unix::fs::PermissionsExt::set_mode
* os::unix::fs::PermissionsExt::from_mode
* os::unix::fs::OpenOptionsExt
* os::unix::fs::OpenOptionsExt::mode
* os::unix::fs::DirEntryExt
* os::unix::fs::DirEntryExt::ino
* os::windows::fs::MetadataExt
* os::windows::fs::MetadataExt::file_attributes
* os::windows::fs::MetadataExt::creation_time
* os::windows::fs::MetadataExt::last_access_time
* os::windows::fs::MetadataExt::last_write_time
* os::windows::fs::MetadataExt::file_size

The `os::unix::fs::Metadata` structure was also removed entirely, moving all of
its associated methods into the `os::unix::fs::MetadataExt` trait instead. The
methods are all marked as `#[stable]` still.

As some minor cleanup, some deprecated and unstable fs apis were also removed:

* File::path
* Metadata::accessed
* Metadata::modified

Features that were explicitly left unstable include:

* fs::WalkDir - the semantics of this were not considered in the recent fs
  expansion RFC.
* fs::DirBuilder - it's still not 100% clear if the naming is right here and if
  the set of functionality exposed is appropriate.
* fs::canonicalize - the implementation on Windows here is specifically in
  question as it always returns a verbatim path. Additionally the Unix
  implementation is susceptible to buffer overflows on long paths unfortunately.
* fs::PathExt - as this is just a convenience trait, it is not stabilized at
  this time.
* fs::set_file_times - this funciton is still waiting on a time abstraction.

@bors bors merged commit ec68c4a into rust-lang:master Jun 12, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@alexcrichton alexcrichton deleted the alexcrichton:stabilize-fs-features branch Jul 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.