Skip to content

Fix target_vendor for aarch64-nintendo-switch-freestanding#131166

Merged
bors merged 1 commit intorust-lang:masterfrom
madsmtm:target-info-switch-vendor
Oct 3, 2024
Merged

Fix target_vendor for aarch64-nintendo-switch-freestanding#131166
bors merged 1 commit intorust-lang:masterfrom
madsmtm:target-info-switch-vendor

Conversation

@madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Oct 2, 2024

Previously set to target_vendor = "unknown", but Nintendo is clearly the vendor of the Switch, and is also reflected in the target name itself.

CC target maintainers @leo60228 and @jam1garner

@madsmtm
Fix target_vendor for aarch64-nintendo-switch-freestanding
746c322 
Previously set to `target_vendor = "unknown"`, but Nintendo is clearly
the vendor of the Switch, and is also reflected in the target name
itself.
@rustbot
Copy link
Collaborator

rustbot commented Oct 2, 2024

r? @chenyukang

rustbot has assigned @chenyukang.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Oct 2, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 2, 2024

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

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 2, 2024

Also, the "freestanding" part of the target triple is not reflected in either target_env nor target_abi, is this intentional? Or would it make sense to somehow expose that information to the user?

@jam1garner
Copy link
Contributor

jam1garner commented Oct 2, 2024

I don't see any reason why this change can't happen, I don't recall there being any specific intent in not specifying vendor

@leo60228
Copy link
Contributor

leo60228 commented Oct 2, 2024

aarch64-nintendo-switch-freestanding was renamed from aarch64-nintendo-switch late in the PR process, I think it was probably an oversight to not put it in target_env.

I don't remember if there was a reason target_vendor was unknown. I know we considered aarch64-unknown-horizon as another possible target name, but I didn't think that was ever implemented.

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 2, 2024 •
edited
Loading

Thanks for the response both of you!

aarch64-nintendo-switch-freestanding was renamed from aarch64-nintendo-switch late in the PR process, I think it was probably an oversight to not put it in target_env.

I'll let the reviewing compiler team member @chenyukang decide whether it's an appropriate value in target_env.

@madsmtm madsmtm mentioned this pull request Oct 2, 2024
Merged
@jieyouxu
Copy link
Member

jieyouxu commented Oct 2, 2024 •
edited
Loading

aarch64-nintendo-switch-freestanding is a Tier 3 target, and since both target maintainers agree:

  • The target_vendor adjustments LGTM.
  • I'm fine with changing target_env to be "freestanding", I don't see why this can't happen. EDIT: see below.

@jieyouxu jieyouxu assigned jieyouxu and unassigned chenyukang Oct 2, 2024
@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 2, 2024

I'm fine with changing target_env to be "freestanding", I don't see why this can't happen.

Mostly because it's a new target_env that isn't used elsewhere in rustc, so it's setting precedent for that value in target_env.

@jieyouxu
Copy link
Member

jieyouxu commented Oct 2, 2024 •
edited
Loading

EDIT: Actually based on the reference https://doc.rust-lang.org/reference/conditional-compilation.html#target_env:

Key-value option set with further disambiguating information about the target platform with information about the ABI or libc used. For historical reasons, this value is only defined as not the empty-string when actually needed for disambiguation. Thus, for example, on many GNU platforms, this value will be empty. This value is similar to the fourth element of the platform’s target triple. One difference is that embedded ABIs such as gnueabihf will simply define target_env as "gnu".

Let's not include the target_env = "freestanding" change for now, as based on existing usage patterns it seems like it's more about the libc used: "gnu", "msvc", "musl", "sgx" etc. Specifically, I don't think we need this string for disambiguation yet, and we can always adjust this in the future if there's another target that needs disambiguation from the eixsting aarch64-nintendo-switch-freestanding target.

r=me after removing target_env changes, sorry for the confusion.

@jieyouxu jieyouxu added I-compiler-nominated Nominated for discussion during a compiler team meeting. and removed I-compiler-nominated Nominated for discussion during a compiler team meeting. labels Oct 2, 2024
@madsmtm madsmtm force-pushed the target-info-switch-vendor branch from a4c0add to 746c322 Compare October 2, 2024 19:59
@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 2, 2024

I'm more comfortable with that myself, too

@jieyouxu
Copy link
Member

jieyouxu commented Oct 2, 2024

You can r=me after PR CI is green.

@bors delegate+ rollup

@bors
Copy link
Collaborator

bors commented Oct 2, 2024

✌️ @madsmtm, you can now approve this pull request!

If @jieyouxu told you to "r=me" after making some further change, please make that change, then do @bors r=@jieyouxu

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 2, 2024

@bors r=jieyouxu

@bors
Copy link
Collaborator

bors commented Oct 2, 2024

📌 Commit 746c322 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 Oct 2, 2024
@workingjubilee workingjubilee mentioned this pull request Oct 3, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 3, 2024
@bors
Auto merge of rust-lang#131185 - workingjubilee:rollup-wkcqe2j, r=wor…
8885239 
…kingjubilee

Rollup of 3 pull requests

Successful merges:

 - rust-lang#126930 (Add unstable support for outputting file checksums for use in cargo)
 - rust-lang#130725 (Parser: better error messages for ``@`` in struct patterns)
 - rust-lang#131166 (Fix `target_vendor` for `aarch64-nintendo-switch-freestanding`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cc61b81 into rust-lang:master Oct 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 3, 2024
@rust-timer
Unrolled build for rust-lang#131166
a2c360b 
Rollup merge of rust-lang#131166 - madsmtm:target-info-switch-vendor, r=jieyouxu

Fix `target_vendor` for `aarch64-nintendo-switch-freestanding`

Previously set to `target_vendor = "unknown"`, but Nintendo is clearly the vendor of the Switch, and is also reflected in the target name itself.

CC target maintainers `@leo60228` and `@jam1garner`
@rustbot rustbot added this to the 1.83.0 milestone Oct 3, 2024
@madsmtm madsmtm deleted the target-info-switch-vendor branch October 3, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jieyouxu jieyouxu

Labels

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

Milestone

1.83.0

Development

Successfully merging this pull request may close these issues.

7 participants

@madsmtm @rustbot @jam1garner @leo60228 @jieyouxu @bors @chenyukang