Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fix failures caused by ICU regression #24190

Merged
merged 1 commit into from
Apr 23, 2019
Merged

Fix failures caused by ICU regression #24190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/System.Private.CoreLib/shared/System/Globalization/CalendarData.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,11 @@ private bool LoadCalendarDataFromSystem(string localeName, CalendarId calendarId
// TODO-NULLABLE: these can return null but are later replaced with String.Empty or other non-nullable value
result &= GetCalendarInfo(localeName, calendarId, CalendarDataType.NativeName, out this.sNativeName!);
result &= GetCalendarInfo(localeName, calendarId, CalendarDataType.MonthDay, out this.sMonthDay!);
this.sMonthDay = NormalizeDatePattern(this.sMonthDay);

if (this.sMonthDay != null)
{
this.sMonthDay = NormalizeDatePattern(this.sMonthDay);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this the code path that could be affected by missing resources?

Copy link
Member Author

Choose a reason for hiding this comment

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

resources here doesn't mean the localized resources, it means the ICU resources (e.g. day or month names...etc.). So, this is the code path we read/get this ICU resources.

Copy link
Member

Choose a reason for hiding this comment

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

Understood. But this line is just accounting for sMonthDay not having been read. I'm asking if there are others besides that, where we assume some field was filled in but it wasn't because ICU was missing the relevant resource.

Copy link
Member Author

Choose a reason for hiding this comment

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

The method LoadCalendarDataFromSystem is the place we fill such properties. sMonthDay is the only property we read and access it before we check it. Note the rest of the code in this method doesn't try to access any of such properties we read. if you look at the other file CultureData.cs where I removed the assert, this is the palace we check which property we couldn't read and then fill it with the invariant values.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks.


result &= EnumDatePatterns(localeName, calendarId, CalendarDataType.ShortDates, out this.saShortDates!);
result &= EnumDatePatterns(localeName, calendarId, CalendarDataType.LongDates, out this.saLongDates!);
Expand Down
3 changes: 2 additions & 1 deletion src/System.Private.CoreLib/shared/System/Globalization/CalendarData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ internal CalendarData(string localeName, CalendarId calendarId, bool bUseUserOve

if (!LoadCalendarDataFromSystem(localeName, calendarId))
{
Debug.Fail("[CalendarData] LoadCalendarDataFromSystem call isn't expected to fail for calendar " + calendarId + " locale " + localeName);
// LoadCalendarDataFromSystem sometimes can fail on Linux if the installed ICU package is missing some resources.
// The ICU package can miss some resources in some cases like if someone compile and build the ICU package manually or ICU has a regression.

// Something failed, try invariant for missing parts
// This is really not good, but we don't want the callers to crash.
Expand Down