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

[HybridGlobalization] Include all globalization symbols #96684

Merged
merged 16 commits into from
Jan 10, 2024

Conversation

mkhamoyan
Copy link
Contributor

By this PR we are Including all globalization symbols.
For HybridGlobalization we will have empty placeholders in pal_placeholders.c and these methods will not be called from managed code.
These symbols are needed for having consistency between System.Private.CoreLib.dll and libSystem.Globalization.Native.a and for future to support also our ICU libs.

These changes should fix xamarin-macios build reported by #96251.

Contributes to #80689

@ghost
Copy link

ghost commented Jan 9, 2024

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

By this PR we are Including all globalization symbols.
For HybridGlobalization we will have empty placeholders in pal_placeholders.c and these methods will not be called from managed code.
These symbols are needed for having consistency between System.Private.CoreLib.dll and libSystem.Globalization.Native.a and for future to support also our ICU libs.

These changes should fix xamarin-macios build reported by #96251.

Contributes to #80689

Author: mkhamoyan
Assignees: mkhamoyan
Labels:

area-System.Globalization, os-ios

Milestone: -

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mkhamoyan mkhamoyan marked this pull request as ready for review January 9, 2024 14:44
@steveisok
Copy link
Member

I ran this against the branch and it no longer prints "Not Found".

cd packages/microsoft.netcore.app.runtime.mono.ios-arm64/9.0.0-alpha.1.23619.7/runtimes/ios-arm64/native
for symbol in $(monodis System.Private.CoreLib.dll | grep 'pinvokeimpl.*libSystem.Globalization.Native' | sed -e 's/.*as "//' -e 's/".*//'); do
	if ! nm libSystem.Globalization.Native.a|grep "_$symbol\$">/dev/null; then
		echo "NOT FOUND: $symbol"
	fi
done

@steveisok steveisok requested a review from rolfbjarne January 9, 2024 15:03
@jkotas
Copy link
Member

jkotas commented Jan 9, 2024

How are you going to implement opt-in to full ICU globalization with this approach?

I do not think that the change in this PR goes in the right direction. Instead, I think you should introduce a new library: libSystem.Globalization.Hybrid. When the hybrid globalization is turned on, link with this library. When the hybrid globalization is turned off, link with the default ICU-based libSystem.Globalization.

@steveisok
Copy link
Member

How are you going to implement opt-in to full ICU globalization with this approach?

I do not think that the change in this PR goes in the right direction. Instead, I think you should introduce a new library: libSystem.Globalization.Hybrid. When the hybrid globalization is turned on, link with this library. When the hybrid globalization is turned off, link with the default ICU-based libSystem.Globalization.

That's exactly what we need to do. I'd like to unblock xamarin asap, so is this an acceptable interim solution?

@jkotas
Copy link
Member

jkotas commented Jan 9, 2024

The best way to unblock xamarin-ios is to revert #93220 that introduced the problem. It is guaranteed to work.

@steveisok
Copy link
Member

The best way to unblock xamarin-ios is to revert #93220 that introduced the problem. It is guaranteed to work.

I agree that it's guaranteed to work, but #93220 is a pretty significant change that I would like to have out early. I feel this PR allows us to run with HG as the default and separates out how we're going to handle toggling between the two modes.

@jkotas
Copy link
Member

jkotas commented Jan 9, 2024

I see that this significant change was not tested end-to-end and that the build system interactions are poorly understood. I think it is likely that there are more blocking problems hidding behind this one. You can certainly try again if you would like.

A better way to push this change through: Introduce property for HG opt-in, let it flow through the system. Once it flows through the system, verify that end-to-end scenarios like building and running MAUI apps work well with HG turned on. Once we have a reasonable confidence that this works for end-to-end, flip the default for the property (one line change). A lot less stress, code flow blocked for weeks, etc.

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 10, 2024
@steveisok
Copy link
Member

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@steveisok
Copy link
Member

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

#include "pal_timeZoneInfo.h"

#ifdef DEBUG
#define assert_err(cond, msg, err) do \
Copy link
Member

Choose a reason for hiding this comment

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

We #include "pal_utilities.h" everywhere else to get these assert definitions. Why can't we do it here?

Copy link
Member

@steveisok steveisok Jan 10, 2024

Choose a reason for hiding this comment

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

The ios runtime build builds globalization into its runtime as opposed to using libSystem.Globalization. I couldn't figure out how to generate pal_config.h, which is required when using pal_utilities. We could either get that right now or remove this when we follow up with generating Hybrid and libSystem.Globalization libraries.

Copy link
Member

Choose a reason for hiding this comment

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

Add it to the cleanup list (#96328) ?

@mkhamoyan
Copy link
Contributor Author

Failures are not related to this PR changes.

@mkhamoyan mkhamoyan merged commit 72db600 into dotnet:main Jan 10, 2024
210 of 212 checks passed
@mkhamoyan mkhamoyan deleted the hybrid_include_symbols branch January 10, 2024 16:28
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
* Include all symbols for Globalization
Co-authored-by: Steve Pfister <steve.pfister@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Feb 10, 2024
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.

3 participants