Add support for ISO8601 calendar and others #667

alblue · 2016-10-04T19:00:42Z

The list of valid keys did not include the ISO8601 calendar, which
meant that code running on Linux that used it would fail. Fix this
by adding the kCFISO8601Calendar and others into the list of known
constants valid for calendars, and add tests for the Gregorian
calendar, as well as for creating all other calendars.

Issue: SR-2551

alblue · 2016-10-04T19:05:37Z

CoreFoundation/Locale.subproj/CFCalendar.c

@@ -236,14 +236,23 @@ CFCalendarRef CFCalendarCopyCurrent(void) {
 }

 Boolean _CFCalendarInitWithIdentifier(CFCalendarRef calendar, CFStringRef identifier) {
-    if (identifier != kCFGregorianCalendar && identifier != kCFBuddhistCalendar && identifier != kCFJapaneseCalendar && identifier != kCFIslamicCalendar && identifier != kCFIslamicCivilCalendar && identifier != kCFHebrewCalendar && identifier != kCFChineseCalendar) {
+    if (identifier != kCFGregorianCalendar && identifier != kCFBuddhistCalendar && identifier != kCFJapaneseCalendar && identifier != kCFIslamicCalendar && identifier != kCFIslamicCivilCalendar && identifier != kCFHebrewCalendar && identifier != kCFRepublicOfChinaCalendar && identifier != kCFPersianCalendar && identifier != kCFCalendarIdentifierCoptic && identifier != kCFCalendarIdentifierEthiopicAmeteMihret && identifier != kCFCalendarIdentifierEthiopicAmeteAlem && identifier != kCFChineseCalendar && identifier != kCFISO8601Calendar && identifier != kCFIslamicTabularCalendar && kCFIslamicUmmAlQuraCalendar) {


I don't know at what point is stops being an optimisation to look for calendars here. Perhaps it should just be a subset of the available calendars?

Also, there's a _CFCalendarInitWithIdentifier and CFCalendarCreateWithIdentifier -- I'm not sure of the difference between them (but I've only implemented this one).

Looks like there's a typo at the end of the condition:
identifier != kCFIslamicTabularCalendar && kCFIslamicUmmAlQuraCalendar

Also, now that we are supporting all the calendar identifiers, do we need this if statement?

The if statement is about optimisation as much as anything else. The CFEqual check performs the equality comparison as well but is more details if that fails, so by doing a first-pass through known good identifiers the fast path can be optimised. Whether it's worth doing that for every calendar or not is debatable.

alblue · 2016-10-04T19:07:00Z

CoreFoundation/Locale.subproj/CFCalendar.c

+        else if (CFEqual(kCFRepublicOfChinaCalendar, identifier)) identifier = kCFRepublicOfChinaCalendar;
+        else if (CFEqual(kCFPersianCalendar, identifier)) identifier = kCFPersianCalendar;
+        else if (CFEqual(kCFIndianCalendar, identifier)) identifier = kCFIndianCalendar;
+        else if (CFEqual(kCFCalendarIdentifierCoptic, identifier)) identifier = kCFCalendarIdentifierCoptic;


It seems there was a name change from kCFxxxCalendar to kCFCalendarXXxx, but I tried to add the older one here. However, I couldn't find the kCFCopticCalendar, so I didn't use that. I wonder if there's benefit in normalising all of these to kCFCalendarXxxx instead?

I think you could define them here: https://github.com/apple/swift-corelibs-foundation/blob/master/CoreFoundation/Locale.subproj/CFLocaleKeys.c#L149

I'm open to that idea, but will wait for the recommended solution before I make any further changes here.

alblue · 2016-10-04T19:07:30Z

CoreFoundation/Locale.subproj/CFLocaleKeys.c

@@ -126,7 +126,7 @@ CONST_STRING_DECL(kCFCalendarIdentifierChinese, "chinese");
 CONST_STRING_DECL(kCFCalendarIdentifierRepublicOfChina, "roc");
 CONST_STRING_DECL(kCFCalendarIdentifierPersian, "persian");
 CONST_STRING_DECL(kCFCalendarIdentifierIndian, "indian");
-CONST_STRING_DECL(kCFCalendarIdentifierISO8601, "");
+CONST_STRING_DECL(kCFCalendarIdentifierISO8601, "iso8601");


Without this line change, the lookup for ISO8601 failed.

alblue · 2016-10-04T19:08:41Z

TestFoundation/TestNSCalendar.swift

+            Calendar.Identifier.republicOfChina
+            ] {
+                let calendar = Calendar(identifier: identifier)
+                XCTAssertEqual(identifier,calendar.identifier)


I'm not sure if there's a generic property I should be looking up here for all calendars to test that they are created appropriately? At the moment it's just checking the identifier and initialiser.

We can have tests that create calendars for all types and then verify the year, month and day (something like gettingDatesOnGregorianCalendar). That should be enough to make sure the underlying ICU is supporting all the types.

Not all calendars have the same concept of year, though.

At the moment, the identifier is the most correct property to be checking for equality. The other properties of the calendar object don't necessarily indicate whether or not the calendar is of a certain kind (meaning "gregorian" "buddist" etc.). Since this test doesn't care about checking the equality of two calendar instances and only cares about the calendar instance having the correct identity, I think this is sufficient for now.

pushkarnk · 2016-10-04T19:47:00Z

@alblue - Do the tests pass on Ubuntu 14.04 as well? We need to make sure the libicu there supports all these calendar types. Thanks.

The list of valid keys did not include the ISO8601 calendar, which meant that code running on Linux that used it would fail. Fix this by adding the kCFISO8601Calendar and others into the list of known constants valid for calendars, and add tests for the Gregorian calendar, as well as for creating all other calendars. Issue: SR-2551

alblue · 2016-10-04T20:56:02Z

The tests build and pass on Linux, and (for example) you can identify certain components from the calendar, such as the identifier itself and the firstWeekday (which is 1 for all of them). It's not clear if that's the default though; for example, .amSymbol throws for both .gregorian and .iso8601. However, whether the ICU library supports the calendars or not I would expect the Swift code to be the same; that is, if the ICU library is upgraded to support new calendars we should expect to be able to sue them without having to change the Swift code for Calendar again. If we want to dynamically figure out which ones work then we should probably have a separate run-time initialisation step that builds up a list of known calendars from the ICU library.

parkera · 2016-10-04T21:25:27Z

We really only should support the calendars identified by the constants exported in struct Calendar. It used to be the case that the identifier argument to init was a string, so you could pass whatever you liked. But in reality we only supported the known set anyway, which is why I changed it to an enum.

parkera · 2016-10-04T21:26:00Z

@swift-ci please test

alblue · 2016-10-04T21:33:40Z

I checked the set of identifiers at https://developer.apple.com/reference/foundation/nscalendar/1652687-calendar_identifiers against this set; I believe they are the same ones.

alblue · 2016-10-05T09:27:58Z

The test in CI failed on the newly added calendars test:

Test Suite 'TestNSCalendar' started at 21:47:18.879
Test Case 'TestNSCalendar.test_allCalendars' started at 21:47:18.879
TestFoundation: /home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift/include/swift/Runtime/../../../stdlib/public/SwiftShims/RefCount.h:252: bool StrongRefCount::doDecrementShouldDeallocate() [ClearPinnedFlag = false]: Assertion `newval + quantum >= RC_ONE && "releasing reference with a refcount of zero"' failed.
/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift/utils/build-script-impl: line 249: 12314 Aborted                 "$@"
/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift/utils/build-script: fatal error: command terminated with a non-zero exit status 134, aborting
Build step 'Execute shell' marked build as failure

I'll investigate further as to the cause, although I did not see this in the local Swift build that I tested it against.

alblue · 2016-10-05T10:22:56Z

Rebuilt with latest HEAD and do not see the same failure as shown by the CI. This output from Linux:

Test Suite 'TestNSCalendar' started at 11:04:07.958
Test Case 'TestNSCalendar.test_allCalendars' started at 11:04:07.958
Test Case 'TestNSCalendar.test_allCalendars' passed (0.0 seconds).
Test Case 'TestNSCalendar.test_gettingDatesOnGregorianCalendar' started at 11:04:07.958
Test Case 'TestNSCalendar.test_gettingDatesOnGregorianCalendar' passed (0.0 seconds).
Test Case 'TestNSCalendar.test_gettingDatesOnHebrewCalendar' started at 11:04:07.959
Test Case 'TestNSCalendar.test_gettingDatesOnHebrewCalendar' passed (0.0 seconds).

> :version
lldb-local-2016-10-05 (LLDB 99c98286cf, LLVM b9bd56d1b8, Clang 66cbe896f7, Swift-3.0 a05e8412fa)

I wonder if this is to do with the latest beta being installed onto the CI servers?

parkera · 2016-10-05T16:45:54Z

Let me try again...

parkera · 2016-10-05T16:46:00Z

@swift-ci please test

alblue · 2016-10-05T19:47:07Z

Same problem. I can remove the failing test which was added here but it's not clear to me why the overrelease is happening, nor why I can't reproduce the error on my Linux build locally.

alblue · 2016-10-06T10:52:24Z

OK, I'm able to reproduce this locally by running swift/utils/build-script -R -T --llbuild --swiftpm --xctest --foundation --libdispatch -- --reconfigure which shows the assertion failure.

alblue · 2016-10-06T12:12:38Z

So the bug occurs when iterating across the fourth calendar item. Using this in the for loop:

 42.                 let calendar = Calendar(identifier: identifier) 
 43.                 print("identifier: \(identifier)") 
 44.                 print("\(identifier == calendar.identifier)")

prints out:

identifier: buddhist
true
identifier: chinese
true
identifier: coptic
true
identifier: ethiopicAmeteAlem
true
repl_swift: /home/ablewitt/swift-build/swift/include/swift/Runtime/../../../stdlib/public/SwiftShims/RefCount.h:252: bool StrongRefCount::doDecrementShouldDeallocate() [ClearPinnedFlag = false]: Assertion `newval + quantum >= RC_ONE && "releasing reference with a refcount of zero"' failed.

but then when running again, it goes through that calendar without any issues:

identifier: buddhist
true
identifier: chinese
true
identifier: coptic
true
identifier: ethiopicAmeteAlem
true
identifier: ethiopicAmeteMihret
true
identifier: gregorian
true
identifier: hebrew
true

pushkarnk · 2016-10-06T12:50:00Z

@alblue : Can we please keep this PR open while the apparent ARC issue is looked into?

alblue · 2016-10-06T14:08:40Z

Sorry, didn't mean to close the request - hit the wrong button.

I have raised https://bugs.swift.org/browse/SR-2879 to look into the release issues, and in the meantime, commented out some of the list contents so that we don't see the four calendars in a row issue that we were seeing earlier. This may allow the test to pass on the Swift CI in the meantime.

alblue · 2016-10-07T13:44:09Z

Looks like I can't reopen this one for some reason. I'll create a new pull request.

alblue · 2016-10-07T19:28:41Z

Since I can't re-open this, I've created #677 instead. I've rebased this on the calendar fix, so hopefully the test won't fail like it did last time.

alblue commented Oct 4, 2016

View reviewed changes

alblue closed this Oct 6, 2016

alblue mentioned this pull request Oct 4, 2016

[SR-2551] Calendar initializer crashes with an error when using Swift binaries 8/30 & 9/01 #4451

Closed

Add support for ISO8601 calendar and others #667

Add support for ISO8601 calendar and others #667

Uh oh!

Conversation

alblue commented Oct 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pushkarnk Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pushkarnk Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pushkarnk commented Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alblue commented Oct 4, 2016

Uh oh!

parkera commented Oct 4, 2016

Uh oh!

parkera commented Oct 4, 2016

Uh oh!

alblue commented Oct 4, 2016

Uh oh!

alblue commented Oct 5, 2016

Uh oh!

alblue commented Oct 5, 2016

Uh oh!

parkera commented Oct 5, 2016

Uh oh!

parkera commented Oct 5, 2016

Uh oh!

alblue commented Oct 5, 2016

Uh oh!

alblue commented Oct 6, 2016

Uh oh!

alblue commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pushkarnk commented Oct 6, 2016

Uh oh!

alblue commented Oct 6, 2016

Uh oh!

alblue commented Oct 7, 2016

Uh oh!

alblue commented Oct 7, 2016

Uh oh!

Uh oh!

pushkarnk Oct 4, 2016 •

edited

Loading

pushkarnk Oct 4, 2016 •

edited

Loading

pushkarnk commented Oct 4, 2016 •

edited

Loading

alblue commented Oct 6, 2016 •

edited

Loading