Absolutify CXX as well as CC

[illicitonion]

Also add the --sysroot flag if needed

This is needed for in-tree CXX values because we chdir before execing the build script binary.

Fixes #950

[hlopko]

Can you think of a test?

[illicitonion]

@hlopko Added one - WDYT?

[UebelAndre]

One comment from me but otherwise looks good 😄

[UebelAndre]

This seems like a potential source of regressions. Maybe do this in a separate PR?

[ikalchev]

Hi all,

I may be wrong as I have little experience with bazel and less so with rules_rust, so bear with me.

I think the compiler_path needs to be transformed to a string before concatenating the sysroot options. As it is, the compiler_path is a PathBuf, so when we push the sysroot we get, e.g. /path/to/gcc/ --sysroot=//path/to/sysroot (note the trailing / after gcc).

Also, for some reason the cc_toolchain flags are not passed. We have a cc_toolchain that sets -D_GLIBCXX_USE_CXX11_ABI=0 and when we try to link a rust_library that depends on a cargo_build_script with a cc_binary, we get an e.g. undefined reference to some_fn(std::basic_string...), because the cc_binary tries to pass a std::basic_string but the cargo_build_script output binary expects a std::__cxx11::basic_string.

I managed to fix sysroot problem by using String before adding the sysroot options. Without this, the command expands to just the sysroot path. I haven't found a fix for the second issue.

I can test any potential fixes so let me know if I can help.

Best,
Ivan

[illicitonion]

I think the compiler_path needs to be transformed to a string before concatenating the sysroot options. As it is, the compiler_path is a PathBuf, so when we push the sysroot we get, e.g. /path/to/gcc/ --sysroot=//path/to/sysroot (note the trailing / after gcc).

Yes indeed, this was a good catch! Fortunately @scentini noticed this and fixed it in #976 :)

Also, for some reason the cc_toolchain flags are not passed. We have a cc_toolchain that sets -D_GLIBCXX_USE_CXX11_ABI=0 and when we try to link a rust_library that depends on a cargo_build_script with a cc_binary, we get an e.g. undefined reference to some_fn(std::basic_string...), because the cc_binary tries to pass a std::basic_string but the cargo_build_script output binary expects a std::__cxx11::basic_string.

This is definitely an issue too, but a separate one - if you're able to put together a PR to fix it that would be wonderful! I imagine we should be using CFLAGS/CXXFLAGS/LDFLAGS env vars to pass these, rather than concatenating them into the CC/CXX env vars, but I'm very willing to be convinced otherwise by someone who knows what they're talking about :)

FWIW there's a demo of getting these flags from the toolchain in a rule here - a PR that basically did this, in cargo_build_script, to set the appropriate env vars, would be great! (Though exactly how we'd go about testing it, I'm not quite sure...)

[ikalchev]

Sound good, thanks for the pointers! Let me see if I can draft something.

[ikalchev]

@illicitonion Any chance we can get a new create_universe release? When using the latest commit, there seems to be an API change that leads to:

Error: Failed to parse config file "crates.resolver_config.json"

Caused by:
unknown field `additional_registries`, expected one of `repository_name`, `packages`, `cargo_toml_files`, `overrides`, `crate_registry_template`, `target_triples`, `cargo`, `rust_rules_workspace_name`, `index_url` at line 2 column 28

[illicitonion]

@ikalchev We're not currently doing releases - how are you pulling the binary in and running your build?

[ikalchev]

We set the rules_rust repo to some CLN above this PR, define the DEFAULT_URL_TEMPLATE for the resolver to point to "https://github.com/bazelbuild/rules_rust/releases/download/crate_universe-13/crate_universe_resolver-{host_triple}{extension}" and then use the create_universe as:

crate_universe(
    name = "crates",
    cargo_toml_files = [
        ...
    ],
    packages = [
        ...
    ],
    resolver_download_url_template = DEFAULT_URL_TEMPLATE,
    resolver_sha256s = DEFAULT_SHA256_CHECKSUMS,
    supported_targets = [
        "x86_64-unknown-linux-gnu",
    ],
)

I guess we can build the resolver from source ourselves and host it somewhere but was hoping for an official release.

[illicitonion]

This reminds me, I think we reached rough consensus on releases, but haven't done any yet... I'll poke #415 :)

[ikalchev]

@illicitonion Is the guidance then to build the resolver from source?

[illicitonion]

@illicitonion Is the guidance then to build the resolver from source?

cargo build --release --manifest-path crate_universe/Cargo.toml should do the job!