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: circular dependencies error when there are none #1505

Merged
merged 8 commits into from
Jun 8, 2022

Conversation

browniebroke
Copy link
Contributor

@browniebroke browniebroke commented Jan 30, 2021

I have the same issue as the one reported in #1397. I'm not sure I can fix it, but I thought it would be helpful to at least write a test reproducing the issue.

Fix #1397

Implementation note

I initially had:

  • chained1 depends on chained2, chained3
  • chained2 depends on chained3
  • chained3 depends on nothing

But this worked fine, there was no problem. It looks like the problem depends on the plugin names, I can reproduce with:

  • chained3 depends on chained2 and chained1
  • chained2 depends on chained1
  • chained1 depends on nothing

Checklist

@sijis
Copy link
Contributor

sijis commented Jan 31, 2021

I really appreciate you putting this together.

Could you elaborate what you mean by

It looks like the problem depends on the plugin names

Maybe its late but I'm missing but both scenarios looks similar if not identical to me.

@browniebroke
Copy link
Contributor Author

I agree, the 2 cases should be equivalent, but one passes the test I wrote while the other doesn't. Since the only difference is with the names I assumed that it's playing a role into the problem.

I guess the plugin discovery system order plugins somehow related to their names. If the name ordering matches the dependency ordering, they can start, otherwise they can't.

@browniebroke browniebroke marked this pull request as ready for review January 31, 2021 17:54
Comment on lines +172 to +175
assert (
"Error: Broken failed to activate: "
"'NoneType' object has no attribute 'is_activated'"
) in testbot.pop_message()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmcevoy14
Copy link

Hi @browniebroke any idea when this fix will be merged?

@browniebroke
Copy link
Contributor Author

No idea, no, it's not really up to me at this point. I think the next step would be for a maintainer to assess whether they are happy with my code of if they have any suggestions. This is probably not the only thing on their plate, though...

In the meantime, if you want to help, I'm sure they would appreciate if you could try my version out and see if it maybe has a bug in it. You can install from source with pip:

pip install -e git+https://github.com/browniebroke/errbot.git@36920efa7818d71d4d9dc839100385f384d4c665#egg=errbot

You can put the part after pip install in your requirements file too. If you find any problem or if it works like a charm, please report back here.

@sijis
Copy link
Contributor

sijis commented Jul 12, 2021

@browniebroke @tmcevoy14 Would you folks be able to see if this issue persist with the latest changes on master branch?

@sijis sijis force-pushed the fix/chained-dependencies branch 3 times, most recently from 3e7c4cb to 9b1486f Compare October 6, 2021 07:15
@sijis sijis force-pushed the fix/chained-dependencies branch from 61b32d6 to 228c737 Compare June 8, 2022 05:43
@sijis sijis merged commit 2b56971 into errbotio:master Jun 8, 2022
@browniebroke browniebroke deleted the fix/chained-dependencies branch June 8, 2022 16:52
sijis added a commit that referenced this pull request Jun 11, 2022
* test: reproduce bug with wrong circular dependencies

Add a new test case reproducing the problem reported in #1397

* chore: fix typos in comment and docstring

* fix: activate plugins in deterministic order

* test: update test case with latest changes

* fix: set while at True

* test: re-enable circular testing

* fix: pin version

* docs: add info to CHANGES

Co-authored-by: Sijis Aviles <sijis.aviles+github@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Circular Dependencies when there are none
3 participants