Skip to content

[release/7.0-rc1] [MONO] Make mono_marshal_ilgen_init a public MONO_API function again - #74660

Closed
github-actions[bot] wants to merge 3 commits into
release/7.0-rc1from
backport/pr-74658-to-release/7.0-rc1
Closed

[release/7.0-rc1] [MONO] Make mono_marshal_ilgen_init a public MONO_API function again#74660
github-actions[bot] wants to merge 3 commits into
release/7.0-rc1from
backport/pr-74658-to-release/7.0-rc1

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Backport of #74658 to release/7.0-rc1

/cc @lewing@naricc

Customer Impact

Testing

Risk

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

@lewing

Copy link
Copy Markdown
Member

We have a workaround in dotnet/macios#15788 so if that lands this can be retargeted to release/7.0

@dalexsoto

Copy link
Copy Markdown
Member

@lewing not sure what will be faster, our CI started going 🍌s today :( so I would say if you can keep the train going on this one as a fail safe might be better

@lewing

Copy link
Copy Markdown
Member

@lewing not sure what will be faster, our CI started going 🍌s today :( so I would say if you can keep the train going on this one as a fail safe might be better

I'm not stopping it but taking a new runtime package will put schedules at risk

@github-actions
github-actionsBotforce-pushed the backport/pr-74658-to-release/7.0-rc1 branch from 697be58 to 2045c58CompareAugust 26, 2022 18:17
@dalexsoto

dalexsoto commented Aug 26, 2022

Copy link
Copy Markdown
Member

@lewing Looks like we need this PR after all see:

dotnet/macios#15788 (comment)

I'm now a little bit concerned that this was the wrong fix.
When using the interpreter you link the mono-ilgen.a library and I think in that case the call to mono_marshal_ilgen_init has to stay. So maybe we need the runtime fix #74658 after all

https://github.com/xamarin/xamarin-macios/blob/028afc2eabb4af8b7087ebd7f9a6cf6f66ec96b8/tools/mtouch/Target.mtouch.cs#L1530

@carlossanlop

Copy link
Copy Markdown
Contributor

@mmitche do we still have runway to get this merged to RC1?

@lewing

Copy link
Copy Markdown
Member

can't merge this as is, it breaks wasm. alternate fix staged in #74675 while we keep working on this

@carlossanlopcarlossanlop added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 26, 2022
@lewinglewing closed this Aug 29, 2022
@carlossanlop
carlossanlop deleted the backport/pr-74658-to-release/7.0-rc1 branch August 30, 2022 16:55
@ghostghost locked as resolved and limited conversation to collaborators Sep 29, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-VM-meta-monoNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@lewing@dalexsoto@carlossanlop