Skip to content

Crate Universe: enable all features when generating metadata #1949

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

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

Conversation

golovasteek
Copy link
Contributor

@golovasteek golovasteek commented Apr 29, 2023

Closes #1939

Objective of this change is to "render" optional dependencies of workspace members. And at the same time do not include them in the all_crate_deps. Optional dependencies should be handled in the BUILD.bazel file by maintainer, in case they are required.

Design notes:

  • The optional dependencies should be resolved recursively, and we need to use cargo for it. Before this change resolution was made with all features disabled, and all the optional dependencies were not resolved.
  • In this change we configure cargo to enable all possible features, and resolve all dependencies.
  • The step above alone, will solve the problem of optional dependencies, but would add all optional dependencies unconditionally to the all_crate_deps()
  • In order to exclude the optional deps from crate deps, we need to filter them out from dependencies list. We do filtering of optional only for workspace members.
  • we need to to provide optional dependencies as a "workspace_members_deps", to ensure that appropriate alias will be created.

Caveats:

  • in the metadata.json resolved dependency name coverted from "hyphen-notation" to "snake_case_notation", so I had to normalize names.

Open topics:

  • Would be nice to test end-to-end: given a Cargo.toml,
    • ensure that dependencies are rendered, fetched, and aliases are created.
    • ensure that all_crate_deps do not include optional deps.
  • Unit test maybe?
    • given Cargo.toml ensure that metadata.json has resolved optinal dependencies
    • given metadata.json, ensure that defs.bzl and BUILD.bazel are generated properly.
  • CI tests are failing, but not sure how failed test related to my change.

@golovasteek golovasteek marked this pull request as ready for review April 30, 2023 13:15
@UebelAndre
Copy link
Collaborator

Thanks for working on this! Got a question for you 😄

Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

I think CI should be fixed now. Would you also be able to add a unit test for this? Something that shows optional dependencies can be correctly identified from some metadata?

@@ -142,6 +142,9 @@ pub struct CommonAttributes {
#[serde(skip_serializing_if = "SelectList::is_empty")]
pub proc_macro_deps_dev: SelectList<CrateDependency>,

#[serde(skip_serializing_if = "SelectList::is_empty")]
pub all_optional_deps: SelectList<CrateDependency>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of tracking all_optional_deps, can you either update Dependency to have an optional field that we can use when gathering workspace member deps, or can you track each kind of optional dependency separately?

@@ -27,6 +27,7 @@ rust_binary(
version = "0.12.0",
deps = all_crate_deps(normal = True) + [
":names",
"@crate_index_cargo_remote//:clap", # Clap is optional dependency and has to be added manually
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure at the moment, what this example is testing.

If I understand correctly, the clap crate is an optional dependency of the names crate. And in this case it is not supposed to be enabled by default.

Therefore we need to specify it manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue here is that clap is an optional dependency. But it is enabled by default feature.

And prior to my changes it was resolved and added to dependencies set.

After my changes, the dependency is resolved, but it is filtered out on the later stage, when we remove all optional dependencies.

I assume that the "defalut" features should be working out of the box. I'm looking into how to fix it.

@@ -715,4 +730,27 @@ mod test {
])
);
}

#[test]
fn generate_metadata_for_crate_with_optional_deps() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@UebelAndre , I'm trying to add test here. The idea of the test is to ensure that MetadataGenerator adds optional dependencies into the metadata.json and correspondingly we have them in CargoMetadata.

Is it a right test suite to add it?

  • Other unit-tests do not execute cargo, and rustc here, so the test is more suitable for integration-tests.
  • on the other hand, the whole metadata module is private and is not visible to the integration test.

What do you think?

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

Successfully merging this pull request may close these issues.

Crates Universe: The optional-only dependency is not fetched even if needed
2 participants