-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Ship a .framework version of Mono #53370
Conversation
src/mono/mono/mini/CMakeLists.txt
Outdated
set_property(TARGET mono-framework APPEND_STRING PROPERTY LINKER_LANGUAGE CXX) | ||
endif () | ||
set_property(TARGET mono-framework APPEND_STRING PROPERTY LINK_FLAGS " -Wl,-compatibility_version -Wl,2.0 -Wl,-current_version -Wl,2.0") | ||
target_sources(mono-framework PRIVATE "${mono-components-objects}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is going to be a mono framework for ios/tvos/catalyst that always statically links in the full non-stub versions of all the components. Is that what we want? We always want to support hot reload and diagnostics, @rolfbjarne ?
Or should each component be its own dynamic .framework, too? (in which case... it would be upto the xamarin-macios embedding code to hook mono_dl_open and locate the components for us)
Or maybe one mono.framework with all the components, and one with only stubs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
statically links
I have not read the PR yet... but the goal of a .framework
is that it's all dynamic (not static) linking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea mono is built as a dynamic thing, but it will include all the components statically as part of it. I left some comments on the Paper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there a hard limit to the number of .frameworks an app can have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always want to support hot reload and diagnostics,
Debug
- We do not care about the app bundle size [1]
- IOW all features (including HR and diagnostics) can be available in the binary (and be enabled/disabled by the app if required)
- We care about build and deploy times
- dynamic linking (used for frameworks) makes the build faster
- the framework (binary) will rarely change (deployed to sim/device once)
[1] unless it starts affecting build/deploy times :-)
Release
- We care about the app bundle size
- by default, we won't be using the
.framework
- using the static libraries
.a
allows the native linker / strip to remove a lot of code. Using the framework would create much larger apps.
- using the static libraries
- a release
.framework
is needed for app extensions- why ? it's smaller to share a single, larger
.framework
than to have a (even partial) runtime in 2 (or more) executables - main app and one/several extension(s)
- why ? it's smaller to share a single, larger
- by default, we won't be using the
- We do not care (within limits) about build times. Release builds being less common than debug ones
- We do not care about deploy times. Since release build are smaller then generally deploy faster anyway.
I do not see "hot reload" needed for release builds. Not entirely sure about "diagnostics" ...
Modularity is more important for the static libraries. The majority of apps ships without extensions (but that could change).
Or should each component be its own dynamic .framework, too?
There's a limit (at least a suggested one, never seen a rejection) to the number of user frameworks that an app should have. However a framework can have several dynamic libraries inside it... so we could control what's in or out.
Not sure if that's really needed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given there are quite a number of conditions and cases I wonder if it'd make more sense for Xamarin.iOS to create the .framework as part of the build when it is needed with just the right components? E.g. I've heard that we have some requirements to not ship the diagnostics TCP server code in release builds, even if it is not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can create Mono.framework, but afaik we can't create the corresponding dSYM, that has to be done on the machine that compiled the object files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done some work relating to debug symbol generation in #53240
dsyms for anything we generate and put in a nupkg should be available from the MS debug symbol service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rolfbjarne we do have the dSYM's and can put them into the runtime pack (or the symbols nuget), would you mind trying if you can create the .framework based off of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akoeplinger I created a very simple test case, and I was able to create a .framework
and the corresponding .framework.dSYM
from a .dylib
and its corresponding .dylib.dSYM
. I ran an executable that consumed the .framework
in lldb, and lldb was able to symbolicate functions in the .framework
. I wasn't able to make macOS' crash reporter symbolicate the framework functions though, so I'm not sure if the end result is actually correct, or if it just happens to work in lldb. One point is that Apple identifies .dSYM directories using a UUID (which shows up in the crash report), and if we create the .framework.dSYM
from the .dylib.dSYM
, those share the same UUID, and potentially things break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the PR itself it's not clear to me what ends up inside that directory ?
Any chance for someone to zip this up and attach it here ? thanks!
src/installer/pkg/sfx/Microsoft.NETCore.App/Microsoft.NETCore.App.Runtime.props
Outdated
Show resolved
Hide resolved
Except it's different on Catalyst, which is why Catalyst builds are breaking (I'm not accounting for stuff winding up in a more complex tree) |
src/mono/mono/mini/CMakeLists.txt
Outdated
|
||
|
||
if(TARGET_DARWIN) | ||
add_library(mono-framework SHARED $<TARGET_OBJECTS:monosgen-objects>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a conditional flag so its only built when building from mono.proj ?
We had a call about runtime components, and basically I think this is what we want: iOS Simulator runtime pack should include:
iOS Device runtime pack should include:
|
@lambdageek @rolfbjarne okay, how's that? |
It would be really nice to fix this. It is causing problems in several places that are difficult to resolve any other way. |
and NuGet/Home#10734 for some of the other cases |
Official build working as planned
|
@@ -381,6 +381,42 @@ if(NOT DISABLE_SHARED_LIBS) | |||
add_library(monosgen-shared-dac SHARED "mini-windows-dlldac.c") | |||
set_target_properties(monosgen-shared-dac PROPERTIES OUTPUT_NAME ${MONO_SHARED_LIB_NAME}-dac) | |||
endif() | |||
|
|||
if(BUILD_DARWIN_FRAMEWORKS) | |||
if(TARGET_DARWIN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be TARGET_DARWIN AND STATIC_COMPONENTS
.
I think this version of the code is only right if the components are static archives, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we should never build a .framework in the case of .dylib components? Didn't you say iOS Simulator should ship dylibs & build against stubs in #53370 (comment) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant to me this looks like the code only supports the static components case. I assumed a dynamic components version will be added in a follow-up PR. Maybe I misunderstood what's happening here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, LGTM.
I think there are a couple of follow-up things we need to do (/cc @steveisok):
- i'm still unclear if this supports dynamic (.dylib) components inside the .framework (which we want for the simulator case). if it doesn't that's a follow-up item (together with switching simulator runs to using dynamic components)
- We should update AppleAppBuilder to support frameworks and add a functional test.
@lambdageek we can't afford to wait on Rolf returning before iterating on this, given the snap date, so I'll merge it as-is and we can follow up for any changes as it's shown they're needed. Of your two followup concerns:
|
This is based on initial work by @akoeplinger, who did most of the CMake bits
Fixes #42846