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.

Sign up for GitHub

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

Jump to bottom

All plugins using sub-directives implemented with Dispatcher broken #339

Open
erikw opened this issue Jul 14, 2023 · 3 comments · May be fixed by #343
Open

All plugins using sub-directives implemented with Dispatcher broken #339

erikw opened this issue Jul 14, 2023 · 3 comments · May be fixed by #343

Comments

@erikw
Copy link

erikw commented Jul 14, 2023 •
edited
Loading

First, thanks for this great dotfiles manager! I recently migrated from my own >10y old semi-homebrewn solution with many clumsy shell scripts. Dotbot made it all much simpler for me! However there's currently a bug preventing some plugins from working...

As first reported at wonderbeyond/dotbot-if#1 (comment), it seems like all plugins that allow using arbitrary sub-expressions broke as of refactorings introduced in b5499c7.

Let's take for example the dotbot-if plugin that allows doing things like

- if:
    cond: 'true'
    met:
    - shell:
      - echo The condition was true

The met: allows any dotbot directive to be run, hereshell was used as the examples.

However as of the changes in b5499c7 where the Dispatcher was modified to no longer load all plugins by itself (used to be done with the helper _load_plugins), plugins relying on using the Dispatcher to parse and handle sub-directives does no longer work as other plugins are not loaded anymore. The plugins array is now an argument that will be passed in. The problem is that the plugins using the Dispatcher does not know all the other plugins to be loaded as this was done before in an outer context not available here in the plugin. The example above will result in

$ ./install -v
Action shell not handled

==> Some tasks were not executed successfully

as reported in wonderbeyond/dotbot-if#1. Please see this issue for details on how the mentioned refactor broke plugins using the Dispatcher.

Now we could say that the problem is not dotbot but the plugins, and maybe it is so that all plugins should update. But to what, what method should plugins use to parse sub-directive now? However as this did work before and no new major breaking version of dotbot was released, I would consider the problem being in dotbot and not the plugins right now.

I could be very wrong in my analysis, and maybe there's already a way in which plugins like dotbot-if could execute sub-directives with the Dispatcher or another class. Let's figure this out together, as plugins like if is kind of essential for dotbot!
@kurtmckee
Copy link
Contributor

Cross-posted to wonderbeyond/dotbot-if#1

@erikw Thanks for pinging me on this, and sorry for the late reply!

I made the changes in the mentioned commit so that it was possible to test dotbot in a single pytest run. Previously, the unit tests were implemented as bash scripts running in a virtual environment managed by vagrant. This design had the benefit of separating dotbot tests from the user's actual filesystem, but precluded any possibility of testing dotbot on platforms other than Linux.

I modified the plugin loading code as a part of a significant effort to move dotbot's test suite to pytest. This new plugin-loading design had the benefit that plugins wouldn't remain permanently loaded between dotbot runs, which allowed the test suite to better exercise dotbot's failure/fallback codepaths when a directive wasn't implemented by a plugin. However, it appears to have broken some functionality that plugins were relying on. 😞

I'll take some time to refamiliarize myself with the plugin loading and the code paths these plugins were relying on. My hope is that there's a simple way to add the lost functionality back in and still maintain isolation of plugin loading between tests (or, possibly, force plugins to unload during test teardown perhaps).

@kurtmckee kurtmckee mentioned this issue Jul 14, 2023
Closed
@erikw
Copy link
Author

erikw commented Jul 14, 2023

Yeah I looked at the refactoring a bit and I think it's all good reasons. Just unfortunate side-effect. I'm sure we can find a way to have the improved test setup, while still empowering plugins to do cool stuff!

Thanks for looking in to it. I currently have commented out some if directives in my dotbot config, but I look forward to be able to use them later!

kurtmckee added a commit to kurtmckee/pr-dotbot that referenced this issue Jul 28, 2023
@kurtmckee
Make Dispatcher a singleton after first instantiation
28fe35c 
This allows plugins to re-instantiate the Dispatcher
(or, now, use the instance cached in `current_dispatcher`)
without knowing what plugins have been loaded by dotbot.

This fixes plugins that want to dispatch to other plugins.

Fixes anishathalye#339
@kurtmckee kurtmckee linked a pull request Jul 28, 2023 that will close this issue
Open
@kurtmckee
Copy link
Contributor

I think that #343 will fix this issue.

kurtmckee added a commit to kurtmckee/pr-dotbot that referenced this issue Sep 10, 2023
@kurtmckee
Make Dispatcher a singleton after first instantiation
2549772 
This allows plugins to re-instantiate the Dispatcher
(or, now, use the instance cached in `current_dispatcher`)
without knowing what plugins have been loaded by dotbot.

This fixes plugins that want to dispatch to other plugins.

Fixes anishathalye#339
@flipphillips flipphillips mentioned this issue Nov 30, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make Dispatcher a singleton after first instantiation kurtmckee/pr-dotbot
2 participants
@kurtmckee @erikw