Fix test_dependencies only checking dependency count

[roypat]

Signed-off-by: Patrick Roy roypat@amazon.co.uk

Changes

This commit changes to the to

1. fix the above issues (do not use pprint and check the actual dependency set instead of just its len)
2. ignore dependency versions, and only care about their names (meaning updates to a dependency will not trigger the test)

Reason

Only checking the number of dependencies allowed dependencies to be replaced without the test failing, as long as the overall number of lines in the dependencies.txt file was correct.

Note that due to the use of pprint automatically wrapping lines at 80 characters, the file contained more lines than dependencies, making the check even more inaccurate (dependencies with long paths/git urls werevbroken up into multiple lines, leading to a leniency of two dependencies that could be added without the test failing).

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

• All commits in this PR are signed (git commit -s).
• If a specific issue led to this PR, this PR closes the issue.
• The description of changes is clear and encompassing.
• Any required documentation changes (code and docs) are included in this PR.
• New unsafe code is documented.
• API changes follow the Runbook for Firecracker API changes.
• User-facing changes are mentioned in CHANGELOG.md.
• All added/changed functionality is tested.

---

• This functionality can be added in rust-vmm.

[andreeaflorescu]

The version of dependencies though might still be a valid check because in most cases you don't want to end up using more than one version of the crate even if they're compatible.

[roypat]

The version of dependencies though might still be a valid check because in most cases you don't want to end up using more than one version of the crate even if they're compatible.

Oh this is a very good point, but should maybe go into a separate test that verifies that no different versions of the same dependency are used. From my understanding the point test_num_dependencies was solely to make the process of adding new dependencies explicitly go through review.

[andreeaflorescu]

Oh this is a very good point, but should maybe go into a separate test that verifies that no different versions of the same dependency are used. From my understanding the point test_num_dependencies was solely to make the process of adding new dependencies explicitly go through review.

That is correct, it was serving the purpose of not using multiple versions of the same crate indirectly though. From my understanding (which might be veeeeery outdated) if you have for example vm-memory v0.9.0 and vm-memory v0.10.0 then this will show up as a "new dependency" because vm-memory would be counted twice. In the end is better to make it explicit instead like you suggested.

[roypat]

Oh this is a very good point, but should maybe go into a separate test that verifies that no different versions of the same dependency are used. From my understanding the point test_num_dependencies was solely to make the process of adding new dependencies explicitly go through review.

That is correct, it was serving the purpose of not using multiple versions of the same crate indirectly though. From my understanding (which might be veeeeery outdated) if you have for example vm-memory v0.9.0 and vm-memory v0.10.0 then this will show up as a "new dependency" because vm-memory would be counted twice. In the end is better to make it explicit instead like you suggested.

I just drafted such a test and interestingly enough it does not pass, even though we ran cargo update just 2 days ago. If we transitively depend on incompatible versions of the same crate (in our case, memoffset 0.6.5 and 0.7.1) there is nothing we can do about it, so I don't think we can add this as part of our CI (at least not easily)