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

Add support for Swift Package Manager v. 5.3 #333

Closed
wants to merge 12 commits into from

Conversation

mattpolzin
Copy link

Add support for Swift Package manager by adding a package manifest and creating a few symlinks where the current directory structure of the project was a bit inconvenient or even incompatible with representation in the package manifest.

This requires version 5.3 of the package manager in order to fully support this project including the bundled images.

Closes #332.

try as c setting in addition to public header path.

switch to path-relative header search paths.

not seeing headers I expect to see still.

re-add public header path

use relative path for public headers directive
try removing include directory workaround and switching back to public header directive but pointing at root of target.
try being exacting about where to locate the protocol headers from the umbrella header.

switch back to original core header file.
@SirWellington
Copy link

Hi @mr-fixit ,
Any feedback for this PR, or can this be merged?

@mr-fixit
Copy link
Contributor

I'll take a look this week! Perhaps @mattpolzin could resolve the merge conflicts?

@mattpolzin
Copy link
Author

Should be good to go.

@mattpolzin
Copy link
Author

Worth noting for those less familiar with Swift Package Manger: Although you can in general build Swift packages from the command line with swift build, in this case you need UIKit available so the easiest way to build this package is by opening the folder containing the project up in Xcode (not an Xcode project, the folder itself) which will load the Swift Package and building from Xcode with an iOS device target will succeed.

@mr-fixit mr-fixit self-requested a review December 22, 2020 20:55
@@ -0,0 +1 @@
../NYTPhotoViewer.bundle/NYTPhotoViewerCloseButtonX.png
Copy link
Contributor

@mr-fixit mr-fixit Dec 23, 2020

Choose a reason for hiding this comment

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

I see these PNGs are duplicated, in the new .bundle and in the new location (the Resources folder). is that necessary?

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately, yes. One would think the resources could just be copied from the .bundle (since it is just a folder) but without the Resources folder SwiftPM will not bother to create a bundle (or at least it skips creating the SWIFTPM_MODULE_BUNDLE macro that is used here to access the SwiftPM bundle).

I did give this a shot because it made plenty of sense to me: I removed the Resources folder and its symlinked files and changed the respective copy lines of the package manifest but then I got a build error in Xcode that SWIFTPM_MODULE_BUNDLE was not defined. Reverting my change allowed me to build successfully again.

There are a few places where the symlinks in this PR could be removed up by making changes to the project structure as a whole to align more closely with what SwiftPM expects out-of-box (the Resources folder being one of those) but my approach here was to leave as much alone as possible; adding symlinks meant I did not need to change the project at large.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, I want to call out explicitly something I've said but not made really front and center so far; although GitHub makes it look like I copied a bunch of files, they are all actually symlinks. I'm still not over-the-moon about them, but at least this has not created a situation where these files need to be modified/deleted/etc. in multiple places.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been looking into this, hoping to find a way to avoid adding the complication of maintaining symlinks to the resources in the .bundle. Your commit history tells me you traveled a similar road vis-a-vis the headers. I was hoping something like this would work:

            resources: [
                .copy("NYTPhotoViewer.bundle/NYTPhotoViewerCloseButtonX.png")

but found this bit of code causes NYTPhotoViewer.bundle to be added to the sources, not the png.

I'm still wrapping my head around the internals of SPM, but I'm thinking the final answer will be to reorganize the resources and includes of NYTPhotoViewer so that SPM can be supported without symlinks.

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense. If it were mine to maintain, I would prefer the project reorganization over the symlinks as well.

Let me know if there’s anything else I can do to help out. I’m no expert, but I do have a bit of history with SPM at this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mr-fixit
Copy link
Contributor

mr-fixit commented Jan 7, 2021

@mattpolzin, I've opened up #338 which is your work + rearranging file hierarchy to avoid the need for symlinks. Could I ask you to take a look?

@mattpolzin
Copy link
Author

Closing in favor of #338

@mattpolzin mattpolzin closed this Jan 13, 2021
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.

Swift Package Manager support (SPM)
3 participants