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

Use Cargo's target information when possible #1225

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Sep 30, 2024

rustc's target triples generally only have a vague resemblance to each other and to the information needed by cc. Let's instead prefer CARGO_CFG_* variables when available, since these contain the information directly from the compiler itself.

In the cases where it isn't available (i.e. when running outside of a build script), we fall back to parsing the target triple, but instead of doing it in an ad-hoc fashion with string manipulation, we do it in a more structured fashion up front.

Fixes #1219 (at least my main gripe with the current implementation).
Closes #693 by making cc depend more on rustc's linker knowledge instead of the other way around.
Part of #1120.

Builds upon #1224.

src/target.rs Outdated Show resolved Hide resolved
src/target.rs Outdated Show resolved Hide resolved
rustc's target triples generally only have a vague resemblance to each
other and to the information needed by `cc`. Let's instead prefer
`CARGO_CFG_*` variables when available, since these contain the
information directly from the compiler itself.

In the cases where it isn't available (i.e. when running outside of a
build script), we fall back to parsing the target triple, but instead of
doing it in an ad-hoc fashion with string manipulation, we do it in a
more structured fashion up front.
src/lib.rs Show resolved Hide resolved
src/target.rs Outdated Show resolved Hide resolved
src/target.rs Outdated Show resolved Hide resolved
src/target.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 2, 2024

I've now gone through my own FIXMEs, and opened PRs against rustc to fix the discrepancies I saw when trying to parse the target information.

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 8, 2024

I've implemented the approach discussed in #1225 (comment) now.

@madsmtm madsmtm marked this pull request as ready for review October 8, 2024 21:01
dev-tools/gen-target-info/src/main.rs Outdated Show resolved Hide resolved
dev-tools/gen-target-info/src/main.rs Outdated Show resolved Hide resolved
///
/// This differs from `cfg!(target_arch)`, which only specifies the
/// overall architecture, which is too coarse for certain cases.
pub full_arch: Cow<'static, str>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can just use &'a str here, it'd be simpler and a smaller struct.

For from_cargo_environment_variables, we can cache the environment variables in cc::Build so that we can return a Target<'_>, we already do env caching so this is not a problem.

We can use the homebrew OnceLock impl within our codebase, so that we could avoid lifetime problems with MutexGuard/RwLockGuard.

Copy link
Collaborator

Choose a reason for hiding this comment

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

P.S. I'm not worried/obsessed about the allocation as it doesn't matter much, I just prefer a simpler structure.

Caching the env var in Build would also be consistent with how other env var currently works:

Once we read the env var from system into Build, it's never changed in that object and any objects created by Build::clone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't see how OnceLock would help with avoiding a guard? Do you mean to store cached environment variables in a global instead?

Copy link
Collaborator

@NobodyXu NobodyXu Oct 10, 2024

Choose a reason for hiding this comment

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

You can put OnceLock inside cc::Build struct.

For example:

struct Build {
    target_triple: Arc<OnceLock<String>>,
    // ...
}

store individual fields of Target directly in it, and return Target<'_> with a lifetime.

Arc is used to be consistent with existing env cache code.

Copy link
Contributor Author

@madsmtm madsmtm Oct 31, 2024

Choose a reason for hiding this comment

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

Hmm, I'm still not entirely sure I get what you want me to do, and I feel like it'd make the code more convoluted? (I'm somewhat trying to move things out of the huge Build struct).

Would you mind doing it?

@madsmtm madsmtm requested a review from NobodyXu October 31, 2024 16:08
Comment on lines -4048 to -4059
fn is_wasi_target(target: &str) -> bool {
const TARGETS: [&str; 7] = [
"wasm32-wasi",
"wasm32-wasip1",
"wasm32-wasip1-threads",
"wasm32-wasip2",
"wasm32-wasi-threads",
"wasm32-unknown-wasi",
"wasm32-unknown-unknown",
];
TARGETS.contains(&target)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cross-referencing #1126, I've replaced usage of is_wasi_target with target.os == "wasi" in this PR, but that of course won't catch wasm32-unknown-unknown - so there might now be places where instead of target.os == "wasi", the correct check would be target.arch == "wasm32" || target.arch == "wasm64"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants