C#: Clear existing data directory extracted from PCK

[scgm0]

Partial fix for #96299 (embed_build_outputs=true)

[raulsntos]

Thanks for the contribution. At a first glance this seems reasonable, but I'm a bit hesitant to deleting directories without some kind of confirmation. It may not be obvious to the user that this data directory is specific to each export and may be overridden, thus breaking the previous export.

We've had similar issues reported in the past about exporting to different platforms/architectures, which is why now the data directory name contains the name of the target platform and architecture. And there's even some users that are not aware that this directory is important and forget to include it when distributing the exported game.

So I'm thinking this may be a documentation issue, and maybe we should recommend that C# projects are always exported to an empty directory.

[raulsntos]

Not sure I trust this to always result in the correct path.

[scgm0]

In my tests, the 'path' is the relative path to the project's 'res' directory as the root (export_presets.cfg stores the 'export_path' is like that), and I just briefly tested the directory exported to 'res' under Linux and a directory that is not 'res', and maybe more testing is needed.

[paulloz]

Yeah, I'm not a big fan of nuking folders without user validation either. The export process should 100% be more documented and explicit about all of this. Also, isn't there already a dialog that opens when you export in a non-empty directory? We might be able to add details in there (e.g. saying that "if you're exporting on top of another architecture / aot mode / platform, you might experience unexpected issues" or something).

[scgm0]

Yeah, I'm not a big fan of nuking folders without user validation either. The export process should 100% be more documented and explicit about all of this. Also, isn't there already a dialog that opens when you export in a non-empty directory? We might be able to add details in there (e.g. saying that "if you're exporting on top of another architecture / aot mode / platform, you might experience unexpected issues" or something).

I don't think there's a problem with deleting the old folder, because the folder isn't designed to be the location the user should use, its contents are fixed and created by Godot at export time, and when 'embed_build_outputs' is enabled, it will be directly put into the 'pck' and will not be copied to the local cache directory until the user runs, and whether 'embed_build_outputs' is turned off or on'embed_build_ outputs', it will directly overwrite the previous results, and there is no convenient way for users to use this folder by themselves, it is created and used by godot automatically.

[scgm0]

Thanks for the contribution. At a first glance this seems reasonable, but I'm a bit hesitant to deleting directories without some kind of confirmation. It may not be obvious to the user that this data directory is specific to each export and may be overridden, thus breaking the previous export.

We've had similar issues reported in the past about exporting to different platforms/architectures, which is why now the data directory name contains the name of the target platform and architecture. And there's even some users that are not aware that this directory is important and forget to include it when distributing the exported game.

So I'm thinking this may be a documentation issue, and maybe we should recommend that C# projects are always exported to an empty directory.

If many users are unaware of the importance of this folder, then perhaps the embed_build_outputs option should be enabled by default? embed_build_outputs is now not only turned off by default, but also hidden in the advanced options. I feel that this will make many users not know that godot has this function.

[raulsntos]

embed_build_outputs is now not only turned off by default, but also hidden in the advanced options.

That's because it's a problematic option that causes issues in some platforms and it's likely to be removed in the future.

[scgm0]

embed_build_outputs is now not only turned off by default, but also hidden in the advanced options.

That's because it's a problematic option that causes issues in some platforms and it's likely to be removed in the future.

I think it works great and a single executable file is more convenient for distribution. What's the problem with it? #95312?

[raulsntos]

It's a potential security vulnerability, it leaves garbage in temporary directories that may never be cleaned after removing the game, in some platforms it prevents the exporter from signing the libraries and might attract unnecessary antivirus software attention.

The benefits of having a single-file executable don't outweigh the drawbacks. In some platforms we already have a single-file without this hack because of the nature of how their exporter works (e.g.: macOS exports a bundle, Android exports an APK/AAB). In my opinion, it's not worth having this "feature" which is only relevant to Windows and Linux, and has so many risks.

[scgm0]

It's a potential security vulnerability, it leaves garbage in temporary directories that may never be cleaned after removing the game, in some platforms it prevents the exporter from signing the libraries and might attract unnecessary antivirus software attention.

The benefits of having a single-file executable don't outweigh the drawbacks. In some platforms we already have a single-file without this hack because of the nature of how their exporter works (e.g.: macOS exports a bundle, Android exports an APK/AAB). In my opinion, it's not worth having this "feature" which is only relevant to Windows and Linux, and has so many risks.

Uh, Android now forces embed_build_outputs to be turned on, and it also has the problem in #96299, which is slightly different when embed_build_outputs is turned on and when it's not, one happens when the game is launched, and the other happens when the game is export, this pr contains both cases.
Problems during export can be solved by changing the export directory, but problems during startup can only be solved by deleting the old folder.

[raulsntos]

I think as long as we still support embed_build_outputs, the changes to godotsharp_dirs.cpp are fine and will be needed for #88803, because the changes in that PR make Android exports installed before the PR fail due to an outdated cache (same as #96299).

But we are still not convinced we want the changes to ExportPlugin.cs and that's blocking this PR. Could you separate the changes to godotsharp_dirs.cpp into a different PR?

[scgm0]

I think as long as we still support embed_build_outputs, the changes to godotsharp_dirs.cpp are fine and will be needed for #88803, because the changes in that PR make Android exports installed before the PR fail due to an outdated cache (same as #96299).

But we are still not convinced we want the changes to ExportPlugin.cs and that's blocking this PR. Could you separate the changes to godotsharp_dirs.cpp into a different PR?

I can split them into two PRs, but they are both assembly mixing problems, they just happen at different times, and I hope they can be fixed together.

[scgm0]

I think as long as we still support embed_build_outputs, the changes to godotsharp_dirs.cpp are fine and will be needed for #88803, because the changes in that PR make Android exports installed before the PR fail due to an outdated cache (same as #96299).

But we are still not convinced we want the changes to ExportPlugin.cs and that's blocking this PR. Could you separate the changes to godotsharp_dirs.cpp into a different PR?

Uh, if split a PR separately, can you come up with a name for this PR and the new PR?

[raulsntos]

I can split them into two PRs, but they are both assembly mixing problems, they just happen at different times, and I hope they can be fixed together.

I agree that they are very similar, but waiting for both to be fixed at the same time will only delay this PR since we are still undecided about parts of it. I think it'd be better to merge the parts that we can agree on.

Uh, if split a PR separately, can you come up with a name for this PR and the new PR?

Naming is hard, but I can try.

• For the changes to ExportPlugin.cs: C#: Clear existing data directory in export output
• For the changes to godotsharp_dirs.cpp: C#: Clear existing data directory extracted from PCK

[scgm0]

I can split them into two PRs, but they are both assembly mixing problems, they just happen at different times, and I hope they can be fixed together.

I agree that they are very similar, but waiting for both to be fixed at the same time will only delay this PR since we are still undecided about parts of it. I think it'd be better to merge the parts that we can agree on.

Uh, if split a PR separately, can you come up with a name for this PR and the new PR?

Naming is hard, but I can try.

* For the changes to `ExportPlugin.cs`: `C#: Clear existing data directory in export output`

* For the changes to `godotsharp_dirs.cpp`: `C#: Clear existing data directory extracted from PCK`

Thank you!

[raulsntos]

Looks good, thanks!

[akien-mga]

Thanks!