Added support for Freestanding Bundles reverse lookup #1570

pvieito · 2018-05-31T00:44:23Z

This enables finding Freestanding bundles directory from the executable path as specified on the FHS Bundles doc.
Unifies Windows implementation with other platforms.

- This enables to find freestanding bundles directories. - Unifies Windows and other platforms approach. - Initial implementation (there are some leaks).

parkera · 2018-05-31T19:24:45Z

CoreFoundation/PlugIn.subproj/CFBundle_Executable.c

    return executableURL;
 }

+static CFStringRef _CFStringRemovePrefixSuffix(CFStringRef str, CFStringRef prefix, CFStringRef suffix) {


This needs to have a Create in the name.

Maybe _CFBundleCopyNameByRemovingExcutablePlatformPrefixSuffix?

parkera · 2018-05-31T19:26:19Z

CoreFoundation/PlugIn.subproj/CFBundle_Executable.c

+                                   kCFCompareAnchored|kCFCompareBackwards);
+
+        CFStringReplace(mutableString, suffixRange, CFSTR(""));
+        return mutableString;


Should this function be calculating the range then just calling CFStringCreateWithSubstring instead?

Yes, that's a lot better, thanks!

parkera · 2018-05-31T19:27:28Z

CoreFoundation/PlugIn.subproj/CFBundle_Executable.c

-        CFIndex extensionLength = CFStringGetLength(_CFBundleSiblingResourceDirectoryExtension);
-        buffLen -= 4;
-        // If this is an _debug, we should strip that before looking for the bundle
-        if (buffLen >= 7 && (_wcsnicmp((wchar_t *)&buff[buffLen-6], L"_debug", 6) == 0)) buffLen -= 6;


It looks like support for _debug may have been lost in this code and above?

It is this still required by Core Foundation on Windows?

parkera · 2018-05-31T19:28:49Z

CoreFoundation/PlugIn.subproj/CFBundle_Executable.c

+
+        // Otherwise, use the executable name as bundle name (CF).
+        if (!bundleName) {
+            bundleName = executableName;


Add a CFRetain(executableName) here and CFRelease(bundleName) below to avoid leaking.

parkera · 2018-05-31T19:29:07Z

CoreFoundation/PlugIn.subproj/CFBundle_Executable.c

+        }
+
+        // Find the sibling `.resources` directory (CF.resources).
+        CFURLRef parentDirectory = CFURLCreateCopyDeletingLastPathComponent(kCFAllocatorSystemDefault, executableURL);


parkera · 2018-05-31T19:29:25Z

CoreFoundation/PlugIn.subproj/CFBundle_Executable.c

+#if FREESTANDING_BUNDLES
+    if (!url) {
+        CFURLRef executableURL = CFURLCreateWithFileSystemPath(kCFAllocatorSystemDefault, str, PLATFORM_PATH_STYLE, false);
+        CFStringRef executableName = CFURLCopyLastPathComponent(executableURL);


parkera · 2018-05-31T19:29:37Z

CoreFoundation/PlugIn.subproj/CFBundle_Executable.c

-        if (buffLen >= 7 && (_wcsnicmp((wchar_t *)&buff[buffLen-6], L"_debug", 6) == 0)) buffLen -= 6;
+#if FREESTANDING_BUNDLES
+    if (!url) {
+        CFURLRef executableURL = CFURLCreateWithFileSystemPath(kCFAllocatorSystemDefault, str, PLATFORM_PATH_STYLE, false);


parkera · 2018-05-31T19:30:01Z

CoreFoundation/PlugIn.subproj/CFBundle_Executable.c

+
+        // Find the sibling `.resources` directory (CF.resources).
+        CFURLRef parentDirectory = CFURLCreateCopyDeletingLastPathComponent(kCFAllocatorSystemDefault, executableURL);
+        CFStringRef siblingResourceDirectoryName = CFStringCreateWithFormat(kCFAllocatorSystemDefault, NULL, CFSTR("%@.%@"), bundleName, _CFBundleSiblingResourceDirectoryExtension);


More leaks... these all should be taken care of.

Yep, I am aware, work in progress :)

parkera · 2018-05-31T19:30:31Z

CoreFoundation/PlugIn.subproj/CFBundle_Internal.h

 #endif

+// Freestanding bundles are supported on all platforms.
+#if TRUE


This should not be enabled on Darwin.

I think this could be useful for things like CLI tools that may require some resources without being in an app bundle. In any case I will disable in Darwin for now.

CLI that require resources have their own mechanism on Darwin. Freestanding bundles are for FHS OSes and Windows only.

parkera · 2018-05-31T19:34:14Z

What's the plan here? Mainly a refactoring?

pvieito · 2018-05-31T19:49:25Z

Yes, a bit of cleaning, unifying the old freestanding Windows bundles with the new cross-platform implementation and allow reverse bundle lookup (finding the bundle resources folder from the executable location) which was not available.

This will enable using Bundle.main.url(forResource:withExtension:) to find resources on a ExecutableName.resources directory sibling to the executable on Linux.

millenomi · 2018-05-31T21:04:03Z

@pvieito Are you sure the failure for that mode wasn't #1568 ? Can you articulate what was the issue here?

pvieito · 2018-05-31T22:10:21Z

@millenomi No, you implemented the logic to search the Bundle executable from the Bundle directory for Freestanding and FHS bundles (in _CFBundleCopyExecutableURLInDirectory2). This PR implements the reverse logic for Freestanding ones, which allows to find the Bundle directory from the executable path (in _CFBundleCopyBundleURLForExecutablePath).

This is required for things like Bundle.main.resourcePath resulting in the sibling ExecutableName.resources directory as specified on the FHS Bundles doc.

Of course the #1568 fix is required, but this PR extends the implementation.

pvieito · 2018-05-31T22:30:33Z

CoreFoundation/PlugIn.subproj/CFBundle_Executable.c

        if (CFStringHasSuffix(exeName, CFSTR(".dylib"))) {
            CFStringRef bareExeName = CFStringCreateWithSubstring(kCFAllocatorSystemDefault, exeName, CFRangeMake(0, CFStringGetLength(exeName)-6));
-            newExeName = CFStringCreateWithFormat(kCFAllocatorSystemDefault, NULL, CFSTR("%@%@.dylib"), exeName, imageSuffix);
+            newExeName = CFStringCreateWithFormat(kCFAllocatorSystemDefault, NULL, CFSTR("%@%@.dylib"), bareExeName, imageSuffix);


This, I suppose was a bug due a typo. Am I correct?

Can we split fixes to the core from refactoring? The more that changes in a single PR, the more confusing differential diagnosis can be in the future.

Tests ability to find bundle path from executable path

pvieito · 2018-05-31T23:30:04Z

@millenomi Also, the reverse lookup for FHS Installed bundles remains undone, so you cannot retrieve resources from a Executable installed in /bin/ExecutableName from /share/ExecutableName.resources with Bundle.main.url(forResource:withExtension:). For now, the bundle directory is “detected” at /bin/.

millenomi

I'd rather have a more focused patch for each issue than a single redo that tries to address multiple things at once.

millenomi · 2018-06-01T20:17:40Z

CoreFoundation/PlugIn.subproj/CFBundle_Executable.c

 #endif

-#if !DEPLOYMENT_RUNTIME_OBJC && !DEPLOYMENT_TARGET_WINDOWS && !DEPLOYMENT_TARGET_ANDROID
+#if FHS_BUNDLES


While it looks cleaner, the use of explicit platforms was intentional — we need to see what code is called on what paths, and obscuring those platforms makes us no favor. It's long but useful.

(In addition, as a note for the future since we should remove this: we do not introduce macros without prefixes. If we wanted to keep this, it should've been _CF_… since it's internal.)

I have changed it but added a comment (/* FHS_BUNDLES */ or /* FREESTANDING_BUNDLES */) in each appearance so all cases are easily searchable. They were already out of sync (some of the FHS_BUNDLES cases had not the DEPLOYMENT_TARGET_ANDROID and some had them).

millenomi · 2018-06-01T20:18:54Z

CoreFoundation/PlugIn.subproj/CFBundle_Executable.c

        if (CFStringHasSuffix(exeName, CFSTR(".dylib"))) {
            CFStringRef bareExeName = CFStringCreateWithSubstring(kCFAllocatorSystemDefault, exeName, CFRangeMake(0, CFStringGetLength(exeName)-6));
-            newExeName = CFStringCreateWithFormat(kCFAllocatorSystemDefault, NULL, CFSTR("%@%@.dylib"), exeName, imageSuffix);
+            newExeName = CFStringCreateWithFormat(kCFAllocatorSystemDefault, NULL, CFSTR("%@%@.dylib"), bareExeName, imageSuffix);


Can we split fixes to the core from refactoring? The more that changes in a single PR, the more confusing differential diagnosis can be in the future.

millenomi · 2018-06-01T20:19:43Z

CoreFoundation/PlugIn.subproj/CFBundle_Executable.c

            newExeName = CFStringCreateWithFormat(kCFAllocatorSystemDefault, NULL, CFSTR("%@%@"), exeName, imageSuffix);
        }
-        executableURL = CFURLCreateWithFileSystemPathRelativeToBase(kCFAllocatorSystemDefault, newExeName, kCFURLPOSIXPathStyle, false, urlPath);
+        executableURL = CFURLCreateWithFileSystemPathRelativeToBase(kCFAllocatorSystemDefault, newExeName, PLATFORM_PATH_STYLE, false, urlPath);


This is not needed, because this region is Darwin only. Using it can confuse the waters a bunch (why are we using a cross-platform macro here? What platforms does this code run on?)

millenomi · 2018-06-01T20:20:38Z

CoreFoundation/PlugIn.subproj/CFBundle_Executable.c

        CFRelease(newExeName);
        CFRelease(imageSuffix);
    }
+#endif


Since these blocks are long-ish, it's probably a good idea to decorate the #endifs.

millenomi · 2018-06-01T20:21:04Z

CoreFoundation/PlugIn.subproj/CFBundle_Executable.c

    if (!executableURL) {
-        executableURL = CFURLCreateWithFileSystemPathRelativeToBase(kCFAllocatorSystemDefault, exeName, kCFURLPOSIXPathStyle, false, urlPath);
+        // Try finding the executable with input name.
+        executableURL = CFURLCreateWithFileSystemPathRelativeToBase(kCFAllocatorSystemDefault, exeName, PLATFORM_PATH_STYLE, false, urlPath);


This code would be executed on Windows.

millenomi · 2018-06-01T20:26:51Z

CoreFoundation/PlugIn.subproj/CFBundle_Executable.c

+        if (_CFURLExists(siblingResourceDirectoryURL)) {
+            url = siblingResourceDirectoryURL;
+        }
+        else {


Style nit: same line as preceding }.

millenomi · 2018-06-01T20:27:23Z

TestFoundation/TestBundle.swift

+    func test_bundleReverseBundleLookup() {
+        _withEachPlaygroundLayout { (playground) in
+            if playground.layout == .fhsInstalled {
+                // Pending to be implemented.


millenomi · 2018-06-01T20:27:45Z

TestFoundation/TestBundle.swift


 import CoreFoundation

+#if DEPLOYMENT_RUNTIME_OBJC || os(Linux) || os(Android)


We have consolidated these imports elsewhere. Isn't it working?

No, unfortunately I need internal symbols that are not available with the @_exported import Foundation line in TestImports.swift. I also tried there to add @testable but it does not work.

A change that needs to be made after the last few updates to CI building: you need to guard use of @testable import like this:

#if NS_FOUNDATION_ALLOWS_TESTABLE_IMPORT #if DEPLOYMENT… @testable import … // etc #endif #endif

Tests that make use of internal symbols must also be bracketed by #if NS_FOUNDATION_ALLOWS_TESTABLE_IMPORT.

(Support for this to actually run those tests once bracketed is coming in the next attempt at #1597. Unfortunately, it has been temporarily reverted.)

Done! So, after that change is merged we should start a new test for this PR as the current test won't test anything, I suppose 🤔

millenomi · 2018-06-01T20:28:23Z

CoreFoundation/PlugIn.subproj/CFBundle_Internal.h

 #endif

-// FHS bundles are supported on the Swift and C runtimes, except on Windows.
+// Freestanding bundles are not supported on Darwin.


See above for the discussion about feature flags. (Short: no.)

millenomi · 2018-06-01T20:28:49Z

CoreFoundation/PlugIn.subproj/CFBundle_Internal.h

+#define FREESTANDING_BUNDLES 0
+#endif
+
+// FHS bundles are not supported on Darwin, Windows and Android.


Incorrect: they're supported on Darwin if we build with the Swift runtime. See earlier comment.

pvieito · 2018-06-13T19:48:53Z

Can anyone start the test, please? Also, any feedback after the last changes?

millenomi · 2018-06-13T19:59:45Z

@swift-ci please test

…NDATION_ALLOWS_TESTABLE_IMPORT

millenomi · 2018-06-13T23:15:47Z

@swift-ci please test

pvieito · 2018-06-20T18:36:02Z

@millenomi Are @testable imports already fixed?

millenomi · 2018-06-20T20:32:46Z

@pvieito they aren't yet, but you can wrap code in a #if NS_FOUNDATION_ALLOWS_TESTABLE_IMPORT block and it will be good for today. (You can test it locally before bracketing it with this #if with ninja test — @testable import will work locally, just not on CI.)

pvieito · 2018-06-20T20:37:41Z

@millenomi Ok, I already did it! Could you start a CI build/test to ensure the PR builds successfully, please?

alblue · 2018-06-22T15:48:59Z

@swift-ci please test

pvieito · 2018-08-04T12:38:26Z

Could we merge this? I was waiting for #1600 to add NS_FOUNDATION_ALLOWS_TESTABLE_IMPORT support but it seems it is not going to be merged any time soon.

alblue · 2018-08-07T12:02:35Z

@millenomi @parkera can you review the changes you requested and approve where appropriate?

millenomi · 2018-10-11T03:19:41Z

@pvieito Sorry for the long wait; it's been an intense summer. Are you still interested in chaperoning this patch?

pvieito · 2018-10-11T19:55:38Z

@millenomi Yes, I have merged the master changes and tested it locally.

pvieito · 2018-10-11T19:56:02Z

@swift-ci please test

millenomi · 2018-10-11T21:18:25Z

@swift-ci please test

pvieito · 2018-10-12T16:23:00Z

@millenomi Any idea why the tests did not start?

pvieito · 2018-10-12T16:23:04Z

@swift-ci please test

spevans · 2018-10-12T16:41:52Z

@swift-ci please test

pvieito · 2018-10-13T13:12:55Z

@swift-ci please test

pvieito · 2018-10-13T13:14:18Z

The macOS test seems to have failed due to external problems

spevans · 2018-10-13T13:20:14Z

@swift-ci please test

pvieito · 2018-10-14T14:41:36Z

@swift-ci please test // cc. @spevans Thanks!

@millenomi If the tests complete successfully the PR should be prepared for merging.

spevans · 2018-10-14T14:48:31Z

@swift-ci please test

pvieito · 2018-10-14T16:34:04Z

The tests have succeeded (even though GitHub is not reporting them):

pvieito · 2018-10-16T19:28:14Z

@millenomi Could you review and merge it (if ok)?

pvieito · 2019-04-17T23:04:50Z

Closing this PR as it is superseded by #2093.

[WIP] Added support for Freestanding Bundles reverse lookup

8bdfb2b

- This enables to find freestanding bundles directories. - Unifies Windows and other platforms approach. - Initial implementation (there are some leaks).

parkera requested changes May 31, 2018

View reviewed changes

Disabled freestanding bundles on Darwin

92b1639

Fixed leaks and cleaned _CFBundleCopyExecutableURLInDirectory2()

4706dd8

pvieito commented May 31, 2018

View reviewed changes

pvieito added 2 commits June 1, 2018 01:17

Added test to check Bundle reverse lookup

7f23670

Tests ability to find bundle path from executable path

Added note as FHS Bundle reverse lookup implementation is pending

ad95f2b

pvieito changed the title ~~[WIP] Added support for Freestanding Bundles reverse lookup~~ Added support for Freestanding Bundles reverse lookup May 31, 2018

millenomi suggested changes Jun 1, 2018

View reviewed changes

pvieito added 2 commits June 3, 2018 15:43

Implemented required changes

482ab99

Used guard instead of if

8327149

Guarded test which require @testable import of Foundation with NS_FOU…

f88a812

…NDATION_ALLOWS_TESTABLE_IMPORT

(refs #) Merged master

3c0192d

pvieito added 2 commits October 14, 2018 16:33

Updated conditional target naming

4ca3eb0

Updated conditional target naming

a8be484

pvieito mentioned this pull request Apr 11, 2019

Complete Freestanding Bundles support in all platforms #2093

Closed

pvieito closed this Apr 17, 2019


		import CoreFoundation

		#if DEPLOYMENT_RUNTIME_OBJC \|\| os(Linux) \|\| os(Android)

Added support for Freestanding Bundles reverse lookup #1570

Added support for Freestanding Bundles reverse lookup #1570

Uh oh!

Conversation

pvieito commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvieito May 31, 2018 • 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!

pvieito May 31, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parkera commented May 31, 2018

Uh oh!

pvieito commented May 31, 2018

Uh oh!

millenomi commented May 31, 2018

Uh oh!

pvieito commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvieito commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

millenomi left a comment

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!

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!

pvieito commented May 31, 2018 •

edited

Loading

pvieito May 31, 2018 •

edited

Loading

pvieito May 31, 2018 •

edited

Loading

pvieito commented May 31, 2018 •

edited

Loading

pvieito commented May 31, 2018 •

edited

Loading

millenomi commented Jun 20, 2018 •

edited

Loading