Skip to content

Conversation

@moosichu
Copy link
Contributor

Pull request #16888 removed searching for libraries with no extensions when linking frameworks... this adds that feature back.

This fixes #17294.

@moosichu
Copy link
Contributor Author

For context, in the loop iterating over extensions was changed when that logic was moved to the frontend:

Removed:

for (&[_][]const u8{ ".tbd", ".dylib", "" }) |ext| {
    if (try MachO.resolveFramework(arena, dir, f_name, ext)) |full_path| {
        const info = options.frameworks.get(f_name).?;
        try libs.put(full_path, .{
            .needed = info.needed,
            .weak = info.weak,
            .path = full_path,
        });
        continue :outer;
    }
}

Added:

for (&[_][]const u8{ "tbd", "dylib" }) |ext| {
    test_path.clearRetainingCapacity();
    try test_path.writer().print("{s}" ++ sep ++ "{s}.framework" ++ sep ++ "{s}.{s}", .{
        framework_dir_path,
        framework_name,
        framework_name,
        ext,
    });
    try checked_paths.writer().print("\n {s}", .{test_path.items});
    fs.cwd().access(test_path.items, .{}) catch |err| switch (err) {
        error.FileNotFound => continue,
        else => |e| fatal("unable to search for {s} framework '{s}': {s}", .{
            ext, test_path.items, @errorName(e),
        }),
    };
    return true;
}

This change restores the previous behavior in main.zig.

Pull request ziglang#16888 removed searching for libraries with no extensions
when linking frameworks... this adds that feature back.
Copy link
Member

@kubkon kubkon left a comment

Choose a reason for hiding this comment

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

Nice, thanks! I think we may need the same treatment in the code responsible for resolving dylib dependencies:

fn accessLibPath(

cc @mikdusan

accessLibPath now has a `noextension` block to search for extension-less
libraries.
@moosichu moosichu requested a review from kubkon October 15, 2023 16:15
@moosichu
Copy link
Contributor Author

I'm not entirely sure the full context of how things are supposed to work when linking MachO, but hopefully I applied the changes in the right way! (if the changes to accessLibPath aren't desired then just merge the first commit 😅 ).

@mikdusan
Copy link
Member

mikdusan commented Oct 15, 2023

Framework primary artifacts are usually named as follows:

  1. Foo.framework/Version/Current/Foo.tbd
  2. Foo.framework/Version/Current/Foo

Do we have any concrete examples of macOS SDK (new and old) or 3rd-party frameworks with following pattern?

  1. Foo.framework/Version/Current/Foo.dylib

@moosichu
Copy link
Contributor Author

So should this change remove the .dylib path also? TBH I've not dived into enough frameworks to know! (and it's obviously very hard to prove the non-existence of something, especially if it's just very rare!).

@moosichu
Copy link
Contributor Author

https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/FrameworkAnatomy.html

This only mentions the primary artifacts having no extension, so is definetely good to go. I presume the .tbd extension is newer than this documentation? (Which was last updated in 2013 - so I presume that very well may be the case!).

So .dylib in the case for frameworks may never need to be handled then?

@kubkon in light of this - what changes would you like to see this PR contain? (what is accessLibPath - is the change I made in the additional commit still sensible in light of this? Had a look at the contexts it is used briefly but personally don't feel confident enough on this one in particular). I am sure that a minimum, the change in main.zig will fix a known issue.

@ghost
Copy link

ghost commented Oct 15, 2023

these are all the folders containing files with .dylib extensions in Frameworks folders in latest xcode. looks like there aren't any .dylib files directly in Foo.framework/Versions/Current like Foo.framework/Versions/Current/Foo.dylib
(note: folder A from below = folder alias Current)

find /Applications/Xcode.app/Contents -name "*.dylib" -type f -exec dirname {} \; | grep Framework

/Applications/Xcode.app/Contents/Developer/Platforms/AppleTVSimulator.platform/Developer/SDKs/AppleTVSimulator.sdk/System/Library/Frameworks/OpenGLES.framework
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk/System/Library/Frameworks/OpenGLES.framework
/Applications/Xcode.app/Contents/SharedFrameworks/SwiftPM.framework/Versions/A/SharedSupport/PluginAPI
/Applications/Xcode.app/Contents/SharedFrameworks/SwiftPM.framework/Versions/A/SharedSupport/PluginAPI
/Applications/Xcode.app/Contents/SharedFrameworks/SwiftPM.framework/Versions/A/SharedSupport/ManifestAPI
/Applications/Xcode.app/Contents/SharedFrameworks/SwiftPM.framework/Versions/A/SharedSupport/ManifestAPI
/Applications/Xcode.app/Contents/SharedFrameworks/SwiftPM.framework/Versions/A/SharedSupport/ManifestAPI
/Applications/Xcode.app/Contents/SharedFrameworks/SourceKit.framework/Versions/A/XPCServices/com.apple.dt.SKAgent.xpc/Contents/Frameworks
/Applications/Xcode.app/Contents/SharedFrameworks/SourceKit.framework/Versions/A/XPCServices/com.apple.dt.SKAgent.xpc/Contents/Frameworks
/Applications/Xcode.app/Contents/SharedFrameworks/SourceKit.framework/Versions/A/XPCServices/com.apple.dt.SKAgent.xpc/Contents/Frameworks
/Applications/Xcode.app/Contents/SharedFrameworks/SourceKit.framework/Versions/A/XPCServices/com.apple.dt.SKAgent.xpc/Contents/Frameworks
/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Versions/A/Libraries/usr/lib/swift/host
/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Versions/A/Libraries/usr/lib/swift/host
/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Versions/A/Libraries/usr/lib/swift/host
/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Versions/A/Libraries/usr/lib/swift/host
/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Versions/A/Libraries/usr/lib/swift/host
/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Versions/A/Libraries/usr/lib/swift/host
/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Versions/A/Libraries/usr/lib/swift/host
/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Versions/A/Libraries/usr/lib/swift/host
/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Versions/A/Libraries/usr/lib/swift/host
/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Versions/A/Libraries/usr/lib/swift/host
/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Versions/A/Libraries/usr/lib/swift/host
/Applications/Xcode.app/Contents/SharedFrameworks/DVTInstrumentsFoundation.framework/Versions/A/Resources
/Applications/Xcode.app/Contents/SharedFrameworks/DVTInstrumentsFoundation.framework/Versions/A/Resources
/Applications/Xcode.app/Contents/SharedFrameworks/AppIntentsSupport.framework/Versions/A/Resources
/Applications/Xcode.app/Contents/SharedFrameworks
/Applications/Xcode.app/Contents/SharedFrameworks/CoreSymbolicationDT.framework/Versions/A/Resources
/Applications/Xcode.app/Contents/SharedFrameworks/CoreSymbolicationDT.framework/Versions/A/Resources/libatos/libs
/Applications/Xcode.app/Contents/Frameworks
/Applications/Xcode.app/Contents/Frameworks

@mikdusan
Copy link
Member

mikdusan commented Oct 15, 2023

what changes would you like to see this PR contain? (what is accessLibPath - is the change I made in the additional commit still sensible in light of this? Had a look at the contexts it is used briefly but personally don't feel confident enough on this one in particular). I am sure that a minimum, the change in main.zig will fix a known issue.

diff --git a/src/main.zig b/src/main.zig
index 4b8061a52..5d7665356 100644
--- a/src/main.zig
+++ b/src/main.zig
@@ -6929,7 +6929,7 @@ fn accessFrameworkPath(
 ) !bool {
     const sep = fs.path.sep_str;
 
-    for (&[_][]const u8{ ".tbd", ".dylib", "" }) |ext| {
+    for (&[_][]const u8{ ".tbd", "" }) |ext| {
         test_path.clearRetainingCapacity();
         try test_path.writer().print("{s}" ++ sep ++ "{s}.framework" ++ sep ++ "{s}{s}", .{
             framework_dir_path,

... for MachO.accessLibPath() it doesn't look like any patches are required for it. Could you re-try the motivating libSDL case without patching MachO.zig ?

also I'd like to add the reason why I am scrutinizing .dylib for frameworks is because, although rare, frameworks may be static, and I found no examples of .a either except for some very poorly named stub libraries in Tcl and Tk frameworks. Basically, if we want to search for .dylib we also should search for .a, but searching for neither looks to be the right behavior!

@mikdusan
Copy link
Member

these are all the folders containing files with .dylib extensions in Frameworks folders in latest xcode. looks like there aren't any .dylib files directly in Foo.framework/Versions/Current like Foo.framework/Versions/Current/Foo.dylib (note: folder A from below = folder alias Current)

yup and seems to be consistent with SDKs all the way back to 10.3.9 .

@kubkon
Copy link
Member

kubkon commented Oct 15, 2023

I want all three extensions handled including .dylib. While Apple might not ship them, don't forget users can create and ship their own frameworks too.

@mikdusan
Copy link
Member

mikdusan commented Oct 15, 2023

I want all three extensions handled including .dylib. While Apple might not ship them, don't forget users can create and ship their own frameworks too.

What about .a ?

So, { ".tbd", ".dylib", ".a", "" } ?

@kubkon
Copy link
Member

kubkon commented Oct 16, 2023

I want all three extensions handled including .dylib. While Apple might not ship them, don't forget users can create and ship their own frameworks too.

What about .a ?

So, { ".tbd", ".dylib", ".a", "" } ?

That one needs more thought - does search strategy impact search here? I would like to leave that one be for the moment until we actually thoroughly investigate static frameworks.

@moosichu
Copy link
Contributor Author

what changes would you like to see this PR contain? (what is accessLibPath - is the change I made in the additional commit still sensible in light of this? Had a look at the contexts it is used briefly but personally don't feel confident enough on this one in particular). I am sure that a minimum, the change in main.zig will fix a known issue.

diff --git a/src/main.zig b/src/main.zig

index 4b8061a52..5d7665356 100644

--- a/src/main.zig

+++ b/src/main.zig

@@ -6929,7 +6929,7 @@ fn accessFrameworkPath(

 ) !bool {

     const sep = fs.path.sep_str;

 

-    for (&[_][]const u8{ ".tbd", ".dylib", "" }) |ext| {

+    for (&[_][]const u8{ ".tbd", "" }) |ext| {

         test_path.clearRetainingCapacity();

         try test_path.writer().print("{s}" ++ sep ++ "{s}.framework" ++ sep ++ "{s}{s}", .{

             framework_dir_path,

... for MachO.accessLibPath() it doesn't look like any patches are required for it. Could you re-try the motivating libSDL case without patching MachO.zig ?

also I'd like to add the reason why I am scrutinizing .dylib for frameworks is because, although rare, frameworks may be static, and I found no examples of .a either except for some very poorly named stub libraries in Tcl and Tk frameworks. Basically, if we want to search for .dylib we also should search for .a, but searching for neither looks to be the right behavior!

Yeah so I already tested with just patching main.zig and that fixed the motivating use case for me.

@moosichu
Copy link
Contributor Author

moosichu commented Oct 16, 2023

So I want to confirm, in terms of this PR, we want this commit (exactly as-is.. leaving the .dylib stuff there):
8870ead
but not this:
023d1ac

So I should go ahead and remove that second one? Is that the correct takeaway from this conversation @kubkon @mikdusan?

I did validate that the first one was all that I needed to get SDL linking again.

Copy link
Member

@kubkon kubkon left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@kubkon kubkon merged commit 78f2ae7 into ziglang:master Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

zig not linking macos frameworks

3 participants