Separate metadata parsing into a library #1000

jyn514 · 2020-08-23T21:07:11Z

This is intended to eventually be published and used by crater for rust-lang/crater#532.

jyn514 · 2020-08-24T00:44:53Z

This is ready for review.

src/docbuilder/metadata/Cargo.toml

pietroalbini

Other than the comments I left, I'd put the crate on the root directory:

/docsrs-metadata
/src
...

It's quite confusing to have other crates nested inside src/ (it's the same for badge).

src/docbuilder/metadata/.gitignore

src/docbuilder/metadata/Cargo.toml

pietroalbini · 2020-08-24T08:29:56Z

src/docbuilder/metadata/lib.rs

+
+        let mut map = HashMap::new();
+        map.insert("RUSTFLAGS", joined(&self.rustc_args));
+        map.insert("RUSTDOCFLAGS", joined(&self.rustdoc_args));


We're not adding -Z unstable-options, so the user's RUSTDOCFLAGS could not work. Also, we're missing a lot of other flags set by docs.rs.

The other flags are set in the rustwide builder: https://github.com/rust-lang/docs.rs/pull/1000/files#diff-8fac5c4dd8b0132a470c525431c6c4bcR609-R611. My reasoning is that crater doesn't need these flags, so they don't need to be part of the library (and moving over anything besides -Z unstable-options will be annoying because it needs access to the full TOML, not just [package.metadata.docs.rs]). Do you want -Z unstable-options in the library?

So, this is tricky: ideally I'd prefer to have all the flags being returned by Metadata, so Crater and other tools can fully test the exact setup docs.rs uses. On the other hand, docs.rs relies on unstable flags, and we often run Crater on the beta branch, so adding -Z unstable-options would prevent this from being used in beta runs :/

If it helps, the only difference this should make is for users passing unstable RUSTDOCFLAGS in CI, which seems like a pretty niche thing to do. Most of the unstable options affect only what documentation is generated, not the code rustdoc sees.

Most of the rustdocflags I've seen in the wild enable unstable compiler features via cfg_attr (mostly #![feature(doc_cfg)]), so do affect the code rustdoc sees.

Or I guess we could add everything to .environment_variables() and trick Crater to think it's on nightly even on beta when the "docs.rs mode" is used.

stable() and unstable() doesn't seem great because we don't know which of the user flags are which, we'd have to put them all in unstable(). I think the best way is to use RUSTC_BOOTSTRAP in crater, yeah.

Ok, let's move all environment variables handling in the library then.

Sorry - I was suggesting to move -Z unstable-options into the library, not --extern-html-root. Aside from the mess that makes on the command line, it's not relevant at all to crater since rustdoc will still see the same code either way, only the generated links will be different. And it will be very hard to make that part of the library.

Pushed a commit adding -Z unstable-options, let me know if you still have concerns. IMO the remaining options don't make sense to use with crater:

-j2

--target

--static-root-path /

--cap-lints warn

--resource-suffix

--extern-html-root-url

src/docbuilder/metadata/lib.rs

pietroalbini · 2020-08-24T08:32:42Z

We'll also need to change cargo test --locked to cargo test --workspace --locked on CI.

src/docbuilder/metadata/lib.rs

jyn514 · 2020-08-24T12:23:51Z

It's quite confusing to have other crates nested inside src/ (it's the same for badge).

Good point, I'll change it here and make a follow-up PR for badge.

jyn514 · 2020-08-24T12:48:16Z

Updated!

.github/workflows/ci.yml

metadata/lib.rs

.github/workflows/ci.yml

Cargo.toml

metadata/build.rs

src/docbuilder/mod.rs

metadata/lib.rs

jyn514 · 2020-08-27T17:52:05Z

Is this waiting on anything? @Kixiron did I address your suggestions or is there something else I should do?

pietroalbini · 2020-08-27T17:53:31Z

I'll take a look at this tomorrow.

Kixiron

LGTM

This keeps the exact same API as the original file.

- Use FromStr instead of a free method; use an error enum

- Greatly improve documentation; return `String` from `cargo_args` - Remove and ignore Cargo.lock - Add info to Cargo.toml - Remove useless .into() - Clean up build.rs - Fix dockerfile - Add another test - Appease clippy - Add even more tests - Warning that this is missing --extern-html-root - Fix wrong rustdoc arguments

- Use a workspace for separate crates - Fix dockerfile - Use serde instead of writing it out by hand As a side benefit, this has much less copying. - Use --workspace instead of --all - Make lints warn() instead of deny() - Make Metadata fields private - Make MetadataError non_exhaustive - Add link to /about/metadata

Many options passed on docs.rs require -Z unstable-options, so passing it in docs.rs and not crater wouldn't completely replicate the build environment. As a consequence, this library can only be used with nightly builds of rustdoc; this adds documentation saying so.

jyn514 added the A-builds Area: Building the documentation for a crate label Aug 23, 2020

jyn514 changed the title ~~[WIP] Separate metadata parsing into a separate library~~ Separate metadata parsing into a separate library Aug 24, 2020

jyn514 changed the title ~~Separate metadata parsing into a separate library~~ Separate metadata parsing into a library Aug 24, 2020