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

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

Merged
merged 1 commit into from
Oct 3, 2024

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

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

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

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.

@jieyouxu
Copy link
Member

jieyouxu commented Oct 2, 2024

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

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
Contributor

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
Contributor

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
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 3, 2024
…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
12 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 3, 2024
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
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
Development

Successfully merging this pull request may close these issues.

7 participants