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

Redox Scheme Path Prefix #51537

Closed
wants to merge 2 commits into from
Closed

Redox Scheme Path Prefix #51537

wants to merge 2 commits into from

Conversation

jackpot51
Copy link
Contributor

This adds Scheme to the path::Prefix enum and fixes some issues with path handling on Redox

@rust-highfive
Copy link
Collaborator

r? @bluss

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2018

/// Scheme `file:` used on Redox
#[stable(feature = "rust1", since = "1.0.0")]
Scheme(#[stable(feature = "rust1", since = "1.0.0")] &'a OsStr),
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding this variant is a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, how should this be addressed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. The docs around Prefix imply that it's meant to be Windows-exclusive, so perhaps defining another type is the way to go? However, the only place where Prefix seems to be used is as part of Component, which is also a fully public enum and can't be extended with another kind of prefix.

It doesn't look like this part of the standard library was designed with this kind of extensibility in mind. Maybe in the next epoch the definition of Prefix could be changed (not sure if epochs allow these kinds of changes)?

Alternatively, you could define a std::os::redox::path::PathExt trait that's implemented by Path and allows access to the prefix. However, this does not allow Path::components to work like it should...

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if epochs allow these kinds of changes

I've also thought of that, but I don't think it would work (though I'd love to be corrected!). How would that work? I guess you would somehow change the version of the Components enum and components() function being used depending on the epoch. But what if code using it then tried to pass the Components to code in the older epoch? Though perhaps an unlikely use case for this particular struct, that probably shouldn't error complaining that the types are different, and couldn't work because the code it's passed to won't know how to deal with the new variant.

@kennytm kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 13, 2018
@pietroalbini
Copy link
Member

Ping from triage @bluss! This PR needs your review.

@pietroalbini
Copy link
Member

Ping from triage! This PR needs a review, can @bluss or someone else from @rust-lang/libs review this?

@alexcrichton alexcrichton added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jun 27, 2018
@TimNN TimNN removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2018
@alexcrichton alexcrichton added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 12, 2018
@alexcrichton
Copy link
Member

The libs team discussed this yesterday and our conclusion was that we indeed cannot extend a stable enum, but we'd be happy to accept a patch that has a redox-specific extension trait!

@jackpot51
Copy link
Contributor Author

Thanks @alexcrichton for the information. I believe more information about the problem I would like to solve is necessary, so I detailed it here, as well as potential solutions: #52331

@alexcrichton
Copy link
Member

Ok! Should this be closed while that's decided?

@jackpot51
Copy link
Contributor Author

Yes. I would expect to reopen with the chosen solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust-2-breakage-wishlist In the wishlist of breaking changes that requires rust 2.0 S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants