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

Cross-compilation to Yocto breaks after #1225 (1.1.32) #1297

Closed
tronical opened this issue Nov 21, 2024 · 10 comments
Closed

Cross-compilation to Yocto breaks after #1225 (1.1.32) #1297

tronical opened this issue Nov 21, 2024 · 10 comments

Comments

@tronical
Copy link
Contributor

I noticed that after #1225 as part of 1.1.32, cross-compilation of the Rust Skia bindings fails when targeting a Yocto environment (say Poky, but could be any distro name / vendor name).

Slightly more concretely, the skia build uses cc to compile some .cpp bindings code and calls cc_build.target(target_str) eventually, where target_str is arm-poky-linux-gnueabi. That's technically the correct target for the gcc Yocto configured/built. The rust target on the other hand is armv7-unknown-linux-gnueabihf.

Should the rust skia bindings build avoid calling set_target(), now that cc also tries to figure out the target from the cargo environment? I'm slightly hesitant here as the rust target is not the same as the one gcc is configured for. Or should cc be extended to parse this triplet?

@madsmtm
Copy link
Collaborator

madsmtm commented Nov 21, 2024

Build::target is the rustc target triple (and always has been), so if that's armv7-unknown-linux-gnueabihf, then that's what you should set (or yes, ideally leave it out if in a build script)

I'm not familiar with neither Yocto nor Poky, but if the above is not enough to get it to work, then we can add special support for this target (or, ideally, add upstream support in rustc).

@tronical
Copy link
Contributor Author

Thank you for the quick response! That sorts it, I'll remove the target() call. (that triggers another issue, but I'll create a PR for that)

@nekopsykose
Copy link

that doesn't really seem correct though? the comment says:

    /// This will fail when using a custom target triple unknown to `rustc`.
    fn from_str(target_triple: &str) -> Result<Self, Error> {
        if let Ok(index) =
            generated::LIST.binary_search_by_key(&target_triple, |(target_triple, _)| target_triple)
        {
            let (_, info) = &generated::LIST[index];
            Ok(info.clone())
        } else {
            Err(Error::new(
                ErrorKind::InvalidTarget,
                format!("unknown target `{target_triple}`"),
            ))
        }
    }

in my case:

$ rustc --print target-list
...
x86_64-chimera-linux-musl
...

i.e. it is definitely a target rustc knows (the distro rust is aware of the custom vendor triples), but yet:

$ hx -g build
Building 206 grammars


error occurred: unknown target `x86_64-chimera-linux-musl`

(that calls .host(BUILD_TARGET) which is set from cargo:rustc-env=BUILD_TARGET=std::env::var("TARGET") in build.rs)

it seems incorrect to me to fail on the default TARGET that cargo itself is setting in build.rs, and only working with e.g. the -unknown- triples that are hardcoded from a pregenerated list (since 290a629).

downstream projects do actually set these values like in https://github.com/helix-editor/helix/blob/f305c7299d8471957eaa66bb83a4c9a70cfc57a9/helix-loader/src/grammar.rs#L437 , and i don't see what they are supposed to do if using cargo's own information from build.rs (TARGET) is now not supported and only a hardcoded list (generated on "someone else's computer") is permitted, which won't necessarily find a match

@madsmtm
Copy link
Collaborator

madsmtm commented Nov 22, 2024

it is definitely a target rustc knows

Then things should just work in build scripts (at least after #1299).

the distro rust is aware of the custom vendor triples

But if you're using this outside a build script, then we're in a tough situation, since cc-rs simply doesn't have enough knowledge about the target (parsing the target triple was considered initially, but was deemed basically impossible to do correctly, since rustc target triples don't follow a consistent format).

To fix this, I'd strongly prefer that you upstreamed those special target triples to rustc, is there anything blocking that? Or if that's not needed, why is Chimera even overriding the target vendor (as opposed to just using the existing x86_64-unknown-linux-musl)?

Other options that I'm less in favour of (and which could be pursued in the meantime anyhow):

  • Invoke rustc --print=target-spec-json if we don't recognize the target.
  • Special-case Chimera in cc-rs.

@NobodyXu
Copy link
Collaborator

Invoke rustc --print=target-spec-json if we don't recognize the target.

We could do that in build-script, the problem with that cc-rs now has to pull in a json parser, which is usually large and something I really want to avoid

@nekopsykose
Copy link

To fix this, I'd strongly prefer that you upstreamed those special target triples to rustc, is there anything blocking that?

i have no idea how to do that, but it wouldn't be of much use since changing the vendor part of the triple is not a rare thing for a distro to do; it would be quite weird for every -linux-musl distro to upstream the same thing but with one part of a string changed, just to fix things that use a hardcoded list (since everything else doesn't care that much)

Special-case Chimera in cc-rs.

(which has the same issue as the above; it's a cat and mouse game since this field is not that special, or at least downstreams treat it that way since for C compilers it definitely means pretty much nothing and gets changed in every downstream)

why is Chimera even overriding the target vendor (as opposed to just using the existing x86_64-unknown-linux-musl)

for *-linux-musl distros in general (alpine also has a changed vendor field), at least one change is to change the default for that os+env from static to dynamic linking (something discussed in rust for the past maybe half decade to change in rust as a default, but would be quite the breaking change if it was done, since it would break the default expected output for cargo build --target=*-linux-musl). for chimera specifically, also because it's not a gcc-based environment so e.g. there is no libgcc_s at all, so it requires fixing rustc for it to be able to link any dynamically-linked output (by default it hard requires -lgcc_s to work on -linux- seemingly). that one has had some bug reports in rustc somewhere that i forgot years ago, but i guess it's rare enough that nobody fixed it.

maybe it's possible to somehow make rustc smarter and not require downstreams to patch such small things and for every *-linux-musl distro to not need a change in the triple (a change as significant as changing the default linkage type does seem to warrant at least changing the vendor section to ease surprises..), but i don't think that would happen anytime soon.

of course, this also used to work before #1225 , but i guess that was deemed a necessary change. since this only breaks when used outside build.rs i guess in practice very few programs will end up breaking on unrecognised triples (i only know of helix, since it can build grammars both at build time and via a command, so the logic is outside the build.rs script), so i guess it can be left as is. sorry for the noise

@NobodyXu
Copy link
Collaborator

Perhaps we can fix it for non-build-script use case, by translating it to a *-unknown-linux-* triple?

@nekopsykose
Copy link

as far as detection, i can't think of why that wouldn't work (after all, it did before where as far as i know this was mostly ignored via the old parsing), as long as it doesn't have any weird implementation gotchas for you.

but if that edited triple is then passed to cc, i.e.

before:

  • rust triple: x86_64-x-linux-gnu
  • "cc triple": x86_64-x-linux-gnu
  • -> cc-rs invokes cc --target x86_64-x-linux-gnu or similar

after:

  • -> cc-rs replaces x with unknown and passes --target x86_64-unknown-linux-gnu to cc

...then that specifically can definitely have observable effects since the compiler driver can be taught different linkage based on the vendor, like how clang does actually pass different final ldflags based on the vendor (some of those are even upstream; like iirc -redhat- is treated specially, -alpine- and a few others default to -z,-now and friends, etc)

if what you mean is to grab the rust triple, replace the vendor-linux-* with unknown-linux-*, match that to the current hardcoded list for what that was added for, but then still pass the original vendor-linux-* triple to cc --target, then that should work fine, i guess, but the more i think about this the more confused i get and it seems like it would just be undoing all the things the hardcoded fallback list was added for?

maybe merely using target-lexicon removes all these issues (and instead adds the issue of it adding to compile times which is why it was avoided in the first place, after reading the merge request). or maybe going back to the old fallback when CARGO_CFG_* vars aren't available is better than the new hardcoded list (and the non-fallback case is still definitely improved, so in all the cc-rs uses in build.rs things are done with the new code). but that's for you to decide- i don't actually know what would be best

@NobodyXu
Copy link
Collaborator

if what you mean is to grab the rust triple, replace the vendor-linux-* with unknown-linux-, match that to the current hardcoded list for what that was added for, but then still pass the original vendor-linux- triple to cc --target

Yeah it sounds like we'd have to use something like this.

It'd be great for rust to support this, because technically they use different flags for compilation.

@madsmtm
Copy link
Collaborator

madsmtm commented Nov 28, 2024

as far as detection, i can't think of why that wouldn't work (after all, it did before where as far as i know this was mostly ignored via the old parsing), as long as it doesn't have any weird implementation gotchas for you.

maybe merely using target-lexicon removes all these issues (and instead adds the issue of it adding to compile times which is why it was avoided in the first place, after reading the merge request).

The reason target-lexicon isn't used is because it only supports LLVM triples. And the reason why it only supports those is because rustc target triples are wildly inconsistent. They're basically just an opaque mapping from string -> TargetInfo, and any information gathered by trying to parse them is in general a doomed endeavour. And that's the implementation gotchas we had before ;)

All that said, I'd be fine with the workaround you propose yourself, i.e. basically do:

  • If not in build script.
  • And target triple not found in precompiled list.
  • .split('-') the target triple into parts.
  • Check if the the third part (usually OS) is linux.
  • Replace the second part (usually vendor) with unknown.
  • Check if we can now find the target triple in precompiled list.
  • Modify details in it (llvm_target and target_vendor) to match the actual target.

It's ugly, but that's how it is when working around vendors that don't upstream their modifications to rustc...

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

No branches or pull requests

4 participants