search

camino-rs / camino

Like Rust's std::path::Path, but UTF-8.
https://docs.rs/camino
Apache License 2.0
435 stars 16 forks source link

Draft: Add Redox scheme #88

Closed rw-vanc closed 8 months ago

rw-vanc commented 10 months ago

Allow Redox Scheme. This is a Path Prefix type that is supported by Redox's fork of std::path, but not yet in Rust's main. You can see it here.

sunshowers commented 10 months ago

Thanks for the PR!

I'm hesitant to include this in camino because this isn't in Rust upstream yet -- camino tracks stable Rust's Path and PathBuf types. What are your thoughts on that? Has there been an attempt to merge this into upstream Rust.

rw-vanc commented 10 months ago

I personally don't know the full history of this problem, but I know it's a long story between the Redox team and the Rust team, with the Rust team showing some intent to solve the problem, but no action taken. https://github.com/rust-lang/rust/pull/51537 https://github.com/rust-lang/rust/issues/52331 Redox uses a fork of libstd with Prefix type Scheme added, so this PR matches that. https://gitlab.redox-os.org/redox-os/rust/-/blob/redox-2023-01-21/library/std/src/path.rs?ref_type=heads#L192 This PR was written to cause minimum disruption, everything is target_os guarded so as to prevent any impact on software that is not expecting it. It would be a great help to us to have it merged, as Camino is the go-to for many Cargo extensions that we want to get working on Redox. We expect that when the Rust team converges on its preferred solution (which is likely to solve problems on other platforms as well), it will require changes to Camino, and we will gladly help with that.

sunshowers commented 10 months ago

Wouldn't this break software that compiles against the x86_64-unknown-redox platform that is already part of upstream Rust?

Given that the discussion in https://github.com/rust-lang/rust/issues/52331 appears to have trended away from the option here, it would seem that just gating this on target_os may cause breakage down the line once upstream Rust has a solution to this.

How about also gating this on a particular --cfg option as well, similar to how Tokio's unstable features work? Maybe camino_redox_unstable. Then when you're building a project against the fork of Rust, you can specify that via RUSTFLAGS.

Alternatively, does the forked version of Rust have any other reliable cfg() predicates that aren't in upstream Rust?

jackpot51 commented 10 months ago

If software uses the enum that was changed on Redox, it will get invalid data before this change and may require adaptation after this change (only on Redox). I'd prefer modifying some software than having it compile but break.

rw-vanc commented 10 months ago

Any software that has been compiled for Redox will not be broken by this, as compiling for Redox uses our Prefix definition. The only software that will be broken will meet all these characteristics:

So, breaking that build test is probably a good thing, as it is not an accurate reflection of support for Redox.

I will discuss adding a feature guard with Jeremy and get his thoughts.

rw-vanc commented 10 months ago

Jeremy says that there is no software that is built with unknown-redox but not with the Redox stdlib, or at least there should not be. So anything that will be broken by this PR is software that is wrong, i.e. it is being built with unknown-redox but not our toolchain and not our stdlib, and we think that build should fail. So it doesn't seem necessary to use a feature guard rather than a target_os guard.

Let me know if you are ok with that. Thanks for your help.

jackpot51 commented 10 months ago

In the meantime I will take on trying to fix the upstream issue in a satisfactory way. If y'all would like to wait on that, it would be fine.

sunshowers commented 10 months ago

If software uses the enum that was changed on Redox, it will get invalid data before this change and may require adaptation after this change (only on Redox). I'd prefer modifying some software than having it compile but break.

Agree with this.

Any software that has been compiled for Redox will not be broken by this

Accepting that software in the present will not be broken by this, I'm actually worried about things breaking in the future. For example, let's say that upstream Rust accepts a change like redox_kind. At that point I presume that the Prefix structure in the Redox fork will be changed to align with upstream. This would mean that existing copies of camino will break when compiled against an updated Redox fork of Rust. And that will likely lead to us getting complaints about camino no longer working. At that point I don't even know what semver guarantees would mean.

Having a --cfg flag (important note: not a feature flag like cfg(feature = "foo"), but a flag set via RUSTFLAGS like cfg(foo)) means that the user building the final application is aware that they're opting into unstable behavior.

As I proposed, another alternative is that the Redox fork of rustc always sets a --cfg flag. Then we could gate on that.

rw-vanc commented 10 months ago

Jeremy is thinking that we don't want to add a --cfg flag, it's a bit awkward for us given our toolchain and build process. Our preference is this PR as is, our second choice is to use a fork until we get the correct fix upstreamed in Rust std, then you or I can do the permanent fix for Camino. Jeremy is going to try again with the Rust folks and see if he can get it solved. Give us a week or two before doing anything.

sunshowers commented 10 months ago

Got it, thanks! I hope I'm not being too much of a bother here, I'm just concerned about the long-term impact of this change.

sunshowers commented 10 months ago

A thing that occurred to me is that you can maintain your own fork, redox-camino, and either use a patch directive to patch that in, or have users use that library on Redox with target-specific dependencies. (You can set lib.name to "camino" in Cargo.toml to let consumers use the same library name.)

rw-vanc commented 10 months ago

Thanks for your help on this. We will have to revisit it after the new year. Jeremy would still like to get Rust std::path fixed. We are going forward with a temporary fork for now and will let you know where we end up.

rw-vanc commented 8 months ago

Hi, we have decided that we are fighting a losing battle trying to get the world to adopt URI as a valid path format in a non-network context. We have changed the path format on Redox to be compatible with Linux format paths, so we are dropping this PR. Camino should work on Redox with no alteration.

sunshowers commented 8 months ago

Understood, thanks!