imjp94 / gd-plug

Minimal plugin manager for Godot
MIT License
217 stars 15 forks source link

Support .plugged within addons folder #37

Open Tattomoosa opened 2 months ago

Tattomoosa commented 2 months ago

Fixed the issue in https://github.com/imjp94/gd-plug/pull/33#issuecomment-2309466681 , in this implementation whether or not to include/exclude files is only determined from the plugin root. This seems like the sanest behavior to me. This fixes #32

For more information on what the issue with that implementation, default "addons" include was matching on "res://addons/gd-plug/.plugged" and attempting to copy all plugin files, then aborted when file overwrite was attempted.

This might need another change to handle the case of copying files from the project root, but I'm not aware of any case where gd-plug should ever be doing that, so not sure how to go about testing that case.

Jack-023 commented 2 months ago

Nice work on this fix. One thing I am noticing in testing this is that having the repos in the .plugged directory is causing conflicts as Godot tries to load both the copies in the addon directories as well as the original in .plugged. It might be a good idea to move the files rather than copying to avoid having the engine try and load them twice.

Tattomoosa commented 2 months ago

Hm I'm not seeing that, seems to play nice for me so far whether or not I've added a .gdignore file to .plugged. Godot should ignore hidden directories, and the LSP shouldn't print the warnings now that those are within the addons folder.

Godot version? Are you having this issue with any plugin or maybe some particular ones you can share? Seems to be improper behavior on Godot's side unless I'm missing something.

Tattomoosa commented 2 months ago

Thinking about what @imjp94 about automated migration. I have a proposal for supporting that in a more general way, but it should be a separate PR and not included in this which I'd consider a bug fix since the .plugged folder should probably be allowed anywhere within the project.

I've changed the name of this PR and pushed another commit so it only includes the fix for file copying behavior.

Jack-023 commented 2 months ago

I am running v4.3.stable.arch_linux. When I launch my project I see many errors for extensions in the form

  Class "PhantomCamera3D" hides a global script class.
  Failed parse script res://addons/gd-plug/.plugged/phantom-camera/addons/phantom_camera/scripts/phantom_camera_host/phantom_camera_host.gd
  Class "PhantomCameraHost" hides a global script class.
  Failed parse script res://addons/gd-plug/.plugged/phantom-camera/addons/phantom_camera/scripts/resources/camera_3d_resource.gd
  Class "Camera3DResource" hides a global script class.
  Failed parse script res://addons/gd-plug/.plugged/phantom-camera/addons/phantom_camera/scripts/resources/tween_resource.gd
  Class "PhantomCameraTween" hides a global script class.
  Failed parse script res://addons/gd-plug/.plugged/ThemeGen/addons/theme_gen/programmatic_theme.gd
  Class "ProgrammaticTheme" hides a global script class.

The addons are already being loaded from the matching addons/addon_name directory and then the same file is attempting to be loaded from the copy in .plugged. I am also seeing some other errors due to plugin configs being read from inside .plugged, for example

Failed parse script res://addons/gd-plug/.plugged/godot-addons/addons/aspect_ratio_resize_container/plugin.gd
  Preload file "res://addons/aspect_ratio_resize_container/aspect_ratio_resize_container.gd" does not exist.

In this case, the addons are from a repo that hosts multiple godot addons kenyoni-software/godot-addons where I am only choosing to download a subset of the addons using gdplug's include parameter.

    plug(
        "kenyoni-software/godot-addons",
        {
            "commit": "2b42b730ba82f04f8001c5acf1f56b42d22cb8d6",
            "include":
            [
                "addons/icon_explorer",
                "addons/licenses",
            ],
        }
    )

the .plugged folder should probably be allowed anywhere within the project.

I would have agreed with you but after this discussion https://github.com/godotengine/godot/pull/93889 the godot maintainers suggested that all third party code should be placed within the addons directory and we aren't able to disable warnings for directories outside of addons.

Tattomoosa commented 2 months ago

I've been trying but haven't been able to reproduce the issue with Godot parsing .plugged and reporting errors... Wonder if the cache is somehow messed up? Have you tried deleting the .godot cache and trying again? Does adding .gdignore to .plugged fix it?

But I did just sort of add a fix for the issue with kenyoni-software/godot-addons polluting the plugin root due to its internal addon folders - made it so an include/exclude can be specified with "res://" to tell gd-plug it should check for inclusion at the plugin root. (happy to make that a separate PR but I don't want to just make a ton of PRs)

I also changed the exclude behavior to quit recursing for that directory immediately upon spotting a path which should be excluded, this makes it much easier to debug what's going on just since the list of files it checks is shorter

I didn't commit changing the default include to "res://addons" to reflect this, but I think it should be

Tattomoosa commented 2 months ago

the .plugged folder should probably be allowed anywhere within the project.

I would have agreed with you but after this discussion godotengine/godot#93889 the godot maintainers suggested that all third party code should be placed within the addons directory and we aren't able to disable warnings for directories outside of addons.

I agree, but I think the way to do it is first add support for it, then add handling of orphaned .plugged directories, then change it.

I'm also wondering if with Godot's import issues it should be an option to simply not keep plugin repos around after installation. Figure keep the config in .plugged but only use it to decide whether to update or not. I have a proposal here https://github.com/imjp94/gd-plug/issues/39#issue-2520879679 which includes adding a _config method to res://plug.gd and this seems like another good use case for it, eg:

func _config():
  remove_after_install = true
Jack-023 commented 1 month ago

Does adding .gdignore to .plugged fix it?

This did fix the issues. I did not know about .gdignore, thanks! Perhaps gd-plug should create that file after running install.