Skip to content

Add an ILLinkTrim.xml file for InteropServices.JavaScript.#37776

Merged
eerhardt merged 3 commits intodotnet:masterfrom
eerhardt:FixJSInteropTrim
Jun 12, 2020
Merged

Add an ILLinkTrim.xml file for InteropServices.JavaScript.#37776
eerhardt merged 3 commits intodotnet:masterfrom
eerhardt:FixJSInteropTrim

Conversation

@eerhardt
Copy link
Copy Markdown
Member

Fix #37775

Copy link
Copy Markdown
Contributor

@marek-safar marek-safar left a comment

Choose a reason for hiding this comment

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

I don't think descriptors are right approach here. Please use linker attributes instead, there is already linker attribute https://github.com/dotnet/runtime/blob/master/src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Runtime.cs#L16 which should handle this if does not work we need to fix it

@layomia layomia self-requested a review June 12, 2020 08:26
@vitek-karas
Copy link
Copy Markdown
Member

Discussed this offline with @marek-safar . It seems that Runtime is not the main entry point from JS to managed. That exists somewhere else (I don't know where). We should have a descriptor for that entry point. And then Runtime can be pulled in by adding DynamicDependency on that entry point (if needed).

Regarding:
https://github.com/dotnet/runtime/blob/master/src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Runtime.cs#L16

This should work if there's something else in the managed code which will pull in the Runtime and enough on it to also pull in the field it's on. I don't know enough about the code here to tell if this is supposed to work or not.

Copy link
Copy Markdown
Contributor

@marek-safar marek-safar left a comment

Choose a reason for hiding this comment

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

LGTM, we'll tweak our approach to use descriptors here (pushed one more here)

@eerhardt
Copy link
Copy Markdown
Member Author

The only failures are ARM64 timeouts which, according to #37623 (comment), @safern is looking into. Merging now.

@eerhardt eerhardt merged commit daf3820 into dotnet:master Jun 12, 2020
@eerhardt eerhardt deleted the FixJSInteropTrim branch June 12, 2020 18:34
@ericstj
Copy link
Copy Markdown
Member

ericstj commented Jun 12, 2020

Opened #37823 for arm64 timeouts.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 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.

System.Runtime.InteropServices.JavaScript.Runtime is not linker-safe

7 participants