Skip to content

Remove rustc-dev-guide submodule #81848

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

Closed

Conversation

camelid
Copy link
Member

@camelid camelid commented Feb 7, 2021

Rationale:

  • The submodule hasn't been updated in 10 months (since April 2020)
  • It's unlikely that there's a need for it to be a submodule -- people
    can just clone the rust-lang/rustc-dev-guide repo
  • Submodules are a pain

See also the discussion on Zulip.

@camelid camelid added A-meta Area: Issues & PRs about the rust-lang/rust repository itself C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Feb 7, 2021
@rust-highfive
Copy link
Contributor

r? @Mark-Simulacrum

(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 Feb 7, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@camelid camelid force-pushed the rm-rustc-dev-guide-submodule branch from 92eac82 to 28a4a8e Compare February 7, 2021 01:52
@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)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/src/bootstrap/toolstate.rs"` failed.
Diff in /checkout/src/bootstrap/toolstate.rs at line 85:
 // We do require that we checked whether they build or not on the tools builder,
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
 // though, as otherwise we will be unable to file an issue if they start
 // failing.
-static NIGHTLY_TOOLS: &[(&str, &str)] = &[
-    ("miri", "src/tools/miri"),
-    ("embedded-book", "src/doc/embedded-book"),
-];
+static NIGHTLY_TOOLS: &[(&str, &str)] =
+    &[("miri", "src/tools/miri"), ("embedded-book", "src/doc/embedded-book")];
 
 fn print_error(tool: &str, submodule: &str) {
     eprintln!();
Build completed unsuccessfully in 0:00:23

Rationale:

* The submodule hasn't been updated in 10 months (since April 2020)
* It's unlikely that there's a need for it to be a submodule -- people
  can just clone the rust-lang/rustc-dev-guide repo
* Submodules are a pain

See also the [discussion on Zulip][z].

[z]: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/rustc-dev-guide.20submodule
@camelid camelid force-pushed the rm-rustc-dev-guide-submodule branch from 28a4a8e to 55d4287 Compare February 7, 2021 02:39
@Mark-Simulacrum
Copy link
Member

Hm, so I actually think we should not do this, but rather get into the habit of bumping the submodule more often and start distributing the dev guide as part of the rustc-docs component (or maybe a new one, given that one has problems with being installed alongside rust-docs). It's annoying that we don't provide an offline copy of it to users today; we should fix that.

That said, I would also not object to approving this in the meantime.

@Mark-Simulacrum Mark-Simulacrum 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 Feb 8, 2021
@jyn514
Copy link
Member

jyn514 commented Feb 13, 2021

Is there a point in having it as a submodule if it's only used for release artifacts? Could we change the release process to clone the rustc-dev-guide repo instead?

@Mark-Simulacrum
Copy link
Member

Submodules or an equivalent gives you pinning, which is critical if we want reproducible builds and just otherwise ensures that CI keeps passing, including in upstream distros from source tarballs. I would guess we can update it roughly on the same pace as other books; @ehuss updates those from time to time I think.

@camelid
Copy link
Member Author

camelid commented Feb 26, 2021

It's annoying that we don't provide an offline copy of it to users today; we should fix that.

You're right that it might be nice to provide an offline copy in rust-lang/rust rust-docs, but if people want an offline copy they can just clone rust-lang/rustc-dev-guide.

@camelid
Copy link
Member Author

camelid commented Feb 26, 2021

I don't care too much either way about this change; I just think we should only keep the submodule if it is regularly updated. So if @ehuss or someone else is okay with updating this every short while, then I don't think I see any harm in keeping it.

Either way, if we remove it or keep it, we can always easily reverse our decision later.

@camelid camelid added I-needs-decision Issue: In need of a decision. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 26, 2021
@Mark-Simulacrum
Copy link
Member

@ehuss would it be difficult to update the dev guide alongside the other books? I think you have a script that you run, right?

@Mark-Simulacrum Mark-Simulacrum removed the I-needs-decision Issue: In need of a decision. label Mar 1, 2021
@ehuss
Copy link
Contributor

ehuss commented Mar 1, 2021

Yea, I can add it. I'll be doing an update tomorrow, I'll include an update then.

I'm not sure I agree with the general idea of providing the dev-guide offline, since I've not seen anyone request it, and I suspect nobody even knows it exists (and how would they?). It makes updating submodules slower for everyone (and for people like me, my internet is fairly slow, it can take a significant amount of time). But it doesn't bother me for the bi-weekly updates.

@Mark-Simulacrum
Copy link
Member

Hm, well, I recall @nikomatsakis at least has mentioned to me that in pre-COVID times when travel was more of a regular occurrence, he would make extensive use of our std docs on planes and wanted the dev guide in some cases (but didn't have it available as it wasn't packaged). I could be convinced that we shouldn't use a submodule for it though - there's not really a need for rustc developers to get it, it'd be fine for only CI to clone the repository and we can track the commit in rustbuild or something.

@Mark-Simulacrum
Copy link
Member

I'll close this in the meantime as it sounds like we're going to start updating the submodule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants