no_std support for the url crate#831
Conversation
|
Went for |
|
I spent some time trying to get the trait `Serialize` is not implemented for `Ipv6Addr` |
If it breaks stable, that's obviously a no-go. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #831 +/- ##
=======================================
Coverage ? 82.25%
=======================================
Files ? 22
Lines ? 3562
Branches ? 0
=======================================
Hits ? 2930
Misses ? 632
Partials ? 0 ☔ View full report in Codecov by Sentry. |
|
It should not break stable, it should only "break" |
|
Moving to |
|
Since I do need the nightly version, but I can see why some people wouldn't, I added two features:
Else we can wait until the features get stabilised. |
| } | ||
| 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())); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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_stdbinary crate which uses the libraries
There was a problem hiding this comment.
Yes I'm trying it with cargo +nightly build -Zbuild-std=core,alloc --target aarch64-unknown-none -v --release --no-default-features
There was a problem hiding this comment.
Could you split this off into a separate PR?
|
Any news on this? :) |
|
☔ The latest upstream changes (presumably #840) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Updated to latest master |
|
I am not a maintainer, so I think @valenting should be requested for a review, rather than me. |
|
I'm not sure if the codecov failure is an issue - it's not caused by the PR as far as I can tell? Will I still need to add a new (unrelated?) testcase somewhere? |
| } | ||
| 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())); |
There was a problem hiding this comment.
Could you split this off into a separate PR?
|
Opened #843 to fix the idna |
|
☔ The latest upstream changes (presumably #853) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably beb2cde) made this pull request unmergeable. Please resolve the merge conflicts. |
.github/workflows/main.yml
Outdated
| if: | | ||
| matrix.os == 'ubuntu-latest' && | ||
| matrix.rust == 'nightly' | ||
| run: rustup target add aarch64-unknown-none && rustup component add rust-src --toolchain nightly-x86_64-unknown-linux-gnu && cargo +nightly build -Zbuild-std=core,alloc --target aarch64-unknown-none -v --release --no-default-features --features=alloc,unstable |
There was a problem hiding this comment.
@lucacasonato you could take over this check to the idna create
|
Apparently building --no-default-features workspace-wide didn't work, moved that check to the |
|
It's green 🎉 |
|
Any news on this? @valenting |
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>
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`).
The PRs in both crates have since been merged :) |
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`).
|
Any news on this? |
|
@valenting What do you think? |
|
I think this looks good. @domenukk could you rebase commits on main? |
|
@domenukk please rebase instead of merging in |
|
Don't you guys just squash this anyway? Do you really want the full history of me trying to fix CI? |
|
(Happy to squash this and force push, or rebase and remove the last commit, or whatever y'all want, but GitHub has a handy squash button when you merge) |
|
@domenukk Yeah, please squash and force push with a rebase. The squash button isn't available on merge queue repos. |
|
TIL :) |
|
Thank you for all the great work @domenukk ! |
|
See also #992 |
|
Are |
|
They are the same type. |
Bumps url from 2.5.2 to 2.5.3. Release notes Sourced from url's releases. v2.5.3 What's Changed fix: enable wasip2 feature for wasm32-wasip2 target by @brooksmtownsend in servo/rust-url#960 Fix idna tests with no_std by @cjwatson in servo/rust-url#963 Fix debugger_visualizer test failures. by @valenting in servo/rust-url#967 Add AsciiSet::EMPTY and boolean operators by @joshka in servo/rust-url#969 mention why we pin unicode-width by @Manishearth in servo/rust-url#972 refactor and add tests for percent encoding by @joshka in servo/rust-url#977 Add a test for and fix issue #974 by @hansl in servo/rust-url#975 no_std support for the url crate by @domenukk in servo/rust-url#831 Normalize URL paths: convert /.//p, /..//p, and //p to p by @theskim in servo/rust-url#943 support Hermit by @m-mueller678 in servo/rust-url#985 fix: support wasm32-wasip2 on the stable channel by @brooksmtownsend in servo/rust-url#983 Improve serde error output by @konstin in servo/rust-url#982 OSS-Fuzz: Add more fuzzer by @arthurscchan in servo/rust-url#988 Merge idna-v1x to main by @hsivonen in servo/rust-url#990 New Contributors @brooksmtownsend made their first contribution in servo/rust-url#960 @cjwatson made their first contribution in servo/rust-url#963 @joshka made their first contribution in servo/rust-url#969 @hansl made their first contribution in servo/rust-url#975 @theskim made their first contribution in servo/rust-url#943 @m-mueller678 made their first contribution in servo/rust-url#985 @konstin made their first contribution in servo/rust-url#982 @arthurscchan made their first contribution in servo/rust-url#988 Full Changelog: servo/rust-url@v2.5.2...v2.5.3 Commits 8a683ff Merge idna-v1x to main (#990) 08a3268 OSS-Fuzz: Add more fuzzers (#988) 5d363cc Improve serde error output (#982) 30e6258 fix: support wasm32-wasip2 on stable channel (#983) bf089c4 support hermit (#985) b08a655 Normalize URL paths: convert /.//p, /..//p, and //p to p (#943) ebd5cfb no_stdsupport for the url crate (#831) 7eccac9 Add a test for and fix issue #974 (#975) 710e1e7 refactor and add tests for percent encoding (#977) 6050a6e mention why we pin unicode-width (#972) Additional commits viewable in compare view Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase. Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: @dependabot rebase will rebase this PR @dependabot recreate will recreate this PR, overwriting any edits that have been made to it @dependabot merge will merge this PR after your CI passes on it @dependabot squash and merge will squash and merge this PR after your CI passes on it @dependabot cancel merge will cancel a previously requested merge and block automerging @dependabot reopen will reopen this PR if it is closed @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
`url` `v0.5.3` and `idna` `v1.0.3` added no-std support: servo/rust-url#831 Since Neqo sets `default-features = false`, the above would break Neqo. Related: servo/rust-url#831
`url` `v0.5.3` and `idna` `v1.0.3` added no-std support: servo/rust-url#831 Since Neqo sets `default-features = false`, the above would break Neqo. Related: servo/rust-url#831
This extends/rebases/fixes #717.
All checks seem to pass.
For
no_stdsupport, however, as mentioned in #717, this still needs a bunch of fixes. Specifically, It looks like we'll have to use theip_in_corenightly 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,
netand theErrortrait have made their way intocore