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

Crates Universe: The optional-only dependency is not fetched even if needed #1939

Open
golovasteek opened this issue Apr 25, 2023 · 6 comments · May be fixed by #1949
Open

Crates Universe: The optional-only dependency is not fetched even if needed #1939

golovasteek opened this issue Apr 25, 2023 · 6 comments · May be fixed by #1949

Comments

@golovasteek
Copy link
Contributor

I’m trying to build a rust binary, that has some optional dependencies in the Cargo.toml, for my configuration I want to use these optional dependencies.
But it looks like the optional dependencies are not even fetched, and not added to the “@crate_index” repository.
That means that even if I specify them explicitly in the rust_binar binary:

rust_binary(
    name = "gui",
    srcs = glob(["src/**/*.rs"]),
    deps = all_crate_deps() + [
        "@crate_index//:astc-decode",
    ],
)

they can not be used:

ERROR: /Users/evgenypetrov/work/bazel_playground/rust/gui/BUILD.bazel:5:12: no such target '@crate_index//:astc-decode': target 'astc-decode' not declared in package '' defined by /private/var/tmp/_bazel_evgenypetrov/0e70737357a2a67453272d7916c8876e/external/crate_index/BUILD.bazel (Tip: use `query "@crate_index//:*"` to see all the targets in that package) and referenced by '//rust/gui:gui'
ERROR: Analysis of target '//rust/gui:gui' failed; build aborted: Analysis failed

Here is the repo, that reproduces the problem

https://github.com/golovasteek/bazel_playground/blob/6f2ade625c80a73a0fcdfc8f936cd46963674454/rust/gui/BUILD.bazel

@UebelAndre
Copy link
Collaborator

Hi, does this occur on 0.21.1?

@golovasteek
Copy link
Contributor Author

Yes. It still occurs on 0.21.1. I've update the example workspace with new release

https://github.com/golovasteek/bazel_playground/tree/main

@golovasteek
Copy link
Contributor Author

If add a dummy cargo crate, that depends on "astc-decode" unconditionally, then I can use it in the initial package as well.

Here is the example change

https://github.com/golovasteek/bazel_playground/pull/1/files

@gferon
Copy link
Contributor

gferon commented Apr 28, 2023

I think it works as designed. Without any changes, your gui crate, which has sdl2 marked as optional, is being processed with default features (similar to what cargo would do). If you want to fetch its dependencies with another feature, you need to use a crate.annotation in the WORKSPACE.

See golovasteek/bazel_playground#2 for a working example.

@UebelAndre maybe we could try to explain this in the documentation somewhere? I don't exactly know what would be the best place for that.

EDIT: I suppose we could rethink the whole think to mimick what Cargo.lock does, which contains all of the possible usable crates, but that would require a lot of effort, and is not worth it IMO because we already don't support features the way cargo does (since Bazel doesn't have this concept).

@UebelAndre
Copy link
Collaborator

I think I could agree that optional dependencies for workspace members should be rendered. But maybe not added to the dependencies api? Or maybe that's expanded to support an optional parameter that would include them? Either way, sounds like a good change to make if you wanted to work on a PR. I'm happy to collaborate and help push the change through if someone wanted to drive it 😄

golovasteek added a commit to golovasteek/rules_rust that referenced this issue Apr 29, 2023
@golovasteek
Copy link
Contributor Author

I've spent some learning the code. I've found where the dependencies are lost initially.
I'm not sure if and how I can add the test.
And how to check influence on dependencies_api.

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

Successfully merging a pull request may close this issue.

3 participants