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

[.NET] Disable output embedding on macOS, move it to the advanced options on other platforms. #90422

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Apr 9, 2024

  • Disable output embedding on macOS (it can't work, and have no benefits since macOS app is a bundle with multiple files anyway).
  • Hides output embedding options on macOS (always disable), iOS (should use iOS specific code path and ignore it) and Android (always enabled).
  • Move output embedding to the advanced options on Linux and Windows (while it can be used on these platforms, embedding and extracting native libs seems like an extremely bad idea, I would consider removing this option entirely, but using it at least should not be encouraged).

Note: removed some dead code and added ifdef to fix .Net build with werror=yes (unrelated to the issue).

Fixes (partially, .Net 8 AOT seems to be broken, but it's likely an unrelated issue) #90397

@bruvzg bruvzg added this to the 4.3 milestone Apr 9, 2024
@bruvzg bruvzg requested review from a team as code owners April 9, 2024 08:20
@akien-mga akien-mga changed the title [.Net] Disable output embedding on macOS, move it to the advanced options on other platforms. [.NET] Disable output embedding on macOS, move it to the advanced options on other platforms. Apr 9, 2024
@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Apr 9, 2024
@bruvzg
Copy link
Member Author

bruvzg commented Apr 9, 2024

For the reference, the embedding option was added as part of Android support - #73257, but it is available on other platforms as well. Enabling it most likely should not cause any issues on Linux, but on Windows it will prevent exporter from signing libraries and might attract unnecessary antivirus software attention. The only benefit seems to be exporting without extra file (apart for executable and .pck).

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

This looks good to me, although I'm not sure how the platform maintainers feel about adding more .NET-specific code to their areas. I think there's precedent in #88245 that supports this.

const String cs_type;
};

static String variant_type_to_managed_name(const String &p_var_type_name) {
Copy link
Member

Choose a reason for hiding this comment

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

This became unused with #87952 and it's OK to remove it.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for pointing it out, so I know this part of the commit should not be cherry-picked to 4.2.

platform/android/export/export_plugin.cpp Outdated Show resolved Hide resolved
platform/ios/export/export_plugin.cpp Outdated Show resolved Hide resolved
platform/macos/export/export_plugin.cpp Outdated Show resolved Hide resolved
@bruvzg bruvzg force-pushed the net_no_native_embed branch from ac24129 to bf558ad Compare April 9, 2024 14:47
@akien-mga
Copy link
Member

akien-mga commented Apr 9, 2024

This looks good to me, although I'm not sure how the platform maintainers feel about adding more .NET-specific code to their areas. I think there's precedent in #88245 that supports this.

Yeah ideally we'd have this better abstracted, so modules can keep their platform-specific code well encapsulated, and likewise platforms should be able to have some module-specific code well encapsulated.

But I don't have a clear idea for how to do this so for now I think it's fine to just hardcode a few checks. We can refactor down the line once there are more modules (especially third-party) that require this kind of hooks.


Note to self: Revert the csharp_script.cpp dead code removal when cherry-picking to 4.2, it's still used there.

@bruvzg
Copy link
Member Author

bruvzg commented Apr 9, 2024

Yeah ideally we'd have this better abstracted, so modules can keep their platform-specific code well encapsulated, and likewise platforms should be able to have some module-specific code well encapsulated.

Export options will need some refactoring anyway, advanced options support was recently added and seems to be only used for Android, and in #90375. But it is useful for the all platforms (some export options are rarely used), so some way for modules to control export option visibility can be added (I do not think there's any, and currently only export platform plugins can do it).

In case of this feature, I'm not sure if it should be an option at all (or it should be hardcoded to be used only for Android).

@akien-mga akien-mga merged commit d3d10b5 into godotengine:master Apr 9, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@raulsntos
Copy link
Member

In case of this feature, I'm not sure if it should be an option at all (or it should be hardcoded to be used only for Android).

After working on #88803 I'm not sure it should be used for Android either. As I understand it, this would include the build outputs for all architectures in the APK in a way that can't be stripped. So users end up downloading a bigger APK that contains all architectures when they only need the files for their target architecture.

This feature was implemented in #76305. Users were asking to include the exported C# files in the PCK to avoid having an extra directory. This is not relevant on Android where we already only have the APK, but it's still relevant for desktop platforms. @bruvzg Is there a way to include these files inside the PCK without the caveats you mentioned? (i.e.: not signing the libraries on Windows, etc.)

@bruvzg
Copy link
Member Author

bruvzg commented Apr 10, 2024

Is there a way to include these files inside the PCK without the caveats you mentioned? (i.e.: not signing the libraries on Windows, etc.)

Libs can be signed before packing. But personally, I would prefer to remove it completely, it is a pointless hack, it's leaving garbage in the AppData, and it's potential vulnerability since anything can replace files in AppData.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release platform:macos topic:dotnet topic:export
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants