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.

Sign up for GitHub

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

Jump to bottom

Fix target_vendor in QNX Neutrino targets #131169

Merged
merged 1 commit into from
Oct 24, 2024
Merged

Fix target_vendor in QNX Neutrino targets #131169

merged 1 commit into from
Oct 24, 2024
+2 −0

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Oct 2, 2024 •
edited
Loading

The x86_64-pc-nto-qnx710 and i586-pc-nto-qnx700 targets have pc in their target triple names, but the vendor was set to the default "unknown".

CC target maintainers @flba-eb, @gh-tr, @jonathanpallant and @japaric

@madsmtm
Fix target_vendor in QNX Neutrino targets
7a3a98d 
The `x86_64-pc-nto-qnx710` and `i586-pc-nto-qnx700` targets have `pc` in
their target triple names, but the vendor was set to the default
`"unknown"`.
@rustbot
Copy link
Collaborator

rustbot commented Oct 2, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
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 madsmtm mentioned this pull request Oct 2, 2024
Merged
@workingjubilee
Copy link
Member

not clear to me why the target_vendor isn't qnx, tbh.

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 2, 2024

The Aarch64 targets are named aarch64-unknown-nto-qnx700 and aarch64-unknown-nto-qnx710, so may just be historical?

@jonathanpallant
Copy link
Contributor

I don't know - what checks for this property?

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 3, 2024

I don't know - what checks for this property?

No idea, only opened this PR to fix the inconsistency (all the other targets with pc in their name does expose it in target_vendor).

wesleywiser
wesleywiser approved these changes Oct 11, 2024
View reviewed changes
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

I don't really have an opinion so I'm happy to merge if any of the target maintainers agree we should do this!

@jonathanpallant
Copy link
Contributor

jonathanpallant commented Oct 15, 2024 •
edited
Loading

I don't know what immediate value it adds, but it doesn't seem to hurt anything and I guess it's better to be consistent. +1 from me.

@jonathanpallant
Copy link
Contributor

jonathanpallant commented Oct 15, 2024 •
edited
Loading

not clear to me why the target_vendor isn't qnx, tbh.

My understanding is that i386-pc-foo specifically refers to an IBM PC compatible, as opposed to any other kind of machine you could build using an Intel x86 processor. It's the machine's "vendor", as in sun, ibm, or apple. Except there's no one vendor for PCs because it's an open standard, so it's just pc. That convention has then stuck.

AFAIK Arm machines get unknown because there's no Arm BIOS or other kind of platform standard.

@workingjubilee
Copy link
Member

Ah, fair enough then!

I don't really have an opinion so I'm happy to merge if any of the target maintainers agree we should do this!

I don't know what immediate value it adds, but it doesn't seem to hurt anything and I guess it's better to be consistent. +1 from me.

assert!(maintainers.iter().any(|maintainer| maintainer.agrees())

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Oct 24, 2024

📌 Commit 7a3a98d has been approved by wesleywiser

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 24, 2024
@Zalathar Zalathar mentioned this pull request Oct 24, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 24, 2024
@bors
Auto merge of rust-lang#132094 - Zalathar:rollup-5r1ppqt, r=Zalathar
55b7f8e 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#130225 (Rename Receiver -> LegacyReceiver)
 - rust-lang#131169 (Fix `target_vendor` in QNX Neutrino targets)
 - rust-lang#131623 (misc cleanups)
 - rust-lang#131756 (Deeply normalize `TypeTrace` when reporting type error in new solver)
 - rust-lang#131898 (minor `*dyn` cast cleanup)
 - rust-lang#131909 (Prevent overflowing enum cast from ICEing)
 - rust-lang#131930 (Don't allow test revisions that conflict with built in cfgs)
 - rust-lang#131956 (coverage: Pass coverage mappings to LLVM as separate structs)
 - rust-lang#132076 (HashStable for rustc_feature::Features: stop hashing compile-time constant)
 - rust-lang#132088 (Print safety correctly in extern static items)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 40d7872 into rust-lang:master Oct 24, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 24, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 24, 2024
@rust-timer
Unrolled build for rust-lang#131169
e1da772 
Rollup merge of rust-lang#131169 - madsmtm:target-info-nto-vendor, r=wesleywiser

Fix `target_vendor` in QNX Neutrino targets

The `x86_64-pc-nto-qnx710` and `i586-pc-nto-qnx700` targets have `pc` in their target triple names, but the vendor was set to the default `"unknown"`.

CC target maintainers `@flba-eb,` `@gh-tr,` `@jonathanpallant` and `@japaric`
@madsmtm madsmtm deleted the target-info-nto-vendor branch October 24, 2024 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

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.84.0
Development

Successfully merging this pull request may close these issues.
6 participants
@madsmtm @rustbot @workingjubilee @jonathanpallant @bors @wesleywiser