Skip to content

Support .plugged within addons folder#37

Open
Tattomoosa wants to merge 3 commits into
imjp94:masterfrom
Tattomoosa:master
Open

Support .plugged within addons folder#37
Tattomoosa wants to merge 3 commits into
imjp94:masterfrom
Tattomoosa:master

Conversation

@Tattomoosa

@Tattomoosa Tattomoosa commented Sep 10, 2024

Copy link
Copy Markdown

Fixed the issue in #33 (comment) , 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

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Author

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 Tattomoosa changed the title Working implementation of moving .plugged to addons/gd-plug/.plugged Support .plugged within addons folder Sep 11, 2024
Changing the intent of this PR to only *support* .plugged within the
addons folder, instead of actually changing the defaults
@Tattomoosa

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown

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 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.

@Tattomoosa

Tattomoosa commented Sep 12, 2024

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
Author

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 #39 (comment) 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

Copy link
Copy Markdown

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.

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.

Godot LSP reports errors on all .plugged repo files

2 participants