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

Introduce infrastructure for generating target docs #121051

Closed
wants to merge 1 commit into from

Conversation

Noratrieb
Copy link
Member

@Noratrieb Noratrieb commented Feb 13, 2024

See #120745

It's highly unlikely that the format is optimal, but it's okay at least and can always be improved. Mostly posting this to get something working so we can continue.

Use TARGET_CHECK_ONLY=0 to actually build the book instead of just checking the new docs.

r? @davidtwco

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Feb 13, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 13, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as resolved.

@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member Author

this is a very funny failure, sadly I cannot reproduce it locally.

@ehuss
Copy link
Contributor

ehuss commented Feb 15, 2024

@Nilstrieb The issue is when it runs --print cfg --target csky-unknown-linux-gnuabiv2 it fails because LLVM 16 does not support that target.

It tries to print the error "No available targets are compatible with triple "csky-unknown-linux-gnuabiv2"".

However, one of the arguments is a SmallCStr which is created from a CStr. I believe there is a bug here where SmallCStr is created without the trailing null byte. It then later calls as_c_str which does from_bytes_with_nul_unchecked, but there is no null.

I can maybe try to post a fix for the panic later tonight. I don't know how best to handle the problem with LLVM 16 not supporting the target. Perhaps target-docs could check stderr for the "no available targets" message, and just skip that target?

@ehuss
Copy link
Contributor

ehuss commented Feb 15, 2024

Posted #121125 so it at least doesn't ICE.

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

I really like this approach. I think the only feedback I've got is about where some of the specific bits of information go.

This approach shines with the overlapping documentation based with patterns, but if you need to have some information for each target (like tier, maintainers, descriptive name, supports std, etc.) and have to make documentation for every target anyway, then you lose some of that benefit.

I think I'd like to see us experiment with putting some of that information into the target specification, and then have a way of dumping that from the compiler (like you do with the cfgs now).

@Noratrieb
Copy link
Member Author

Noratrieb commented Feb 20, 2024

I like that. We could even print the short description with --print target-list --verbose :). I will take a look.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 5, 2024
…idtwco

Add a `description` field to target definitions

Starts addressing rust-lang#121051 (review)

This is the short description (`64-bit MinGW (Windows 7+)`) including the platform requirements.

The reason for doing it like this is that this PR will be quite prone to conflicts whenever targets get added, so it should be as simple as possible to get it merged. Future PRs which migrate targets are scoped to groups of targets, so they will not conflict as they can just touch these.

This moves some of the information from the rustc book into the compiler.
It cannot be queried yet, that is future work. It is also future work to fill out all the descriptions, which will coincide with the work of moving over existing target docs to the new format.

r? `@davidtwco` but anyone is also free to steal it
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 5, 2024
…idtwco

Add a `description` field to target definitions

Starts addressing rust-lang#121051 (review)

This is the short description (`64-bit MinGW (Windows 7+)`) including the platform requirements.

The reason for doing it like this is that this PR will be quite prone to conflicts whenever targets get added, so it should be as simple as possible to get it merged. Future PRs which migrate targets are scoped to groups of targets, so they will not conflict as they can just touch these.

This moves some of the information from the rustc book into the compiler.
It cannot be queried yet, that is future work. It is also future work to fill out all the descriptions, which will coincide with the work of moving over existing target docs to the new format.

r? ``@davidtwco`` but anyone is also free to steal it
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 5, 2024
…idtwco

Add a `description` field to target definitions

Starts addressing rust-lang#121051 (review)

This is the short description (`64-bit MinGW (Windows 7+)`) including the platform requirements.

The reason for doing it like this is that this PR will be quite prone to conflicts whenever targets get added, so it should be as simple as possible to get it merged. Future PRs which migrate targets are scoped to groups of targets, so they will not conflict as they can just touch these.

This moves some of the information from the rustc book into the compiler.
It cannot be queried yet, that is future work. It is also future work to fill out all the descriptions, which will coincide with the work of moving over existing target docs to the new format.

r? ```@davidtwco``` but anyone is also free to steal it
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2024
…twco

Add a `description` field to target definitions

Starts addressing rust-lang#121051 (review)

This is the short description (`64-bit MinGW (Windows 7+)`) including the platform requirements.

The reason for doing it like this is that this PR will be quite prone to conflicts whenever targets get added, so it should be as simple as possible to get it merged. Future PRs which migrate targets are scoped to groups of targets, so they will not conflict as they can just touch these.

This moves some of the information from the rustc book into the compiler.
It cannot be queried yet, that is future work. It is also future work to fill out all the descriptions, which will coincide with the work of moving over existing target docs to the new format.

r? `@davidtwco` but anyone is also free to steal it
@bors

This comment was marked as resolved.

@Noratrieb Noratrieb added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2024
@Noratrieb
Copy link
Member Author

this is blocked on work on the tracking issue

@rustbot
Copy link
Collaborator

rustbot commented Mar 16, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member

@rollup=iffy

@dpaoliello
Copy link
Contributor

#123010 (comment) @Nilstrieb I think this was yours, unfortunately.

Checking this change out on Windows was failing - * is not a valid character for file names.

@Noratrieb
Copy link
Member Author

I see, Windows exists. Good point. I changed the filename to use _ instead of * for wildcards. Since that collides with _ present in some targets (like that one obscure architecture), I instead moved the pattern to the frontmatter, where asterisks are allowed (supposedly). target-docs enforces that the file name is consistent with the pattern.
I didn't rebase, so the force push compare page is accurate.
@rustbot ready

@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 Mar 27, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 27, 2024
…ng, r=compiler-errors

Let nils know about changes to target docs

i'll probably expand the paths and add a message after rust-lang#121051 but i honestly don't expect that to land very soon lol, so it would be nice to get notified about changes already and watch what's happening there

approve this pr if you're cool
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 27, 2024
…ng, r=compiler-errors

Let nils know about changes to target docs

i'll probably expand the paths and add a message after rust-lang#121051 but i honestly don't expect that to land very soon lol, so it would be nice to get notified about changes already and watch what's happening there

approve this pr if you're cool
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2024
Rollup merge of rust-lang#123142 - Nilstrieb:nils-knows-whats-happening, r=compiler-errors

Let nils know about changes to target docs

i'll probably expand the paths and add a message after rust-lang#121051 but i honestly don't expect that to land very soon lol, so it would be nice to get notified about changes already and watch what's happening there

approve this pr if you're cool
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

Changes in the last push seems solid. Can we add unit test in parse/tests.rs for the logic here?

let expected_file_name = frontmatter.pattern.replace('*', "_");
if expected_file_name != file_name {
bail!(
"`target pattern does not match file name. file name must be pattern with `*` replaced with `_`.\n\
Expected file name `{expected_file_name}.md`"
);
}

@klensy
Copy link
Contributor

klensy commented Mar 29, 2024

Well, why glob-match crate? There already glob and globset in workspace, and current usage should be doable via https://docs.rs/glob/latest/glob/struct.Pattern.html#method.matches.

@davidtwco
Copy link
Member

r=me with this comment resolved and if it isn't hard to use glob or globset instead, then that would be good too.

@onur-ozkan onur-ozkan 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 Apr 4, 2024
@Noratrieb
Copy link
Member Author

@onur-ozkan lol, thanks for mentioning the tests, I completely forgot about them and had to fix them

Another thing: I had to add this check:
https://github.com/rust-lang/rust/pull/121051/files#diff-b6fc213859d4da4176033b362381724914fea88a6c04122c0048ab5b76382343R1158

It was already kinda broken, lint-docs emitted warnings about things not quite matching up, so I don't think this should break anything and if it does it's good that it's broken.

@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot)
# be missing.
ENV IS_NOT_LATEST_LLVM 1


# Ubuntu LLVM 17 does not have support for experimental targets like csky.
ENV TARGET_DOCS_SKIP_TARGETS "csky-unknown-linux-gnuabiv2,csky-unknown-linux-gnuabiv2hf"
# Using llvm-link-shared due to libffi issues -- see #34486
ENV RUST_CONFIGURE_ARGS \
      --build=x86_64-unknown-linux-gnu \
      --llvm-root=/usr/lib/llvm-17 \
---
------
 > importing cache manifest from ghcr.io/rust-lang-ci/rust-ci-cache:1735714b6b19ef0a4e8a4e6747a678dadee568ebdeb9719db617faccdb946b2eba73802683684b5fe46c7560fac125406846a9bd7e9383f3421f89057efbc7ae:
------
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
  Downloaded boml v0.3.1
   Compiling boml v0.3.1
   Compiling y v0.1.0 (/checkout/compiler/rustc_codegen_gcc/build_system)
    Finished `release` profile [optimized] target(s) in 3.57s
     Running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-codegen/x86_64-unknown-linux-gnu/release/y test --use-system-gcc --use-backend gcc --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc --release --mini-tests --std-tests`
Using system GCC
[BUILD] example
[AOT] mini_core_hello_world
/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc/mini_core_hello_world
abc
---
running 46 tests
error: test failed, to rerun pass `--lib`

Caused by:
  process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/deps/bootstrap-de0d1d488e5b0274 --test-threads=1 --quiet -Z unstable-options --format json` (exit status: 1)
note: test exited abnormally; to see the full output pass --nocapture to the harness.
.....  local time: Thu Apr  4 19:35:20 UTC 2024
  network time: Thu, 04 Apr 2024 19:35:20 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@bors
Copy link
Contributor

bors commented May 14, 2024

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

@ehuss
Copy link
Contributor

ehuss commented May 22, 2024

lint-docs emitted warnings about things not quite matching up, so I don't think this should break anything and if it does it's good that it's broken.

This behavior was intentional, and I'm a little concerned about changing it. We want to be able to build documentation with stage 0 (#73964 has some more context on that). The lint docs were designed to handle that case. Changing that causes ./x.py doc to fail (with the default configuration). The doc command is designed to run in stage 0 by default.

Is it possible to have the target docs also just generate degraded output if using stage 0?

@ChrisDenton
Copy link
Member

Is it possible to have the target docs also just generate degraded output if using stage 0?

For now, we could just skip building target docs at --stage 0?

@oskgo
Copy link
Contributor

oskgo commented Aug 9, 2024

@Noratrieb Any updates on this? Thanks

@alex-semenyuk alex-semenyuk 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 4, 2024
@JohnCSimon
Copy link
Member

@Noratrieb
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this PR.
Thanks

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Nov 29, 2024
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Nov 29, 2024
@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.