search

servo / rust-url

URL parser for Rust
https://docs.rs/url/
Apache License 2.0
1.27k stars 318 forks source link

`no_std` support for the `url` crate #831

Open domenukk opened 1 year ago

domenukk commented 1 year ago

This extends/rebases/fixes #717. All checks seem to pass. For no_std support, however, as mentioned in https://github.com/servo/rust-url/pull/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 commented 1 year ago

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

domenukk commented 1 year ago

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 commented 1 year ago

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[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Please upload report for BASE (main@54346fa). Learn more about missing BASE report.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #831 +/- ## ======================================= Coverage ? 81.82% ======================================= Files ? 21 Lines ? 3549 Branches ? 0 ======================================= Hits ? 2904 Misses ? 645 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

domenukk commented 1 year ago

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: https://github.com/dunmatt/no-std-net/pull/16

domenukk commented 1 year ago

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

domenukk commented 1 year ago

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. Feedback welcome.

domenukk commented 1 year ago

Any news on this? :)

bors-servo commented 1 year ago

:umbrella: The latest upstream changes (presumably #840) made this pull request unmergeable. Please resolve the merge conflicts.

domenukk commented 1 year ago

Updated to latest master

OverOrion commented 1 year ago

I am not a maintainer, so I think @valenting should be requested for a review, rather than me.

domenukk commented 1 year ago

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?

domenukk commented 1 year ago

Opened #843 to fix the idna no_std bug.

bors-servo commented 1 year ago

:umbrella: The latest upstream changes (presumably #853) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo commented 1 year ago

:umbrella: The latest upstream changes (presumably beb2cde6469b215b52c895a7cf0859a219f72271) made this pull request unmergeable. Please resolve the merge conflicts.

domenukk commented 1 year ago

Apparently building --no-default-features workspace-wide didn't work, moved that check to the url create only -> CI might turn green

domenukk commented 1 year ago

It's green 🎉

domenukk commented 8 months ago

Any news on this? @valenting

domenukk commented 8 months ago

Any hint on what to do with codecov CI?

mspiegel commented 4 days ago

Is this PR still maintained? no_std support can now be achieved using stable rust 1.81. I have a patch ready that keeps the MSRV at 1.56 for the feature std. I see from the comments this would be released as a breaking change for the url crate.

domenukk commented 4 days ago

I'm happy to help to get this patch, your patch, or a mix of both into the url crate, but of course up to the maintainers regarding pace. I think keeping the MSRV for std, while adding no_std support for the latest stable rust could be a good way forward? (If that is possible at all)

Manishearth commented 4 days ago

So the upcoming changes to URL affect its msrv but not that intensely, otherwise I'd suggest piggybacking on that.

I'm mostly worried about --no-default-features users having an MSRV change. Do we have an idea as to how many of those there are?

Would be nice to land though.

mspiegel commented 4 days ago

I modified the Python script (above) for finding crates using default-features = false. The crates are printed along with the version requirement string for the url dependency. All the crates are using some variation of ^2. I was relieved to see nothing with a > comparison requirement. There appears to be a consensus that changing the default features of the url crate would increment the major version number. I have opened #953 for discussion. The PR includes a new section in the crate documentation that explains the breaking change from version 2 to version 3 when default-features = false.

Manishearth commented 4 days ago

There appears to be a consensus that changing the default features of the url crate would increment the major version number.

Wait, where is this consensus? This is not my experience with Rust.

Manishearth commented 4 days ago

To be clear, my worry is specifically that if this feature is implemented in a way that changes our MSRV in --no-default-features mode, and we have users in --no-default-features mode that care about MSRV, then we should be a bit wary. But I suspect we do not have such users given that the crate currently doesn't have any default features.

And even if we did, I wouldn't consider it breaking, I would consider it a "tread carefully".

madsmtm commented 4 days ago

I agree that the SemVer breakage is lessened now that both Error and the net types are available in core, but even if we're fine with bumping MSRV for users of default-features = false, the breakage is not eliminated, since we're still making certain APIs unavailable to those users.

E.g. both this PR and https://github.com/servo/rust-url/pull/953 will still break users of default-features = false if they also use Url::socket_addrs, Url::from_file_path, Url::from_directory_path or Url::to_file_path.

domenukk commented 4 days ago

FWIW just noticed that Error is going to be available in 8 days from now with 1.80.0, so don't merge just yet! :)

(Else we can also leave out the Error trait for now and add it later, but it'll be a new MSRV discussion)