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

Add Build::from_cargo or similar to make it very explicit when in a build script #1219

Open
madsmtm opened this issue Sep 29, 2024 · 3 comments · May be fixed by #1225
Open

Add Build::from_cargo or similar to make it very explicit when in a build script #1219

madsmtm opened this issue Sep 29, 2024 · 3 comments · May be fixed by #1225

Comments

@madsmtm
Copy link
Contributor

madsmtm commented Sep 29, 2024

cc's code is littered with brittle checks centered on rustc target triple. A lot of this is unnecessary though, since Cargo sets cfg values for the target as environment variables, see the docs for more examples. cc can't actually use those in most cases though, since it doesn't know whether it's being run inside a Cargo build script, or outside of it.

A prominent example is that CARGO_CFG_TARGET_ABI would be very useful for detecting Apple's simulator targets (doing it based on the target name is misery, and I think we're currently doing it wrong some places).

So I got to thinking, would it make sense to "split" cc's entry point into two, and deprecated cc::Build::new? Something like:

// build.rs
fn main() {
    // Emits all the rerun-if-changed flags, `cargo_metadata` defaults to true, etc.
    // Reads from `CARGO_CFG_TARGET_*` to initialize target data.
    cc::Build::from_cargo()
        .file("foo.c")
        .file("bar.c")
        .compile("foo");
}

// main.rs
fn main() {
    // Does not print needless Cargo metadata.
    // Using cc from outside build.rs, so has to specify target.
    // The target is parsed eagerly into some internal structure that represents it,
    // maybe using `target-lexicon`, and behind a feature flag? In any case, we
    // can say that this use-case is less supported, and the support burden will
    // perhaps be lessened?
    cc::Build::from_target("aarch64-unknown-linux-gnu")
        .file("foo.c")
        .file("bar.c")
        .compile("foo");
}
@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 29, 2024

Actually, I think my biggest issue is the ad-hoc parsing of the target triple; ideally, that would be done first thing inside Build::try_compile as a completely separate step, and the rest of the code would never touch the unparsed target triple.

Would you accept a PR changing the target parsing to be up-front? And would you accept a private dependency on target-lexicon (also used by the Cranelift and GCC rustc backends)? Or would it need to be vendored (possibly with a script) to maintain the illusion of low compile times just because you don't have any dependencies?

@NobodyXu
Copy link
Collaborator

Would you accept a PR changing the target parsing to be up-front?

Yeah I think having a new method calls from_cargo_build_script_env makes sense

And would you accept a private dependency on target-lexicon (also used by the Cranelift and GCC rustc backends)?

I think target-lexicon is small enough, as long as the serde feature is not enabled it should be ok to include it.

@madsmtm madsmtm mentioned this issue Sep 30, 2024
Open
@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 30, 2024

Hmm, I realized that target-lexicon has a build script to determine the host architecture, which would probably be a bit too expensive :/ (mostly in terms of "sequential" delay, Cargo would need to compile and link target-lexicon's build script, then compile target-lexicon, then compile cc, then compile and link the user's build script).

I have opened bytecodealliance/target-lexicon#112 to track that, not sure whether I will start on this in cc before or after that has been resolved.

This was referenced Sep 30, 2024
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use Cargo's target information when possible madsmtm/cc-rs
2 participants
@madsmtm @NobodyXu