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

Draft: Add Redox scheme #88

Closed
wants to merge 4 commits into from
Closed

Conversation

rw-vanc
Copy link

@rw-vanc rw-vanc commented Dec 21, 2023

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
Copy link
Collaborator

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
Copy link
Author

rw-vanc commented Dec 21, 2023

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.
rust-lang/rust#51537
rust-lang/rust#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
Copy link
Collaborator

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 rust-lang/rust#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
Copy link

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
Copy link
Author

rw-vanc commented Dec 22, 2023

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:

  • does an exhaustive match on the Prefix enum
  • has not been compiled using the actual Redox toolchain
  • includes unknown-redox as part of its automation despite not being tested against the Redox toolchain

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
Copy link
Author

rw-vanc commented Dec 22, 2023

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
Copy link

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
Copy link
Collaborator

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
Copy link
Author

rw-vanc commented Dec 22, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

sunshowers commented Dec 24, 2023

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
Copy link
Author

rw-vanc commented Dec 24, 2023

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 rw-vanc changed the title Add Redox scheme Draft: Add Redox scheme Jan 16, 2024
@rw-vanc
Copy link
Author

rw-vanc commented Jan 29, 2024

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.

@rw-vanc rw-vanc closed this Jan 29, 2024
@sunshowers
Copy link
Collaborator

Understood, thanks!

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.

3 participants