CoreFoundation cleanup for Windows #1828
Conversation
|
@swift-ci please test |
compnerd
force-pushed
the
dependencies
branch
from
71c8070 to
dab3144
Compare
|
@swift-ci please test |
| #define _CF_EXTERN extern | ||
| #endif | ||
|
|
||
| #if defined(_USRDLL) |
There was a problem hiding this comment.
This is defined by the Windows SDK or compiler when building shared libraries?
There was a problem hiding this comment.
Its a slight abuse of the normal behaviour for Windows. _USRDLL is pretty commonly defined (by Visual Studio's default build rules), but it technically is meant for ATL/MFC applications building a COM server for a DLL (InProc server).
| @@ -57,10 +57,12 @@ CF_EXPORT void _CFRuntimeSetCFMPresent(void *a); | |||
| CF_EXPORT const char *_CFProcessPath(void); | |||
| CF_EXPORT const char **_CFGetProcessPath(void); | |||
| CF_EXPORT const char **_CFGetProgname(void); | |||
|
|
|||
| #if !TARGET_OS_WIN32 | |||
There was a problem hiding this comment.
We like to keep these in the positive sense if possible
There was a problem hiding this comment.
Hmm, would you prefer:
#if TARGET_OS_WIN32
#else
...
#endif
Trying to exhaustively enumerate the other targets seems like a failing exercise since most OSes are Unixy :-(. But, if that really is preferable, I can attempt to do that.
There was a problem hiding this comment.
Hm, maybe it's ok. I'm not sure if we are super consistent about it anyway.
| #if DEPLOYMENT_TARGET_WINDOWS | ||
| __CFWindowsNamedPipeInitialize(); | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Did we remove this completely?
There was a problem hiding this comment.
Nearly completely ... had two straggling references remaining which I just cleaned up :-).
| @@ -495,7 +495,7 @@ static CFTypeRef _CFErrorPOSIXCallBack(CFErrorRef err, CFStringRef key) CF_RETUR | |||
| if (!errStr) return NULL; | |||
| if (CFEqual(key, kCFErrorDescriptionKey)) return errStr; // If all we wanted was the non-localized description, we're done | |||
|
|
|||
| #if DEPLOYMENT_TARGET_MACOSX || DEPLOYMENT_TARGET_EMBEDDED || DEPLOYMENT_TARGET_WINDOWS | |||
There was a problem hiding this comment.
Hm, why is this concept not portable to Windows?
There was a problem hiding this comment.
Windows doesn't really have POSIX-y errors. We can translate errors using FormatMessage which will take care of the localization. However, Rather than trying to implement something for that, it seemed that _CFErrorPOSIXCallBack was invoked from one site, _CFErrorInitializeCallBackTable, which is used by CFError.*CallBackBlockForDomain which is currently not in use in Foundation ... so I thought I would take the lazy way out :-p.
| CFStringRef winDir = _CFGetWindowsAppleSystemLibraryDirectory(); | ||
| __tzZoneInfo = CFStringCreateWithFormat(NULL, NULL, CFSTR("%@\\etc\\zoneinfo"), winDir); | ||
| } | ||
| // TODO(compnerd) figure out how to initialize __tzZoneInfo |
There was a problem hiding this comment.
I think we'll need to make sure this works before merging right?
There was a problem hiding this comment.
So, the interesting thing is that there is __CFCopyWindowsTimeZoneList which will enumerate the data from the registry. We reference that in CFTimeZoneCopyKnownNames. I don't have the context on why CoreFoundation attempted to provide its own copy of the timezone data rather than using the system one on Windows (as it already uses the system provided one on Darwin, BSD, and Linux).
There was a problem hiding this comment.
I think our Windows version of CF used to bring its own, but using the system TZ data is almost certainly better, since it can change underneath us.
There was a problem hiding this comment.
Agreed, that's why I was okay with this being addressed subsequently when we can start playing with Foundation on Windows :-).
Use the CMake defined `_EXPORT` macro to determine if the symbols should be exported. Rely on `_USRDLL` to indicate if the compilation is meant for static or shared builds and mark the symbols for export or not to prevent the symbols from being re-exported from the consuming library.
This function is publicly visible and used by Foundation on Windows. Ensure that the symbol is defined.
This function does not have a definition. Furthermore, there is no special initialization required for the use of NamedPipes in Windows NT. Remove the straggling references.
…indows Remove the `CFCopySearchPathForDirectoriesInDomains` interface from the Windows build. This interface is not used anywhere in the Windows path and is not exported. The single use was in the POSIX error domain which does not apply to Windows.
The current source base does not have `_CFGetWindowsAppleSystemLibraryDirectory` defined. Remove the single use of the function which would attempt to provide a location for tzone.tab for time zone information. The current implementation is able to inspect the time zone information from the Windows registry. Remove this function.
These declarations are only implemented for the macOS target, so do not provide a declaration on other targets.
These are needed for the implementation on Windows. Provide the aliases on Windows as well.
The UID concept does not map to Windows very well where users are identified by a GUID and do not have a primary group. The user permission model is far richer and cannot be easily distilled to the simplistic Unix model. These are not currently used in swift-corelibs-foundation, so simply elide the interfaces for now.
The static CRT is fraught with peril. Simply assume that we will never support it - full ostrich mode enabled!
compnerd
force-pushed
the
dependencies
branch
from
dab3144 to
a497812
Compare
|
@swift-ci please test |
|
@parkera please LMK if I've missed anything else that needs to be addressed. |
This has most of the cleanup needed for using CoreFoundation for Windows. There are a couple of minor macro definitions that are needed, but that can be done as a follow up.