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

Improve documentation when adding a new target #135992

Merged
merged 2 commits into from
Jan 31, 2025

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Jan 24, 2025

#133631 (comment) shows that it can be a bit difficult process-wise to add a new target.

I've added a bit of text to the docs, suggesting that users add the target defintion/spec first, and later work on std support.

I also found that we have two places where we document how to add a new target. I've linked these for now, but they should probably be merged somehow in the future.

@rustbot label A-docs
r? compiler
CC @workingjubilee who's worked a lot on target specs IIRC.

Both the target tier policy and the rustc-dev-guide has documentation on
this, let's make sure people see both.
@rustbot rustbot added A-rustc-dev-guide Area: rustc-dev-guide S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 24, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 24, 2025

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol

@rustbot rustbot added the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label Jan 24, 2025
Comment on lines 128 to 132
Note that crates like `libc` and `cc` may not accept your patches before the
`rustc` target specification itself exists, so it is recommended that you
start out with a minimal pull request adding just that (i.e. only
`#![no_core]` support), and then slowly expand the support in later pull
requests.
Copy link
Member

Choose a reason for hiding this comment

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

Hang on, doesn't sometimes r-l/r side PR need PRs to libc and cc to unblock the r-l/r side too? Won't this be a cyclic requirement then deadlock?

Copy link
Member

Choose a reason for hiding this comment

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

Ohh I see, this is explicitly to break that cycle right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. Not happy with how it's written, open to suggestions for better wording.

Copy link
Member

Choose a reason for hiding this comment

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

I'll open a T-compiler thread about this to clarify how to handle this cycle when trying to add targets

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I'll compiler-nominate this for awareness.

Copy link
Member

Choose a reason for hiding this comment

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

cc @NobodyXu in case you would like to add anything about adding new targets on the cc-rs side

Copy link
Member

@jieyouxu jieyouxu Jan 31, 2025

Choose a reason for hiding this comment

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

Exactly. Not happy with how it's written, open to suggestions for better wording.

Maybe something like

Note that adding a new target that wants to support std would transitively require cc and libc support. However, cc and libc would need to know about the target from rustc as well. To break this cycle, you are strongly encouraged to add a minimal #![no_core] target spec first to teach rustc about the target's existence, which cc and libc could then learn from, and add std support as a follow-up.

Let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rewritten it to something of that effect.

@rustbot ready

@jieyouxu jieyouxu added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 24, 2025
@jieyouxu jieyouxu self-assigned this Jan 24, 2025
@jieyouxu jieyouxu added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 24, 2025
@jieyouxu
Copy link
Member

jieyouxu commented Jan 24, 2025

Nominating this for awareness (please feel free to discuss async over at https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Improving.20docs.20for.20adding.20new.20targets):

Currently adding new Tier 3 targets is proving to be a lot of friction, because there's a cycle:

  • If you want to add a new target that wants to support std, you'll find std depends on cc-rs and libc
  • But cc-rs and libc needs to know about the new target from rustc, and sometimes can't accept new targets before said target spec exists in rustc

So now we have a cycle. Anything we can do to make this less of a pain? E.g.:

  • This PR modifies the Target Tier policy to recommend adding a bare metal target spec first in order for the target spec to exist in rustc, which makes it easier for cc-rs and libc to accept a patch to add the new target
  • Do we want to unconditionally require adding Tier 3 targets to need a MCP so that if the MCP is accepted, libc/cc-rs can be more comfortable in accepting a PR to add a new target even if it doesn't necessarily already exist in rustc...? See below

@rustbot label: +I-compiler-nominated

@rustbot rustbot added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Jan 24, 2025
@NobodyXu
Copy link
Contributor

libc/cc-rs can be more comfortable in accepting a PR to add a new target even if it doesn't necessarily already exist in rustc...?

I am willing to accept PR to cc-rs for new target, except for modifications on auto-generated target scrapped from rustc nightly

@BoxyUwU BoxyUwU removed their assignment Jan 30, 2025
@jieyouxu
Copy link
Member

@rustbot review

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 30, 2025
@apiraino
Copy link
Contributor

We had a look at the wording during the t-compiler triage meeting on Zulip and it looks fine.

@rustbot label -I-compiler-nominated

@rustbot rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Jan 30, 2025
@jieyouxu jieyouxu removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jan 31, 2025
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, some fiddling on the wording but LGTM as well otherwise.

Comment on lines 128 to 132
Note that crates like `libc` and `cc` may not accept your patches before the
`rustc` target specification itself exists, so it is recommended that you
start out with a minimal pull request adding just that (i.e. only
`#![no_core]` support), and then slowly expand the support in later pull
requests.
Copy link
Member

@jieyouxu jieyouxu Jan 31, 2025

Choose a reason for hiding this comment

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

Exactly. Not happy with how it's written, open to suggestions for better wording.

Maybe something like

Note that adding a new target that wants to support std would transitively require cc and libc support. However, cc and libc would need to know about the target from rustc as well. To break this cycle, you are strongly encouraged to add a minimal #![no_core] target spec first to teach rustc about the target's existence, which cc and libc could then learn from, and add std support as a follow-up.

Let me know what you think.

@jieyouxu
Copy link
Member

@rustbot author

@rustbot rustbot 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 31, 2025
Co-Authored-By: Jieyou Xu <jieyouxu@outlook.com>
@rustbot rustbot added 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 Jan 31, 2025
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, that looks good.

@jieyouxu
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 31, 2025

📌 Commit 7df38d9 has been approved by jieyouxu

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 31, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2025
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#134531 ([rustdoc] Add `--extract-doctests` command-line flag)
 - rust-lang#135860 (Compiler: Finalize dyn compatibility renaming)
 - rust-lang#135992 (Improve documentation when adding a new target)
 - rust-lang#136194 (Support clobber_abi in BPF inline assembly)
 - rust-lang#136325 (Delay a bug when indexing unsized slices)
 - rust-lang#136326 (Replace our `LLVMRustDIBuilderRef` with LLVM-C's `LLVMDIBuilderRef`)
 - rust-lang#136330 (Remove unnecessary hooks)
 - rust-lang#136336 (Overhaul `rustc_middle::util`)
 - rust-lang#136341 (Remove myself from vacation)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2025
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#134531 ([rustdoc] Add `--extract-doctests` command-line flag)
 - rust-lang#135860 (Compiler: Finalize dyn compatibility renaming)
 - rust-lang#135992 (Improve documentation when adding a new target)
 - rust-lang#136194 (Support clobber_abi in BPF inline assembly)
 - rust-lang#136325 (Delay a bug when indexing unsized slices)
 - rust-lang#136326 (Replace our `LLVMRustDIBuilderRef` with LLVM-C's `LLVMDIBuilderRef`)
 - rust-lang#136330 (Remove unnecessary hooks)
 - rust-lang#136336 (Overhaul `rustc_middle::util`)
 - rust-lang#136341 (Remove myself from vacation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f818842 into rust-lang:master Jan 31, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 31, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2025
Rollup merge of rust-lang#135992 - madsmtm:new-target-docs, r=jieyouxu

Improve documentation when adding a new target

rust-lang#133631 (comment) shows that it can be a bit difficult process-wise to add a new target.

I've added a bit of text to the docs, suggesting that users add the target defintion/spec first, and later work on `std` support.

I also found that we have two places where we document how to add a new target. I've linked these for now, but they should probably be merged somehow in the future.

`@rustbot` label A-docs
r? compiler
CC `@workingjubilee` who's worked a lot on target specs IIRC.
@madsmtm madsmtm deleted the new-target-docs branch February 2, 2025 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools A-rustc-dev-guide Area: rustc-dev-guide S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants