Skip to content

Switch to CfgExpr for target testing. #8333

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

Open
ehuss opened this issue Jun 5, 2020 · 2 comments
Open

Switch to CfgExpr for target testing. #8333

ehuss opened this issue Jun 5, 2020 · 2 comments
Labels
A-cargo-api Area: cargo-the-library API and internal code issues C-cleanup Category: cleanup within the codebase E-hard Experience: Hard P-low Priority: Low S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@ehuss
Copy link
Contributor

ehuss commented Jun 5, 2020

The code here should not be testing on the target name, but instead should be using the cfg data to test the vendor/os/env fields.

There may be other places in the Cargo codebase that example the target triple that should probably also use cfg testing.

@ehuss ehuss added the A-cargo-api Area: cargo-the-library API and internal code issues label Jun 5, 2020
@weihanglo weihanglo added C-enhancement Category: enhancement C-cleanup Category: cleanup within the codebase E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed C-enhancement Category: enhancement labels Apr 25, 2023
@weihanglo weihanglo added E-medium Experience: Medium E-hard Experience: Hard and removed E-easy Experience: Easy E-medium Experience: Medium labels May 31, 2023
@weihanglo
Copy link
Member

I thought it is easy. I was completely wrong 🤯. Found that it is impossible for us to parse <arch>-<vendor>-<os>-<env> correctly unless we rewrite Triple::normalize in LLVM TargetParser. And we do have some funky real world examples, like wasm32-wasi is actually wasm32-unknown-wasi.

Or alternatively, we can check the supported target list in rust-lang/rust in as test cases, and make sure they are all good?

Feel like we probably should just follow what has been done in rust-lang/rust — compare string directly, so no change needed?

@ehuss
Copy link
Contributor Author

ehuss commented Jun 1, 2023

Hm, sorry if my original message wasn't clear (or maybe I'm confused). The intent is that code should be using the cfg data from TargetInfo. I don't think there is a matching function readily available, but it would essentially be something that iterates over the [Cfg] and finds one that says something like "target_env=msvc" or "target_vendor=apple".

I'm not sure if, for performance reasons, that iterating over the list is an issue, or if it would need a more sophisticated data structure or if it should stash the information somewhere so it only iterates once. My instinct is that it should be fine since the list is usually relatively small, but worth checking.

FWIW, I think this is relatively low priority. The current strings are unlikely to have an unexpected collision with another target, and otherwise it is mostly relevant to using target-spec JSON files, which are unstable.

@weihanglo weihanglo added the P-low Priority: Low label Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cargo-api Area: cargo-the-library API and internal code issues C-cleanup Category: cleanup within the codebase E-hard Experience: Hard P-low Priority: Low S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

No branches or pull requests

2 participants