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

feat: Add support for workspace.dependencies in cargo 1.64.0+ #5794

Merged
merged 2 commits into from
Oct 8, 2022

Conversation

poliorcetics
Copy link
Contributor

@poliorcetics poliorcetics commented Sep 26, 2022

I did not write tests because I don't know enough Ruby to do it, but I
will gladly accept directions on it.

Closes #5315

See the section on Cargo in https://github.com/rust-lang/rust/releases/tag/1.64.0 for more details about the workspace feature.

@poliorcetics poliorcetics requested a review from a team as a code owner September 26, 2022 14:43
@poliorcetics
Copy link
Contributor Author

Since Dependabot already handled dependencies like dep = { path = "path" } without a version, I expect that dependencies with workspace = true to work correctly out of the box. This makes this PR simply a matter of teaching Dependabot to look at [workspace.dependencies]

@Nishnha
Copy link
Member

Nishnha commented Sep 26, 2022

Hi @poliorcetics, thanks for this contribution!

I'm new to the Rust ecosystem, so I did some light reading on [workspace.dependencies] and it looks like the key is actually called [workspace.dependencies] so we should try matching on that instead of [workspace] and [dependencies] separately.

Dependabot will already include any dependencies under the [dependencies] key in cargo.toml files with

DEPENDENCY_TYPES.each do |type|
parsed_file(file).fetch(type, {}).each do |name, requirement|
next unless name == name_from_declaration(name, requirement)
next if lockfile && !version_from_lockfile(name, requirement)
dependency_set << build_dependency(name, requirement, type, file)
end

In order to get this merged in we will need to test that a project with [workspace.dependencies] in its cargo.toml has its dependencies parsed correctly.

Something similar to

context "with declarations in dependencies and build-dependencies" do
let(:manifest_fixture_name) { "repeated_dependency" }
describe "the last dependency" do
subject(:dependency) { dependencies.last }
it "has the right details" do
expect(dependency).to be_a(Dependabot::Dependency)
expect(dependency.name).to eq("time")
expect(dependency.version).to be_nil
expect(dependency.requirements).to eq(
[
{
requirement: "0.1.12",
file: "Cargo.toml",
groups: ["dependencies"],
source: nil
},
{
requirement: "0.1.13",
file: "Cargo.toml",
groups: ["build-dependencies"],
source: nil
}
]
)
end
end
end

The manifest referenced in that file is at https://github.com/dependabot/dependabot-core/blob/78afa00dbd274ed937d8ff70118d2d4335d39526/cargo/spec/fixtures/manifests/repeated_dependency

@poliorcetics
Copy link
Contributor Author

I'm new to the Rust ecosystem, so I did some light reading on [workspace.dependencies] and it looks like the key is actually called [workspace.dependencies] so we should try matching on that instead of [workspace] and [dependencies] separately.

This is not a single TOML key, This is a key and a subkey. The following samples are equivalent (Rust playground code for example)

[workspace.dependencies]
dep-name = "version"
[workspace]
dependencies = {  dep-name = "version"  }
workspace.dependencies = { dep-name = "version" }

So to parse [workspace.dependencies] in a Cargo.toml, I need to first look for workspace, then inside that, dependencies.

Thanks for the guidance on testing, I'll try to write one

@poliorcetics poliorcetics force-pushed the 5315-rust-cargo-workspace branch 2 times, most recently from f1b723f to 70e26f9 Compare September 29, 2022 13:56
Copy link
Member

@Nishnha Nishnha left a comment

Choose a reason for hiding this comment

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

This PR updates the version of Cargo we use in our Dockerfile since it introduces support for a new feature.

I thought about moving the check for workspace dependencies into our existing DEPENDENCY_TYPES.each loop in the file_parser, but it's more maintainable as it is and Cargo workspaces currently only support regular dependencies (as opposed to dev-dependencies and build-dependencies), so this is fine.

@poliorcetics
Copy link
Contributor Author

Thanks for the help and fixing my mistakes @Nishnha, it was greatly appreciated 😄

@poliorcetics
Copy link
Contributor Author

What's left to do ?

Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

I don't know a ton about Rust, but from what I understand this looks straightforward.

We follow a deploy-then-monitor-prod-then-merge strategy, so one of us will try to do that when we get a few spare cycles in the next few days.

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.

Rust: support workspace inheritance
3 participants