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

Fix infinite loop when creating a graph for a project with cyclic dependencies #183

Merged

Conversation

Veetaha
Copy link
Contributor

@Veetaha Veetaha commented May 1, 2020

Fixes #184

I am not well acquainted with the codebase. Not sure how those KrateSpans are connected with bans::check, just created them analogously to existing tests.
Ideally, there should be some integration tests infra. I hand-rolled a test project in tests/test_data/cyclic_dependencies just for this case.

cc @Jake-Shadle

@Veetaha Veetaha force-pushed the fix/cyclic-deps-infinite-loop branch 2 times, most recently from d2d6ac8 to 3bb9c63 Compare May 1, 2020 20:04
@Veetaha Veetaha marked this pull request as draft May 1, 2020 20:05
@Veetaha Veetaha force-pushed the fix/cyclic-deps-infinite-loop branch from 3bb9c63 to 536bd34 Compare May 1, 2020 20:07
@Veetaha Veetaha force-pushed the fix/cyclic-deps-infinite-loop branch from 536bd34 to fb9b234 Compare May 1, 2020 20:14
@Veetaha
Copy link
Contributor Author

Veetaha commented May 1, 2020

I separated a test and a fix into two commits so that is evident that the test fails without the fix.
I am still not sure the fix will work in 100%, looks like the hash sets were created to handle this problem?
Anyway, after my amendments, cargo deny works fine with my initial project and with the test project too:

@Veetaha Veetaha marked this pull request as ready for review May 1, 2020 20:23
@Jake-Shadle
Copy link
Member

Yes, the hashset was meant to avoid this problem, but I think it might have gotten lost in the last refactor (or never worked!)

This was a great PR though, thanks for both adding a test and splitting it between broken and fixed!

@Jake-Shadle Jake-Shadle self-requested a review May 1, 2020 20:36
@Jake-Shadle Jake-Shadle merged commit d7cf73b into EmbarkStudios:master May 1, 2020
@Jake-Shadle
Copy link
Member

This was published in the 0.6.7 release, thanks again!

@Veetaha Veetaha deleted the fix/cyclic-deps-infinite-loop branch May 1, 2020 20:57
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.

Infinite loop during duplicate graph contstruction
2 participants