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

no_std support for the url crate #831

Open
wants to merge 46 commits into
base: main
Choose a base branch
from
Open

Conversation

domenukk
Copy link
Contributor

@domenukk domenukk commented Apr 8, 2023

This extends/rebases/fixes #717.
All checks seem to pass.
For no_std support, however, as mentioned in #717, this still needs a bunch of fixes. Specifically, It looks like we'll have to use the ip_in_core nightly feature, or use this crate: https://docs.rs/no-std-net. I'd personally opt for nightly, as it looks like it will be merged eventually, but I could do both.

/edit: For future reference, in the meantime, net and the Error trait have made their way into core

@domenukk
Copy link
Contributor Author

domenukk commented Apr 8, 2023

Went for ip_in_core now, so it needs nightly, but seems to work.

@domenukk domenukk marked this pull request as ready for review April 8, 2023 16:09
url/src/lib.rs Outdated Show resolved Hide resolved
@domenukk
Copy link
Contributor Author

I spent some time trying to get serde no_std support working, but it doesn't play nicely with the ip_in_core nightly feature - it throws errors like

the trait `Serialize` is not implemented for `Ipv6Addr`

@valenting
Copy link
Collaborator

Went for ip_in_core now, so it needs nightly, but seems to work.

If it breaks stable, that's obviously a no-go.
Also, our msrv is 1.56.0, so preferably the no_std changes would also work on that version.

@codecov
Copy link

codecov bot commented Apr 10, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@6050a6e). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #831   +/-   ##
=======================================
  Coverage        ?   81.85%           
=======================================
  Files           ?       21           
  Lines           ?     3560           
  Branches        ?        0           
=======================================
  Hits            ?     2914           
  Misses          ?      646           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@domenukk
Copy link
Contributor Author

It should not break stable, it should only "break" no_std (which never worked in the first place).
Switching to no-std-net also doesn't fix the problem with serde - the project seems stale/would need this to be merged:
dunmatt/no-std-net#16

@domenukk
Copy link
Contributor Author

Moving to no-std-net anyway, since it seems to fit your msrv better

@domenukk
Copy link
Contributor Author

Since I do need the nightly version, but I can see why some people wouldn't, I added two features:

  • no_std_net uses the no_std_net crate, no nightly needed (but the extra dependency)
  • unstable enabled unstable net_in_core feature, and also the error_in_core feature. Due to its opt-in nature, I would assume this is an okay solution (?)

Else we can wait until the features get stabilised.
Feedback welcome.

@@ -453,7 +453,7 @@ impl Idna {
return Errors::default();
}
let mut errors = processing(domain, self.config, &mut self.normalized, out);
self.output = std::mem::replace(out, String::with_capacity(out.len()));
self.output = core::mem::replace(out, String::with_capacity(out.len()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, this fixes a bug in the current idna no_std support. Why is it not caught in CI, and should I open an extra PR for it?

Choose a reason for hiding this comment

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

I recently dabbled with no_std stuff as well and I think the sure-fire way to test no_std compatibility is to:

  • target a platform that does not have std (certain ARM platforms) OR
  • have a really simple no_std binary crate which uses the libraries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I'm trying it with cargo +nightly build -Zbuild-std=core,alloc --target aarch64-unknown-none -v --release --no-default-features

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you split this off into a separate PR?

@domenukk
Copy link
Contributor Author

Any news on this? :)

@domenukk
Copy link
Contributor Author

Error has since landed in core.
Apart from the potentially breaking feature flag, this PR is ready from my point of view.

url/src/lib.rs Outdated
extern crate alloc;

#[cfg(not(feature = "alloc"))]
compile_error!("the `alloc` feature must be enabled");
Copy link
Member

Choose a reason for hiding this comment

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

issue: we should just not have an alloc feature, and unconditionally use alloc::Foo and core::Foo.

Copy link
Contributor Author

@domenukk domenukk Sep 19, 2024

Choose a reason for hiding this comment

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

Happy to change it, however I assumed a subset of features may be possible without alloc - and then introducing those will be another breaking change. Adding a feature flag to Crates.toml isn't a lot of work for users

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, all other crates like idna and data-url already have the alloc feature

Copy link
Member

Choose a reason for hiding this comment

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

If we wish to preplan for that I am happy to do so if someone can sketch out a viable plan for it, otherwise it feels like a premature optimization.

Furthermore as I said before adding new default features is typically not considered breaking in the ecosystem, and I do not consider it breaking here.

@domenukk
Copy link
Contributor Author

A very similar discussion is going on over at thiserror: dtolnay/thiserror#304

@Manishearth
Copy link
Member

CI is broken

I want to merge this only if @valenting also approves of the approach here.

@madsmtm
Copy link
Contributor

madsmtm commented Sep 19, 2024

I downloaded all the 186 possibly affected crates as determined by #831 (comment) (i.e. reverse-dependencies of url with default-features = false), and searched for instances of Url::socket_addrs, Url::from_file_path, Url::from_directory_path and Url::to_file_path.

Results:

  • vmemcached uses Url::socket_addrs in here.
  • wash-lib uses Url::from_file_path in here.
  • git-url-parse uses Url::from_file_path in here.
  • wick-sql uses Url::to_file_path in here.
  • wick-config uses Url::to_file_path in here.
  • release_plz_core uses Url::to_file_path in here.
  • wrpc-wasmtime-nats-cli uses Url::to_file_path in here.

I won't comment on whether that's an acceptable amount of breakage or not (I don't maintain this crate, so I don't really know what the general expected breakage is from any change), but it would be fairly actionable to go and submit PRs to fix these (I'm not motivated enough to do it myself) (git-url-parse and release_plz_core seems like the ones that the ecosystem would benefit the most from not breaking).

@Manishearth
Copy link
Member

Oh, thank you so much for doing that work! That does give me some pause but yeah if we know which crates are doing it it's easy to patch out.

@domenukk
Copy link
Contributor Author

I'm happy to open PRs to a handful of repositories, but since right now we don't have the std feature in url, we can't really do anything

@madsmtm
Copy link
Contributor

madsmtm commented Sep 20, 2024

I'm happy to open PRs to a handful of repositories, but since right now we don't have the std feature in url, we can't really do anything

They just have to remove default-features = false.

domenukk added a commit to domenukk/vmemcached-rs that referenced this pull request Sep 20, 2024
In order to make the rust-url compatible with no_std, the crate needs to introduce a `std` feature and make it default.
See servo/rust-url#831.

In order to reduce impact, downstream libraries should leave url's default features enabled (no other features are currently `default`).
domenukk added a commit to domenukk/wasmCloud that referenced this pull request Sep 20, 2024
In order to make the rust-url compatible with no_std, the crate needs to introduce a `std` feature and make it default.
See servo/rust-url#831.

In order to reduce impact, downstream libraries should leave url's default features enabled (no other features are currently `default`).
domenukk added a commit to domenukk/git-url-parse-rs that referenced this pull request Sep 20, 2024
In order to make the rust-url compatible with no_std, the crate needs to introduce a `std` feature and make it default.
See servo/rust-url#831.

In order to reduce impact, downstream libraries should leave url's default features enabled (no other features are currently `default`).
domenukk added a commit to domenukk/wick that referenced this pull request Sep 20, 2024
In order to make the rust-url compatible with no_std, the crate needs to introduce a `std` feature and make it default. See servo/rust-url#831.

To reduce impact, downstream libraries should leave url's default features enabled (no other features are currently `default`).
domenukk added a commit to domenukk/release-plz that referenced this pull request Sep 20, 2024
In order to make the rust-url compatible with no_std, the crate needs to introduce a `std` feature and make it default. See servo/rust-url#831.

To reduce impact, downstream libraries should leave url's default features enabled (no other features are currently `default`).
domenukk added a commit to domenukk/wrpc that referenced this pull request Sep 20, 2024
In order to make the rust-url compatible with no_std, the crate needs to introduce a `std` feature and make it default. See servo/rust-url#831.

To reduce impact, downstream libraries should leave url's default features enabled (no other features are currently `default`).
@domenukk
Copy link
Contributor Author

Opened the PRs mentioned in #831 (comment)

domenukk added a commit to domenukk/wasmCloud that referenced this pull request Sep 20, 2024
In order to make the rust-url compatible with no_std, the crate needs to introduce a `std` feature and make it default.
See servo/rust-url#831.

In order to reduce impact, downstream libraries should leave url's default features enabled (no other features are currently `default`).

Signed-off-by: Dominik Maier <dmnk@google.com>
tjtelan pushed a commit to tjtelan/git-url-parse-rs that referenced this pull request Sep 20, 2024
In order to make the rust-url compatible with no_std, the crate needs to introduce a `std` feature and make it default.
See servo/rust-url#831.

In order to reduce impact, downstream libraries should leave url's default features enabled (no other features are currently `default`).
@domenukk
Copy link
Contributor Author

domenukk commented Sep 20, 2024

git-url-parse and release_plz_core seems like the ones that the ecosystem would benefit the most from not breaking

The PRs in both crates have since been merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants