Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automate copy-frameworks #2731

Open
wants to merge 32 commits into
base: master
Choose a base branch
from
Open

Automate copy-frameworks #2731

wants to merge 32 commits into from

Conversation

dimazen
Copy link
Contributor

@dimazen dimazen commented Mar 13, 2019

This PR resolves #2605.
Tested on a test project as well as two production projects - works as expected. However, I need your advise on:

  • How to better write tests for this case? For integration testing we would need a project with couple of frameworks and an executable to run otool -L. I don't know for sure, but MachO format seems to be quite stable, so no changes from otool expected.
  • I'm not sure whether check for static frameworks is reliable or not. frameworksInDirectory returns both static and dynamic frameworks. Attempt to filter them out by checking their MachHeader.fileType doesn't help, because both return MH_EXECUTE. I end up checking whether framework nested in the Static folder or not but is that ok?

@tmspzz
Copy link
Member

tmspzz commented Mar 13, 2019

How to better write tests for this case? For integration testing we would need a project with couple of frameworks and an executable to run otool -L. I don't know for sure, but MachO format seems to be quite stable, so no changes from otool expected.

Using otool is perfectly fine. You can add integration tests, check bats files.

I'm not sure whether check for static frameworks is reliable or not. frameworksInDirectory returns both static and dynamic frameworks. Attempt to filter them out by checking their MachHeader.fileType doesn't help, because both return MH_EXECUTE. I end up checking whether framework nested in the Static folder or not but is that ok?

MH_EXECUTE means they are executable files, not frameworks. MH_OBJECT is for static libraries or object files and MH_DYLIB for dynamically linked libraries.

Looking into the Static directory is ok because carthage has already separated static from dynamic libraries before hand.

@dimazen
Copy link
Contributor Author

dimazen commented Mar 13, 2019

@blender when I'm checking a Static Library (for Bolts in particular) file type, it reports Executable (surprisingly!) and not the MH_OBJECT which I was expecting. But I'll recheck. However, if Static component check is fine, then lets leave it as is.

I'll review bats files and ping you if needed.

@bwhiteley
Copy link
Contributor

Will this search for frameworks in FRAMEWORK_SEARCH_PATHS or only under Carthage/Build/<platform> ?

@dimazen
Copy link
Contributor Author

dimazen commented Mar 14, 2019

@bwhiteley it only looks at the Carthage/Build/<platform>, since that's the default way of the Carthage usage. Or there might be some issues by not evaluating FRAMEWORK_SEARCH_PATH?

@typfel
Copy link

typfel commented Mar 14, 2019

While working on changes to a framework I often drag in the framework in question as a subproject in the main project. I then modify the copy phase to take the dependency from the BUILT_PRODUCTS_DIR instead of Carthage/Build, this enables me to make changes to the framework and have them immediately available in the main project.

Would such a workflow also be possible when using this automated copy phase?

@tmspzz
Copy link
Member

tmspzz commented Mar 14, 2019

The manually specified list will still be there @typfel, @bwhiteley .

Maybe there should be a set union based on the framework name, where the manual list wins over the inferred list since I think @typfel use case would not work.

@dimazen
Copy link
Contributor Author

dimazen commented Mar 14, 2019

@typfel you shouldn't worry. This PR omits need to specify standard paths such as Carthage/Build/iOS/Alamofire.framework. It doesn't replace this functionality but automates whenever possible.
That's a perfect catch, however, to give a higher priority for any manually specified values. I'll incorporate those changes on the weekends, @blender.

@tmspzz
Copy link
Member

tmspzz commented Mar 14, 2019

@dimazen I would also consider adding a flag to enable this behavior, making it opt-in rather than default.

@bwhiteley
Copy link
Contributor

Here is how I would hope this feature would work...

  1. Add first level dependencies to Linked Frameworks and Libraries.
  2. One-liner build phase for copy frameworks with no input/output file lists.
  3. Copy frameworks phase builds the dependency graph recursively with otool -L.
  4. Frameworks are searched for in FRAMEWORK_SEARCH_PATH. This could be enabled with a command-line switch with the default behavior only looking in Carthage/Build/<platform>.
  5. Copy frameworks phase checks timestamps to skip frameworks that are already up-to-date.

(This is how our Carthage wrapper works, mentioned in #2605 . When it was mentioned in #2605 it was reading the project file to get the framework list, but has since changed to use otool -L)

The reason for #4 is similar to what @typfel said. We have a lot of internal frameworks. Often we work on the framework individually but sometimes we work on the framework in the context of a consuming app (or another framework). To work on a framework in the context of a consuming app, simply create a temporary workspace containing the app project and the framework project. Due to #4, if the framework project is in the workspace the built framework in BUILT_PRODUCTS_DIR is preferred to the Carthage-built framework. To switch back to the Carthage framework just close the workspace and open the app project instead of the workspace (since BUILT_PRODUCTS_DIR by default is different depending on whether you open the workspace or the app project). The nice thing about this approach is that no changes to the project are necessary to switch a framework into or out of "development mode" (by opening a workspace vs. the project). You don't have to adjust framework paths; it just automatically does what you would expect when the framework project is in a workspace . You can have multiple workspaces with different subsets of frameworks. Or you can quickly change which frameworks are in "development mode" by adding or removing projects to a workspace. Again, no project modifications necessary.

It would be great if the Carthage team found this behavior desirable because then we could use carthage directly in our copy frameworks build phase and remove a bunch of functionality from our wrapper tool. :)

I've seen at least three separate implementation of a tool to flip frameworks into "development mode" in a carthage environment, so I know we aren't the only group interested in this functionality.

Perhaps if #4 isn't part of this PR it could come later...

@tmspzz
Copy link
Member

tmspzz commented Mar 15, 2019

@bwhiteley are you sure that if you use a workspace copy-frameworks is needed?

@dimazen
Copy link
Contributor Author

dimazen commented Mar 15, 2019

@bwhiteley
3. For the recursively linked frameworks I'm going to consult with Cartfile.resolved to ignore frameworks that are out of scope. However, does development dependencies somehow reflected in the Cartfile.resolved or Cartfile? We definitely don't want to start linking automatically all of the frameworks we'll be able to find :)
4. As above I need to ensure, that linked frameworks listed within Cartfile or Cartfile.resolved (preferable).
5. That's definitely needs to be done as a separate PR :)

I gonna play around with development projects to see how they affect xcodebuild build settings to see what we can do with it. If you have a sample project - please attach it here :)

@blender what do you think about automatic linking of the development dependencies as well as recursive dependencies copying (i.e. FacebookSwift will automatically copy Facebook-ObjC and Bolts)? Should we add separate option for this?

  • --auto will utilise automatic linked frameworks inference and copying
  • --prefer-development-builds will be used to indicate that we wanna copy dependency from the build dir (or whatever env it'll be).

Option names are just an example (especially second one).

@bwhiteley
Copy link
Contributor

@bwhiteley are you sure that if you use a workspace copy-frameworks is needed?

If you always use a workspace (meaning you probably aren't using carthage to build the frameworks, only to resolve/fetch them) then you probably have the frameworks listed in Emedded Binaries, and you don't need a carthage copy-frameworks build phase. However, if you normally build the frameworks with carthage and open the project file, but sometimes use a workspace to temporarily have a framework in "development" mode, then the carthage copy-frameworks phase is needed to avoid needing to temporarily modify the project (such as temporarily adding the frameworks to Emedded Binaries with correct paths).

@bwhiteley
Copy link
Contributor

  1. For the recursively linked frameworks I'm going to consult with Cartfile.resolved to ignore frameworks that are out of scope. However, does development dependencies somehow reflected in the Cartfile.resolved or Cartfile? We definitely don't want to start linking automatically all of the frameworks we'll be able to find :)
  2. As above I need to ensure, that linked frameworks listed within Cartfile or Cartfile.resolved (preferable).

But Cartfile and Cartfile.resolved list repos, not frameworks. It is common for a repo to contain multiple frameworks or for the framework to have a different name from the repo. As a result, Cartfile.resolved can't be used to filter the frameworks.

A dependency graph recursively built with otool -L rooted at EXECUTABLE_PATH is sufficient to get all the necessary frameworks without pulling in extraneous frameworks.

@bwhiteley
Copy link
Contributor

It will also be useful to print the input files that were found to satisfy the graph with their full paths, as well as indicating any that were skipped because the corresponding output was already up-to-date. This could be enabled with a verbose switch, or just do it by default.

@dimazen
Copy link
Contributor Author

dimazen commented Mar 16, 2019

@bwhiteley
Here is what I found so far:

  • Automatic copying of the nested dependencies is possible. When I removed FBSDKCoreKit and Bolts from the Linked Frameworks Phase but copied them over to the app it was fine. What I'm not clear about is how canImport works in this case, because it STILL works even if framework is not linked explicitly. Also, NSClassFromString returns me correct classes. I would assume that everything is still fine, because dyld was able to load FBSDKCoreKit for FacebookCore but looking up at the @rpath/Frameworks/FBSDKCoreKit.framework/FBSDKCoreKit.
    My suggestion about Cartfile and Cartfile.resolved was just assumption. otool works great.

  • Regarding FRAMEWORK_SEARCH_PATH usage: it's a bit more tricky than I was thinking initially. What if user have specified multiple locations that contain a framework we're looking for? Which directory should we pick? I don't see for now how to make it explicit for user in which order we're going to traverse directories. What if one of the directories contains framework copy (different build/version) that we actually don't want to copy?
    However, relying on the manually specified script input file works fine, because it takes precednse over ./Carthage/Build/<platform>/.... Any suggestions on this ambiguity?

Also, can you provide me some sample project with development dependency you're talking about please? I don't seem to understand how it works (never worked like that before).

@bwhiteley
Copy link
Contributor

  • Regarding FRAMEWORK_SEARCH_PATH usage: it's a bit more tricky than I was thinking initially. What if user have specified multiple locations that contain a framework we're looking for? Which directory should we pick? I don't see for now how to make it explicit for user in which order we're going to traverse directories. What if one of the directories contains framework copy (different build/version) that we actually don't want to copy?
    However, relying on the manually specified script input file works fine, because it takes precednse over ./Carthage/Build/<platform>/.... Any suggestions on this ambiguity?

FRAMEWORK_SEARCH_PATH is an array of directories in priority order. If two directories contain the same framework, the directory listed first wins. This ensures that the bundled frameworks match those used by Xcode while compiling/linking (which also honors the priority order).

Using FRAMEWORK_SEARCH_PATH might simplify the algorithm because you don't need special cases for BUILT_PRODUCTS_DIR or Carthage/Build/<platform>. Those are both in FRAMEWORK_SEARCH_PATH in the correct order.

Also, can you provide me some sample project with development dependency you're talking about please? I don't seem to understand how it works (never worked like that before).

Yeah, I can put something together.

@dimazen
Copy link
Contributor Author

dimazen commented Mar 16, 2019

@bwhiteley ok, I got what you mean about FRAMEWORK_SEARCH_PATH. And I'm afraid it won't work. Here is why: right know I'm traversing linked frameworks from the main executable and filtering out all of them that are out of the Carthage build directory. This acts as a constrain to prevent copying frameworks that are managed by user / other package manager. In case of FRAMEWORK_SEARCH_PATH there are no more such constraints which mean that we'll end up copying everything that was linked and located in the specified directories. Hence we may copy frameworks from the Embed Frameworks phase and other frameworks that are not handled by the Carthage at all. Therefore we can start even interfering with CocoaPods :)

@bwhiteley
Copy link
Contributor

@bwhiteley ok, I got what you mean about FRAMEWORK_SEARCH_PATH. And I'm afraid it won't work. Here is why: right know I'm traversing linked frameworks from the main executable and filtering out all of them that are out of the Carthage build directory. This acts as a constrain to prevent copying frameworks that are managed by user / other package manager. In case of FRAMEWORK_SEARCH_PATH there are no more such constraints which mean that we'll end up copying everything that was linked and located in the specified directories. Hence we may copy frameworks from the Embed Frameworks phase and other frameworks that are not handled by the Carthage at all. Therefore we can start even interfering with CocoaPods :)

The timestamp/up-to-date check takes care of this. The only constraint is that the carthage copy-frameworks build phase come after the Embed Frameworks build phase and any cocoapods build phase. Some of our app projects have "internal" framework targets with the frameworks listed in Embedded Binaries. These frameworks are skipped because nothing needs to be done.

@typfel
Copy link

typfel commented Jul 12, 2019

@blender and for Alamofire it works or what was your question?

@bwhiteley
Copy link
Contributor

I don't think this is related to scheme names, but maybe target names. I think the issue here is that APFS is case-insensitive, case-preserving by default, while this new framework resolver (not to be confused with the dependency resolver) in Carthage that maps otool output to frameworks found on the filesystem is case-sensitive. in the wireapp example, It looks like the app is linked against Ziphy. The wireapp build would likely fail on a Mac where APFS had been set up to be case-sensitive (not common).

@bwhiteley
Copy link
Contributor

From the Wire-iOS project.pbxproj:

		5EF4A6E720F497E300A9EF22 /* Ziphy.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Ziphy.framework; path = Carthage/Build/iOS/Ziphy.framework; sourceTree = "<group>"; };

Tests/CarthageKitTests/InputFilesInferrerSpec.swift Outdated Show resolved Hide resolved
Tests/CarthageKitTests/InputFilesInferrerSpec.swift Outdated Show resolved Hide resolved
Tests/CarthageKitTests/InputFilesInferrerSpec.swift Outdated Show resolved Hide resolved
Tests/CarthageKitTests/InputFilesInferrerSpec.swift Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@tmspzz
Copy link
Member

tmspzz commented Jul 12, 2019

@dimazen last effort. Please fix the typos and then let's merge it.

@dimazen
Copy link
Contributor Author

dimazen commented Jul 12, 2019

@blender Should we also fix case-sensitive lookup to handle issue described by @typfel?

@tmspzz
Copy link
Member

tmspzz commented Jul 12, 2019

I'm a little confused.

Correct me if I'm wrong:

Wire's executable was probably linked against Ziphy.framework (and that's exactly what the product name in wire-ios-zipy is. I checked and built, this is the output: /Users/Tommaso/Library/Developer/Xcode/DerivedData/Ziphy-fehvdjnzdgvnntcgvxedbmecnqow/Build/Products/Debug-iphonesimulator/Ziphy.framework/Ziphy ) but for some reason @typfel has on disk "ziphy.framework".

So if @typfel had a case sensitive file system, it should not have worked.
If he had a case-insensitive filesystem, this should have worked.

Since it didn't work, I can only make out he has a case-sensitive file system. That in combination with the name mismatch on Ziphy-ziphy, would have led even the regular copy-framework phase to fail.

Are we interfering with the file system lookup of the path on disk somewhere? I didn't find any evidence. The framework map comes directly form otool, which outputs a path thus preserving whatever file system preference there was at the time of linking and leaving the re-interpretation of the path to be handled by the file system where dyldwill be executed.

If I'm right, I don't think there should be any fix.

@jdhealy jdhealy self-requested a review July 12, 2019 19:46
@typfel
Copy link

typfel commented Jul 13, 2019

Wire's executable was probably linked against Ziphy.framework (and that's exactly what the product name in wire-ios-zipy is. I checked and built, this is the output: /Users/Tommaso/Library/Developer/Xcode/DerivedData/Ziphy-fehvdjnzdgvnntcgvxedbmecnqow/Build/Products/Debug-iphonesimulator/Ziphy.framework/Ziphy ) but for some reason @typfel has on disk "ziphy.framework".

This is probably due to some CI script which incorrectly upload the file based on the project name.

So if @typfel had a case sensitive file system, it should not have worked.
If he had a case-insensitive filesystem, this should have worked.

My file system is the default case insensitive, APFS (Encrypted)

Since it didn't work, I can only make out he has a case-sensitive file system. That in combination with the name mismatch on Ziphy-ziphy, would have led even the regular copy-framework phase to fail.

This worked because in the regular copy-framework phase the output file was listed as Ziphy.framework.

One last observation:

When running copy-frameworks --auto --verbose it doesn't copy the ziphy.framework unless I rename to the correct name Ziphy.framework. I also tried re-adding the framework as ziphy.framework to the project but the copy-frameworks still didn't pick it up.

But maybe this behaviour is what we want since the iOS file system is case sensitive?

@tmspzz
Copy link
Member

tmspzz commented Jul 13, 2019

@typfel was your error a compile time or a "failed to load library" from dyld?

@typfel
Copy link

typfel commented Jul 13, 2019

@typfel was your error a compile time or a "failed to load library" from dyld?

"failed to load library" since it's not copied to the framework directory.

@tmspzz
Copy link
Member

tmspzz commented Jul 13, 2019

could you please otool -L Wire's executable and paste the output here?

@typfel
Copy link

typfel commented Jul 13, 2019

$ otool -L Wire 
Wire:
	@rpath/Ziphy.framework/Ziphy (compatibility version 13.0.0, current version 1.0.0)
	@rpath/WireDataModel.framework/WireDataModel (compatibility version 185.0.0, current version 185.0.0)
	@rpath/WireCanvas.framework/WireCanvas (compatibility version 10.0.0, current version 10.0.0)
	/System/Library/Frameworks/LocalAuthentication.framework/LocalAuthentication (compatibility version 1.0.0, current version 425.250.11)
	@rpath/FLAnimatedImage.framework/FLAnimatedImage (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/Contacts.framework/Contacts (compatibility version 0.0.0, current version 0.0.0)
	@rpath/Down.framework/Down (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/Intents.framework/Intents (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/CallKit.framework/CallKit (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/Photos.framework/Photos (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/AVKit.framework/AVKit (compatibility version 1.0.0, current version 1.0.0)
	@rpath/WireRequestStrategy.framework/WireRequestStrategy (compatibility version 163.0.0, current version 163.0.0)
	@rpath/DifferenceKit.framework/DifferenceKit (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/VideoToolbox.framework/VideoToolbox (compatibility version 1.0.0, current version 1.0.0)
	@rpath/libcmark.framework/libcmark (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/UIKit.framework/UIKit (compatibility version 1.0.0, current version 61000.0.0)
	@rpath/WireShareEngine.framework/WireShareEngine (compatibility version 151.0.0, current version 151.0.0)
	@rpath/WireCommonComponents.framework/WireCommonComponents (compatibility version 0.0.0, current version 0.0.0)
	@rpath/HockeySDK.framework/HockeySDK (compatibility version 1.0.0, current version 1.0.0)
	@rpath/FormatterKit.framework/FormatterKit (compatibility version 1.0.0, current version 1.0.0)
	@rpath/avs.framework/avs (compatibility version 0.0.0, current version 0.0.0)
	@rpath/HTMLString.framework/HTMLString (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/Foundation.framework/Foundation (compatibility version 300.0.0, current version 1570.15.0)
	/usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1252.250.1)
	/System/Library/Frameworks/AVFoundation.framework/AVFoundation (compatibility version 1.0.0, current version 2.0.0)
	/System/Library/Frameworks/CoreData.framework/CoreData (compatibility version 1.0.0, current version 866.5.0)
	/System/Library/Frameworks/CoreFoundation.framework/CoreFoundation (compatibility version 150.0.0, current version 1570.15.0)
	/System/Library/Frameworks/CoreGraphics.framework/CoreGraphics (compatibility version 64.0.0, current version 1251.12.0)
	/System/Library/Frameworks/CoreImage.framework/CoreImage (compatibility version 1.0.0, current version 5.0.0)
	/System/Library/Frameworks/CoreLocation.framework/CoreLocation (compatibility version 1.0.0, current version 2245.12.30)
	/System/Library/Frameworks/CoreMedia.framework/CoreMedia (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/CoreMotion.framework/CoreMotion (compatibility version 1.0.0, current version 2245.12.30)
	/System/Library/Frameworks/CoreTelephony.framework/CoreTelephony (compatibility version 1.0.0, current version 0.0.0)
	/System/Library/Frameworks/CoreText.framework/CoreText (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/ImageIO.framework/ImageIO (compatibility version 1.0.0, current version 0.0.0)
	/System/Library/Frameworks/MapKit.framework/MapKit (compatibility version 1.0.0, current version 14.0.0)
	/System/Library/Frameworks/MediaPlayer.framework/MediaPlayer (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/MessageUI.framework/MessageUI (compatibility version 1.0.0, current version 3445.104.7)
	/System/Library/Frameworks/MobileCoreServices.framework/MobileCoreServices (compatibility version 1.0.0, current version 943.1.0)
	/System/Library/Frameworks/PassKit.framework/PassKit (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/QuartzCore.framework/QuartzCore (compatibility version 1.2.0, current version 1.11.0)
	/System/Library/Frameworks/SafariServices.framework/SafariServices (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/UserNotifications.framework/UserNotifications (compatibility version 1.0.0, current version 1.0.0)
	@rpath/Cartography.framework/Cartography (compatibility version 1.0.0, current version 1.0.0)
	@rpath/WireCryptobox.framework/WireCryptobox (compatibility version 19.0.0, current version 19.0.0)
	@rpath/WireLinkPreview.framework/WireLinkPreview (compatibility version 23.0.0, current version 23.0.0)
	@rpath/WireProtos.framework/WireProtos (compatibility version 20.0.0, current version 20.2.0)
	@rpath/WireSyncEngine.framework/WireSyncEngine (compatibility version 267.0.0, current version 267.0.0)
	@rpath/WireSystem.framework/WireSystem (compatibility version 28.0.0, current version 28.0.2)
	@rpath/WireTransport.framework/WireTransport (compatibility version 60.0.0, current version 60.0.0)
	@rpath/WireUtilities.framework/WireUtilities (compatibility version 33.0.0, current version 33.0.0)
	@rpath/ZipArchive.framework/ZipArchive (compatibility version 1.0.0, current version 1.0.0)
	@rpath/libswiftAVFoundation.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftAssetsLibrary.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftCallKit.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftContacts.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftCore.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftCoreAudio.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftCoreData.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftCoreFoundation.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftCoreGraphics.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftCoreImage.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftCoreLocation.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftCoreMedia.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftDarwin.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftDispatch.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftFoundation.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftIntents.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftMapKit.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftMediaPlayer.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftMetal.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftObjectiveC.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftPhotos.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftQuartzCore.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftSwiftOnoneSupport.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftUIKit.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftos.dylib (compatibility version 1.0.0, current version 1001.0.82)
	@rpath/libswiftsimd.dylib (compatibility version 1.0.0, current version 1001.0.82)

@bwhiteley
Copy link
Contributor

bwhiteley commented Jul 13, 2019

@typfel was your error a compile time or a "failed to load library" from dyld?

"failed to load library" since it's not copied to the framework directory.

OK, this is a problem. The current implementation in this PR silently ignores missing frameworks. I verified by renaming a framework in Carthage/Build/iOS by changing its case. The build succeeded but the framework was not bundled. carthage copy-frameworks --auto --verbose exited with 0 and shows no complaints about the missing framework. This moves what should be a build-time error to run-time.

IMO this needs to be fixed so that a missing library causes carthage copy-frameworks --auto to exit with a non-zero exit status and print what went wrong (with or without --verbose).

@bwhiteley
Copy link
Contributor

@typfel I'm curious... in the situation where your app links against Ziphy but ziphy.framework is what is bundled, does the app run on a device? I would expect it to run in the iOS simulator but fail to launch on device because APFS is case-insensitive on macOS (by default) but is case-sensitive on iOS.

@dimazen
Copy link
Contributor Author

dimazen commented Jul 13, 2019

@bwhiteley

OK, this is a problem. The current implementation in this PR silently ignores missing frameworks. I verified by renaming a framework in Carthage/Build/iOS by changing its case. The build succeeded but the framework was not bundled. carthage copy-frameworks --auto --verbose exited with 0 and shows no complaints about the missing framework. This moves what should be a build-time error to run-time.

Let me explain current implementation and it's limitations in the give context:
it recursively finds all dependencies via otool and based on the dependencies list in the default mode (without --use-framework-search-paths) performs lookup of the found dependencies in the Carthage/Build/<platform>/ directory. In case some dependency is missing - we're skipping it because there is zero knowledge about whether this is a Carthage's dependency or a regular third-party dependency. Raising an errors will cause false-positive errors about missing dependencies that are managed by "Embed Frameworks" phase or by another dependency manager.

Workaround is to ask user to place carthage copy-frameworks Build Phase in the end of the list, but this is not reliable.

@dimazen
Copy link
Contributor Author

dimazen commented Jul 13, 2019

Just to add: in order to verify that all of the dependencies are in place I'm running a carthage-verify script. Would be good to make it a built-in tool. Anyway, verification is needed.

@bwhiteley
Copy link
Contributor

Fair enough. In that case I vote to merge.

@tmspzz
Copy link
Member

tmspzz commented Jul 15, 2019

@rpath/Ziphy.framework/Ziphy (compatibility version 13.0.0, current version 1.0.0)

I think we can all agree that the actual casing on the file system should be Ziphy.framework and this PR is good to merge as is.

@dimazen @typfel @bwhiteley @jdhealy

@dimazen
Copy link
Contributor Author

dimazen commented Jul 28, 2019

Any update on this?

@tmspzz
Copy link
Member

tmspzz commented Jul 28, 2019

@dimazen yep @jdhealy wanted to review

@stale

This comment has been minimized.

@stale stale bot added the stale label Aug 27, 2019
@stale stale bot removed the stale label Aug 27, 2019
@dimazen
Copy link
Contributor Author

dimazen commented Sep 9, 2019

Any updates about review? :|

@dimazen
Copy link
Contributor Author

dimazen commented Jun 28, 2020

Sooo, is there anything else left to add to merge this PR finally? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automate copy-frameworks command?
6 participants