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

Split rustdoc JSON types into separately versioned crate #81287

Merged
merged 8 commits into from
Jan 29, 2021

Conversation

CraftSpider
Copy link
Contributor

For now just an in-tree change.

In the future, this may be exposed as a standalone crate with standard semver.

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 22, 2021
@CraftSpider
Copy link
Contributor Author

r? @jyn514
I'll remember to put this in the original post someday

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-9 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
##########################################################                81.2%
######################################################################## 100.0%
extracting /checkout/obj/build/cache/2020-11-19/rustfmt-nightly-x86_64-unknown-linux-gnu.tar.xz
    Updating crates.io index
error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
If you want to try to generate the lock file without accessing the network, use the --offline flag.
Build completed unsuccessfully in 0:00:39
make: *** [prepare] Error 1
Makefile:60: recipe for target 'prepare' failed
Command failed. Attempt 2/5:
Command failed. Attempt 2/5:
error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
If you want to try to generate the lock file without accessing the network, use the --offline flag.
Build completed unsuccessfully in 0:00:00
make: *** [prepare] Error 1
Makefile:60: recipe for target 'prepare' failed
Command failed. Attempt 3/5:
Command failed. Attempt 3/5:
error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
If you want to try to generate the lock file without accessing the network, use the --offline flag.
Build completed unsuccessfully in 0:00:00
make: *** [prepare] Error 1
Makefile:60: recipe for target 'prepare' failed
Command failed. Attempt 4/5:
Command failed. Attempt 4/5:
error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
If you want to try to generate the lock file without accessing the network, use the --offline flag.
Build completed unsuccessfully in 0:00:00
make: *** [prepare] Error 1
Makefile:60: recipe for target 'prepare' failed
Command failed. Attempt 5/5:
Command failed. Attempt 5/5:
error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
If you want to try to generate the lock file without accessing the network, use the --offline flag.
Build completed unsuccessfully in 0:00:00
make: *** [prepare] Error 1
Makefile:60: recipe for target 'prepare' failed
The command has failed after 5 attempts.

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
    Checking tempfile v3.1.0
    Checking regex v1.4.3
    Checking json-types v0.1.0 (/checkout/src/librustdoc/json-types)
    Checking rustdoc v0.0.0 (/checkout/src/librustdoc)
error: Prefer FxHashMap over HashMap, it has better performance
   --> src/librustdoc/json/mod.rs:219:35
    |
219 |                 std::collections::HashMap::with_capacity(len),
    |                                   ^^^^^^^ help: use: `FxHashMap`
note: the lint level is defined here
   --> src/librustdoc/lib.rs:21:9
    |
    |
21  | #![deny(rustc::internal)]
    |         ^^^^^^^^^^^^^^^
    = note: `#[deny(rustc::default_hash_types)]` implied by `#[deny(rustc::internal)]`
    = note: a `use rustc_data_structures::fx::FxHashMap` may be necessary
error: aborting due to previous error

error: could not compile `rustdoc`

@GuillaumeGomez
Copy link
Member

Could you provide more information about this please? Why making it a standalone library would be better for example?

@CraftSpider
Copy link
Contributor Author

See this conversation for the details: https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/JSON.20Format/near/223685843
Basically, these types are part of the public interface of the rustdoc JSON output. Making them their own crate allows them to be versioned and distributed without having to depend on any rustdoc internals. This way, consumers can rely on this crate for both documentation of the output, and as a way to read the output easily, and its versioning is intended to follow semver guarantees about the version of the format. So JSON format X will always be compatible with json-types version N.

@GuillaumeGomez
Copy link
Member

Ok I see, makes sense. However, could you move this library outside of src/librustdoc please? In src/json-types directly would be fine I guess.

@CraftSpider
Copy link
Contributor Author

@jyn514 advised me to put it in librustdoc, so I'd like to give him a chance to comment first. If he agrees, I'll move it.

@GuillaumeGomez
Copy link
Member

You're right, let's maybe talk about it before you make any further changes. ;)

@jyn514
Copy link
Member

jyn514 commented Jan 22, 2021

I would prefer to keep this in librustdoc. It will only be used by rustdoc (src/json-types? types of what?) and I think having the Cargo.toml will make it clear that it's a separate crate.

@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 22, 2021
@GuillaumeGomez
Copy link
Member

I disagree. We have split all sub-crates and put them in their own folders for the compiler, so I think we should do the same here since the end goal is to put it out of tree. If the src location doesn't look good to you, another folder sounds good to me. Just not the same as librustdoc.

@jyn514
Copy link
Member

jyn514 commented Jan 22, 2021

Ok, then src/etc/json-types seems reasonable to me. But I don't think it should be at the top-level.

@GuillaumeGomez
Copy link
Member

Sounds good to me!

@CraftSpider
Copy link
Contributor Author

Alright, if src/etc/json-types is agreed on, I'll do the move

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

r=me with the directory renamed to src/etc/json-types.

@Mark-Simulacrum
Copy link
Member

etc seems like a really bad place. I'm worried we might be blanket copying that directory for dist too somewhere. Can we do src/rustdoc-json-types?

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 24, 2021
@CraftSpider
Copy link
Contributor Author

Moved the crate, and updated all the relevant files

@jyn514
Copy link
Member

jyn514 commented Jan 24, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jan 24, 2021

📌 Commit 36284a3 has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 24, 2021
@bors
Copy link
Contributor

bors commented Jan 27, 2021

☔ The latest upstream changes (presumably #80987) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 27, 2021
@CraftSpider
Copy link
Contributor Author

Merge conflict fixed, checks are green

@GuillaumeGomez
Copy link
Member

Thanks!

@bors r=jyn514,GuillaumeGomez squash

@bors
Copy link
Contributor

bors commented Jan 28, 2021

📌 Commit 3aa8456 has been approved by jyn514,GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 28, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 29, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#79570 (rustc: Stabilize `-Zrun-dsymutil` as `-Csplit-debuginfo`)
 - rust-lang#79819 (Add `SEMICOLON_IN_EXPRESSIONS_FROM_MACROS` lint)
 - rust-lang#79991 (rustdoc: Render HRTB correctly for bare functions)
 - rust-lang#80215 (Use -target when linking binaries for Mac Catalyst)
 - rust-lang#81158 (Point to span of upvar making closure FnMut)
 - rust-lang#81176 (Improve safety of `LateContext::qpath_res`)
 - rust-lang#81287 (Split rustdoc JSON types into separately versioned crate)
 - rust-lang#81306 (Fuse inner iterator in FlattenCompat and improve related tests)
 - rust-lang#81333 (clean up some const error reporting around promoteds)
 - rust-lang#81459 (Fix rustdoc text selection for page titles)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 788036d into rust-lang:master Jan 29, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 29, 2021
@CraftSpider CraftSpider deleted the json-crate branch January 29, 2021 04:17
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 4, 2021
rustdoc-json: Fix has_body

Previously, `has_body` was always true. Now propagate the type of the method to set it correctly. Relies on rust-lang#81287, that will need to be merged first.
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 4, 2021
rustdoc-json: Fix has_body

Previously, `has_body` was always true. Now propagate the type of the method to set it correctly. Relies on rust-lang#81287, that will need to be merged first.
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 5, 2021
rustdoc-json: Fix has_body

Previously, `has_body` was always true. Now propagate the type of the method to set it correctly. Relies on rust-lang#81287, that will need to be merged first.
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 5, 2021
rustdoc-json: Fix has_body

Previously, `has_body` was always true. Now propagate the type of the method to set it correctly. Relies on rust-lang#81287, that will need to be merged first.
@aDotInTheVoid
Copy link
Member

@rustbot modify labels: +A-rustdoc-json

@rustbot rustbot added the A-rustdoc-json Area: Rustdoc JSON backend label Feb 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants