-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Conversation
@dotnet/dnceng could you please have a look at this one?
|
@tarekgh looking now |
@tarekgh checking the table, machine dci-mac-build-053 is back up to 42662 MB free disk space, so I'm guessing the DDFUN folks who clean this up already did. A simple |
Thanks @MattGal |
test OSX10.12 x64 Checked Innerloop Build and Test please |
@@ -581,7 +581,8 @@ extern "C" int32_t GlobalizationNative_GetLatestJapaneseEra() | |||
if (U_FAILURE(err)) | |||
return 0; | |||
|
|||
int32_t ret = ucal_getLimit(pCal, UCAL_ERA, UCAL_MAXIMUM, &err); | |||
ucal_set(pCal, UCAL_EXTENDED_YEAR, 9999); |
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.
Nit, this could maybe use a comment, as most likely future developers will have to dig up the change description to understand why 9999 is used here.
No need to reset the PR though.
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.
Or is it possible to add a test for this? That's even better than a comment, if possible.
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 already have many tests for this area. The only thing I cannot add test for is using future era (from ICU). but I have tested this manually. Our test coverage is fairly good in this area.
Approved for 2.1.10 and 2.2.4. |
@tarekgh Good to go? |
@mmitche yes, I merged it. thanks. |
This is straight port of the fix #21087
edit:
Description
We have fixed an issue with Japanese era in 3.0 through the PR #21087 and we got the request #21087 to port that to 2.1 and 2.2. I think it make sense to port the fix to the servicing branches to avoid any problem can happen when starting the new Japanese era in May 1st.
The issue we fixed is, on Linux we depend on ICU library for getting the list of the supported Japanese eras. We use ICU API ucal_getLimit to do that. Unfortunately, this API return the list of the eras up to the system clock time and not reporting any future eras even if the system support that. This means, if the system updated with the new era before May 1st we’ll not be able to handle this new era. We have raised the issue to ICU and this maybe end as by design for them or they may decide to change it but so far is not clear what they will do. Our fix is we get the era list more reliably by would be better just to get the era of Gregorian year 9999 which always should return the max era anyway.
Here is the opened PR’s for porting the fix
#20727
#20729
These fixes originally code reviewed by Eric Erhardt.
Customer Impact
This is important fix specific because we learned Japan is going to announce the era name by April which means the software companies will try to update their software before May 1st with the new era information. That create a risk if anyone using 2.1 or 2.2 can run into problems at that time.
Regression?
No.
Risk
The risk is really very low as the change is very scoped to Japanese calendar with the era scenario and the fix has been in 3.0 master branch for awhile too. So, the fix is exercised for a while now.