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

Don't set a default for UseRidGraph #49663

Closed
wants to merge 1 commit into from
Closed

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Jul 26, 2023

Part of #49486

@vitek-karas the string EnableUnsafeBinaryFormatterSerialization doesn't appear anywhere in the repo - is the ask that we set it to true globally so that the SDK doesn't default it to false?

@wtgodbe wtgodbe requested a review from a team as a code owner July 26, 2023 21:12
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI label Jul 26, 2023
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jul 26, 2023
@wtgodbe
Copy link
Member Author

wtgodbe commented Jul 26, 2023

@elinor-fung looks like we're still seeing ericsink/SQLitePCL.raw#543

@vitek-karas
Copy link
Member

I think that even if this change would "fix" it, it's not the right fix.
As described in the original issue #49486 the current behavior is an artifact of how we construct the shared framework:

  • we build a normal console app (with the name of the shared framework)
  • we patch some things up
  • we remove some files (like the executable and the dll produced for the app)
  • we add some files (version info and so on)
  • we package it as shared framework

What is showing through here is that "building a console app" is an SDK gesture which has specific meaning - SDK will set certain defaults for it. It will set different defaults for other "verticals", like ASP.NET, Blazor, WinForms, ....
None of these defaults should be encoded into the shared framework - shared frameworks should be agnostic to app types. So ideally we would not use SDK in "console app mode" to produce this, but that's a much bigger change.

I think the correct way to fix this would be to hook into the SDK into the right spot and modify the item group which contains all the runtime properties (about to be written into .runtimeconfig.json). I didn't try this but looking at the SDK it should be something like:

  • Hook a custom target before GenerateBuildRuntimeConfigurationFiles target
  • Clear the RuntimeHostConfigurationOption item group (which contain the properties to be written)
  • Optionally put back the MetadataUpdater item if we want to keep it for compat (I would remove it, as we did in runtime/WindowsDesktop, but that's a risk decision)

The runtime and WindowsDesktop use a target from Arcade which is slightly better, it still uses the same trick, but it doesn't run the full build of the app, it only sets SDK up as a console app and then it runs select targets from the SDK to get the files it wants. It also allows for easier customizations.

As to your specific question about EnableUnsafeBinaryFormatterSerialization - it comes from the SDK. The default for console apps in .NET 8 is to set this to false - the SDK does that for all console apps.

@wtgodbe
Copy link
Member Author

wtgodbe commented Jul 27, 2023

Closing in favor of #49682

@wtgodbe wtgodbe closed this Jul 27, 2023
@wtgodbe wtgodbe deleted the wtgodbe/runtimeconfig branch July 27, 2023 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants