-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Conversation
Both the target tier policy and the rustc-dev-guide has documentation on this, let's make sure people see both.
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. |
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Improving.20docs.20for.20adding.20new.20targets, please feel free to add additional details as you see fit
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 requirecc
andlibc
support. However,cc
andlibc
would need to know about the target fromrustc
as well. To break this cycle, you are strongly encouraged to add a minimal#![no_core]
target spec first to teachrustc
about the target's existence, whichcc
andlibc
could then learn from, and addstd
support as a follow-up.
Let me know what you think.
There was a problem hiding this comment.
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
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:
So now we have a cycle. Anything we can do to make this less of a pain? E.g.:
@rustbot label: +I-compiler-nominated |
I am willing to accept PR to cc-rs for new target, except for modifications on auto-generated target scrapped from rustc nightly |
@rustbot review |
There was a problem hiding this 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.
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. |
There was a problem hiding this comment.
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 requirecc
andlibc
support. However,cc
andlibc
would need to know about the target fromrustc
as well. To break this cycle, you are strongly encouraged to add a minimal#![no_core]
target spec first to teachrustc
about the target's existence, whichcc
andlibc
could then learn from, and addstd
support as a follow-up.
Let me know what you think.
@rustbot author |
Co-Authored-By: Jieyou Xu <jieyouxu@outlook.com>
f3e0f12
to
7df38d9
Compare
There was a problem hiding this 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.
@bors r+ rollup |
…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
…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
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.
#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.