Skip to content

Use Roslyn apis to generate Platform Not Supported Assemblies - #40296

Merged
Anipik merged 5 commits into
dotnet:masterfrom
Anipik:NotSupportedAsm
Aug 18, 2020
Merged

Use Roslyn apis to generate Platform Not Supported Assemblies#40296
Anipik merged 5 commits into
dotnet:masterfrom
Anipik:NotSupportedAsm

Conversation

@Anipik

@Anipik Anipik commented Aug 4, 2020

Copy link
Copy Markdown
Contributor

Fixes #33631

@Anipik
Anipik requested review from ViktorHofer and ericstj August 4, 2020 01:05
@Dotnet-GitSync-Bot

Copy link
Copy Markdown
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ViktorHofer ViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would you mind adding comments to code paths which aren't self-explanory like the string replacement, the rooted check, etc. Also would it make sense to move the added infra code to a separate file instead, ie notsupported.targets?

@Anipik Anipik added the NO-REVIEW Experimental/testing PR, do NOT review it label Aug 4, 2020
@Anipik Anipik changed the title Use Msbuild to generate Platform Not Supported Assemblies [WIP]Use Msbuild to generate Platform Not Supported Assemblies Aug 4, 2020
@Anipik Anipik removed NO-REVIEW Experimental/testing PR, do NOT review it area-Infrastructure-coreclr Only use for closed issues labels Aug 13, 2020
@Anipik Anipik changed the title [WIP]Use Msbuild to generate Platform Not Supported Assemblies ]Use Roslyn apis to generate Platform Not Supported Assemblies Aug 13, 2020
@Anipik Anipik changed the title ]Use Roslyn apis to generate Platform Not Supported Assemblies Use Roslyn apis to generate Platform Not Supported Assemblies Aug 13, 2020
@Anipik
Anipik requested review from eerhardt and safern August 13, 2020 19:04
Comment thread src/libraries/Directory.Build.targets Outdated
Comment thread src/libraries/System.DirectoryServices/src/System.DirectoryServices.csproj Outdated
Comment thread src/libraries/System.Net.NameResolution/src/ExcludeApiList.PNSE.Browser.txt Outdated
@Anipik

Anipik commented Aug 14, 2020

Copy link
Copy Markdown
Contributor Author

System.IO.Filesystem tests are being shown as failures on wasm, but the log doesnt show any failing tests. @lewing and @steveisok are u aware of such issues ?
This Pr doesnt affect system.IO.Filesystem. so this should not fail

@ghost

ghost commented Aug 14, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

@Anipik

Anipik commented Aug 15, 2020

Copy link
Copy Markdown
Contributor Author

@akoeplinger @marek-safar can you help with this failure ?

[06:33:11] dbug: test[0]
      WASM-ERR: 
      
[06:33:11] dbug: test[0]
      WASM-ERR: Unhandled Exception:
      
[06:33:11] dbug: test[0]
      WASM-ERR: System.PlatformNotSupportedException: System.Diagnostics.Process is not supported on this platform.
      
[06:33:11] dbug: test[0]
      WASM-ERR:    at System.Diagnostics.Process.Dispose(Boolean disposing)
      
[06:33:11] dbug: test[0]
      WASM-ERR:    at System.ComponentModel.Component.Finalize()
      
[06:33:11] dbug: test[0]
      WASM-ERR: [ERROR] FATAL UNHANDLED EXCEPTION: System.PlatformNotSupportedException: System.Diagnostics.Process is not supported on this platform.
      
[06:33:11] dbug: test[0]
      WASM-ERR:    at System.Diagnostics.Process.Dispose(Boolean disposing)
      
[06:33:11] dbug: test[0]
      WASM-ERR:    at System.ComponentModel.Component.Finalize()
      
[06:33:11] info: test[0]
      06:33:11.3507301 Process exited with 255
      
XHarness exit code: 1001
XHarness artifacts: ./xharness-output

@akoeplinger

Copy link
Copy Markdown
Member

@Anipik it looks like the finalizer is throwing PNSE which shouldn't happen. Did something change with the PNSE assembly generator? GenAPI has code to omit the destructor when generating PNSE: https://github.com/dotnet/arcade/blob/89b5ce0df8958006752f50264eab47f6e4866081/src/Microsoft.Cci.Extensions/Writers/CSharp/CSDeclarationWriter.Methods.cs#L28-L35

@Anipik

Anipik commented Aug 15, 2020

Copy link
Copy Markdown
Contributor Author

yes we are adding the platform not supported exception to the destructor as well.

@Anipik

Anipik commented Aug 15, 2020

Copy link
Copy Markdown
Contributor Author

@akoeplinger

Copy link
Copy Markdown
Member

that's a problem then since finalizers aren't supposed to throw exceptions.

@Anipik

Anipik commented Aug 15, 2020

Copy link
Copy Markdown
Contributor Author

that's a problem then since finalizers aren't supposed to throw exceptions.

is there a specific reason behind everything throwing the exception beside destructor. The fix should be in tooling or in implementation ?

@akoeplinger

Copy link
Copy Markdown
Member

the problem with throwing from the destructor is that you can't catch that exception, e.g. the following code would still crash the process once the object is finalized:

try {
   var p = new ClassThatThrowsPNSE();
} catch (PlatformNotSupportedException) {
  // alternate logic
}

so yeah the PNSE generation tooling needs to ensure not to generate an exception for the destructor.

@safern

safern commented Aug 15, 2020

Copy link
Copy Markdown
Member

@Anipik I merged my change yesterday to mode the Drawing Converters down to System.Drawing.Common and that's using the old exclude api list argument, so that we don't forget to update it here as well.

@Anipik

Anipik commented Aug 17, 2020

Copy link
Copy Markdown
Contributor Author

@Anipik I merged my change yesterday to mode the Drawing Converters down to System.Drawing.Common and that's using the old exclude api list argument, so that we don't forget to update it here as well.

yeah i already added that.

@Anipik

Anipik commented Aug 17, 2020

Copy link
Copy Markdown
Contributor Author

Thanks @akoeplinger for the example. the ask seems reasonable, i can add this to the tooling.

@Anipik
Anipik merged commit d0efddd into dotnet:master Aug 18, 2020
@Anipik
Anipik deleted the NotSupportedAsm branch August 18, 2020 00:01
Anipik added a commit that referenced this pull request Aug 18, 2020
#40961)

* fixes on the runtime side

* address feedback

* update arcade version

* merge conflicts

* update the version
@ericstj

ericstj commented Aug 18, 2020

Copy link
Copy Markdown
Member

Yep, just for reference: https://docs.microsoft.com/en-us/visualstudio/code-quality/ca1065?view=vs-2019#finalizers

That said, how do you even get an instance of these types constructed (and thus run their finalizer) if all the constructors throw? N/M I see that we always run the finalizer even on partially constructed object. Interesting fact to file away.

@ericstj

ericstj commented Aug 18, 2020

Copy link
Copy Markdown
Member

Actually, I take that back. It looks like .NETFramework will run the finalizer on a partially constructed object but .NETCore will not. Perhaps mono should be changed to match .NETCore? Still this argues for making our behavior support the running of finalizers since some of these assemblies need to work on .NETFramework, but I will correct that fact filed away to remember that this is actually something a little more subtle than it seems.

@marek-safar

Copy link
Copy Markdown
Contributor

Made tracking issue for the finalizer behaviour on MonoVM -- #41272

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove multi-stage build for libraries when building from source

9 participants