Dev/update version and notes#223
Conversation
|
@cpu ping? I do not mean to hurry. I totally understand if you do not have time, just tell me :) |
|
Thanks for the ping and sorry for letting it sit without a reply! I'm a bit more over-subscribed than usual this week and probably won't have a chance to look at it until ~next. You don't have to wait for me of course if that's too long to wait. |
|
Quick question, is the code in this PR what landed in the 0.18 release? Thank you all |
|
Hi @cpu,
|
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
Thanks for the ping and apologies for letting this fall off my radar! Here's some initial feedback.
| [](https://github.com/rusticata/x509-parser/actions) | ||
| [](#rust-version-requirements) | ||
|
|
||
| <!-- cargo-rdme start --> |
There was a problem hiding this comment.
Unfortunately it seems like this commit is breaking many markdown links in the rendered preview (https://github.com/rusticata/x509-parser/blob/fe005007af027b0cbf24f8a8bd8fadac204df527/README.md)
I think the link targets aren't quite right.
There was a problem hiding this comment.
This is an annoying limitation of cargo rdme: not all intradoc links are supported, for ex [`visitor`] is not replaced.
I found a workaround by giving the full path [`X509Certificate`](crate::certificate::X509Certificate) but now cargo doc is complaining that the path is redundant
If I push a commit to use this workaround, this will add warnings when running cargo doc (and cannot fix all links).
The last solution is to strip the intradoc links when generating the README.
There was a problem hiding this comment.
About #230 , I cherry-picked the commit in this branch with a few changes to resolve the conflicts and change src/lib.rs as well as README.md.
There was a problem hiding this comment.
This is an annoying limitation of cargo rdme: not all intradoc links are supported, for ex [
visitor] is not replaced.
Bummer :-(
Would you be open to a more direct/dumber solution? We use a small script in rustls that slurps the header rustdoc from lib.rs and inserts it into the README at a fixed location. In CI we run the same script and then error if there's a diff to make sure we always update lib.rs and not the copy in README.md.
I don't think we've had any significant trouble/friction with this approach but it's not quite as polished as a dedicated tool like cargo rdme.
If I push a commit to use this workaround, this will add warnings when running cargo doc (and cannot fix all links).
I wonder if it's possible to silence/allow just that one warning about redundant paths. That seems preferable to allowing all warnings, or stripping links if you want to stick with cargo rdme.
There was a problem hiding this comment.
Hi @cpu
I am trying to resurrect this PR and attempt to merge it before merging others (and releasing a beta.1)
As I see it, the problem is deeper: cargo readme and cargo rdme both have limitations/issues with intradoc links, so many links will fail (now and in the future). I can think of several solutions:
- in the past, I used a scripting solution (bash+awk) to do some links substitution, this is similar to your script. This almost works, but is clearly hackish and sometimes breaks
- Remove links in the README part: that would be sad, since it looks like most crates do that :/
- Wait for
cargo-rdmeto support intradoc links: this seems complex and not likely to happen soon - Use other tools based on
rustdoc(unlike the above ones) like cargo doc2readme- pros:
doc2readmeseems to work here, at least I could generate aREADME.mdfile with all links working (result uploaded here, see below for important note). It also has support for CI checks, and runs easily and automatically. - cons: it is not widely used, so there is always the risk to have a new breakage in some time
- NOTE: the doc links use the version referenced in the crate (0.19.1-beta.1), which is not yet released. Thus, they are broken until this is released.
- pros:
I would like to go forward and release a new version ASAP, so I think some compromise is necessary. What do you think of switching to doc2readme?
There was a problem hiding this comment.
Thanks for doing this analysis.
What do you think of switching to doc2readme?
It sounds like an OK option given the various trade-offs. 👍 For what it's worth I've also had positive exchanges with the maintainer in other repos.
NOTE: the doc links use the version referenced in the crate (0.19.1-beta.1), which is not yet released. Thus, they are broken until this is released.
I also notice it doesn't seem to directly link to the items, but instead populates a search query. That's not ideal, but also doesn't seem like a deal breaker. Maybe there's a way to improve on that in the future.
I would like to go forward and release a new version ASAP
Beyond the README issue I strongly think it makes sense to consider increasing MSRV with this release. There are many important crates with an MSRV of 1.85+ these days (reqwest, tokio, rand, quinn, rustls, hickory-dns, etc) and setting it to 1.85 would still cover what ships with Debian stable, while also addressing issues coming up in PRs (e.g. #233). I personally don't think this crate is in a position where a more aggressive policy is helpful.
There are also a number of bug fix PRs I've approved that would be nice to include in a release soon. I've been hesitant to merge anything without your review, no matter how trivial.
There was a problem hiding this comment.
Thank you for your quick reply
What do you think of switching to doc2readme?
It sounds like an OK option given the various trade-offs. 👍 For what it's worth I've also had positive exchanges with the maintainer in other repos.
Good to know!
NOTE: the doc links use the version referenced in the crate (0.19.1-beta.1), which is not yet released. Thus, they are broken until this is released.
I also notice it doesn't seem to directly link to the items, but instead populates a search query. That's not ideal, but also doesn't seem like a deal breaker. Maybe there's a way to improve on that in the future.
I will continue investigating a bit - it seems the tool is supposed to fall back on use search only on fall back cases, so maybe this can be improved in my example
I would like to go forward and release a new version ASAP
Beyond the README issue I strongly think it makes sense to consider increasing MSRV with this release. There are many important crates with an MSRV of 1.85+ these days (reqwest, tokio, rand, quinn, rustls, hickory-dns, etc) and setting it to 1.85 would still cover what ships with Debian stable, while also addressing issues coming up in PRs (e.g. #233). I personally don't think this crate is in a position where a more aggressive policy is helpful.
The MSRV policy (now clarified in this PR :) ) states 12 months minimum, so 1.85 is acceptable.
There are also a number of bug fix PRs I've approved that would be nice to include in a release soon. I've been hesitant to merge anything without your review, no matter how trivial.
This is exactly the goal: I would like to first get back master in shape (fixing CI etc.), then sort issues/PRs to merge some of them before beta.1, and continue working. This is just the first step
There was a problem hiding this comment.
Wonderful! I should be available this week if I can do anything to help.
3 participants
Misc changes to prepare first beta release (which will be 0.19.0-beta.1):
timeandthiserrortimedepends ontime-corewhich unfortunately requires rust 1.81, which explains the next commits--preciseflag withcargo updatecargo rdme, add CI checks and update linkssrc/lib.rsand README.md in sync