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

Add a description field to target definitions #121905

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

Noratrieb
Copy link
Member

Starts addressing #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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 2, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 2, 2024

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

@bors
Copy link
Contributor

bors commented Mar 3, 2024

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

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.

LGTM, r=me after rebase if you're happy

Are there other things we want to add here? Like which tier a target is, or a yes/no for host toolchain support or std support? These are things that are always specific to each target, so it avoids having to have a file just for this metadata in your other PR with the documentation. Though they do feel like a strange thing to have here, maybe together in a metadata field.

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.
@Noratrieb
Copy link
Member Author

@bors r=davidtwco

@bors
Copy link
Contributor

bors commented Mar 5, 2024

📌 Commit 1db67fb has been approved by davidtwco

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 Mar 5, 2024
@Noratrieb
Copy link
Member Author

@bors p=1

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
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#121065 (Add basic i18n guidance for `Display`)
 - rust-lang#121301 (errors: share `SilentEmitter` between rustc and rustfmt)
 - rust-lang#121744 (Stop using Bubble in coherence and instead emulate it with an intercrate check)
 - rust-lang#121829 (Dummy tweaks (attempt 2))
 - rust-lang#121857 (Implement async closure signature deduction)
 - rust-lang#121894 (const_eval_select: make it safe but be careful with what we expose on stable for now)
 - rust-lang#121905 (Add a `description` field to target definitions)
 - rust-lang#122022 (loongarch: add frecipe and relax target feature)
 - rust-lang#122028 (Remove some dead code)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Mar 5, 2024

⌛ Testing commit 1db67fb with merge 3c02972...

@bors
Copy link
Contributor

bors commented Mar 5, 2024

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 3c02972 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 5, 2024
@bors bors merged commit 3c02972 into rust-lang:master Mar 5, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 5, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3c02972): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.2% [2.2%, 2.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.2% [2.2%, 2.2%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.9% [-2.3%, -1.5%] 2
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 644.518s -> 644.259s (-0.04%)
Artifact size: 175.01 MiB -> 175.02 MiB (0.00%)

@dpaoliello
Copy link
Contributor

FYI, description was added to from_json but not to_json, so check_consistency is failing if you try to set a description for a target: https://github.com/rust-lang/rust/actions/runs/8180997936/job/22370052535#step:27:4967

@Noratrieb Noratrieb deleted the add-empty-targets branch March 7, 2024 05:47
@Noratrieb
Copy link
Member Author

lol, oops. Happy to take a PR adding it, or I will do it in the future

@dpaoliello
Copy link
Contributor

check_consistency is failing if you try to set a description for a target: https://github.com/rust-lang/rust/actions/runs/8180997936/job/22370052535#step:27:4967

Done: #122157

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 8, 2024
Add the new description field to Target::to_json, and add descriptions for some MSVC targets

The original PR to add a `description` field to `Target` (<rust-lang#121905>) didn't add the field to `Target::to_json`, which meant that the `check_consistency` testwould fail if you tried to set a description as it wouldn't survive round-tripping via JSON: https://github.com/rust-lang/rust/actions/runs/8180997936/job/22370052535#step:27:4967

This change adds the field to `Target::to_json`, and sets some descriptions to verify that it works correctly.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
Rollup merge of rust-lang#122157 - dpaoliello:targetdesc, r=Nilstrieb

Add the new description field to Target::to_json, and add descriptions for some MSVC targets

The original PR to add a `description` field to `Target` (<rust-lang#121905>) didn't add the field to `Target::to_json`, which meant that the `check_consistency` testwould fail if you tried to set a description as it wouldn't survive round-tripping via JSON: https://github.com/rust-lang/rust/actions/runs/8180997936/job/22370052535#step:27:4967

This change adds the field to `Target::to_json`, and sets some descriptions to verify that it works correctly.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Mar 9, 2024
Add the new description field to Target::to_json, and add descriptions for some MSVC targets

The original PR to add a `description` field to `Target` (<rust-lang/rust#121905>) didn't add the field to `Target::to_json`, which meant that the `check_consistency` testwould fail if you tried to set a description as it wouldn't survive round-tripping via JSON: https://github.com/rust-lang/rust/actions/runs/8180997936/job/22370052535#step:27:4967

This change adds the field to `Target::to_json`, and sets some descriptions to verify that it works correctly.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2024
Add metadata to targets

follow up to rust-lang#121905 and rust-lang#122157

This adds four pieces of metadata to every target:
- description
- tier
- host tools
- std

This information is currently scattered across target docs and both
- not machine readable, making validation harder
- sometimes subtly encoding by the table it's in, causing mistakes and making it harder to review changes to the properties

By putting it in the compiler, we improve this. Later, we will use this canonical information to generate target documentation from it.

I used find-replace for all the `description: None`.

One thing I'm not sure about is the behavior for the JSON. It doesn't really make sense that custom targets supply this information, especially the tier. But for the roundtrip tests, we do need to print and parse it. Maybe emit a warning when a custom target provides the metadata key? Either way, I don't think that's important right now, this PR should get merged ASAP or it will conflict all over the place.

r? davidtwco
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

6 participants