Skip to content
This repository was archived by the owner on Jun 4, 2025. It is now read-only.

Add Package.swift and remove no longer needed project files#120

Merged
crazytonyli merged 3 commits intotrunkfrom
swift-package-support
Nov 14, 2022
Merged

Add Package.swift and remove no longer needed project files#120
crazytonyli merged 3 commits intotrunkfrom
swift-package-support

Conversation

@crazytonyli
Copy link
Contributor

@crazytonyli crazytonyli commented Nov 9, 2022

Similar changes as Automattic/Gridicons-iOS#71. And I'll create a follow up PR to reorganise the folders in this repo.


  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

@crazytonyli crazytonyli requested a review from a team November 9, 2022 21:23
Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

Nice.

I think this is possibly out of scope because we have no plan to work on these libraries via SPM alone, bug just wanted to share the result of running swift test

➜ swift test
warning: 'wordpressui-ios': found 1 file(s) which are unhandled; explicitly declare them as resources or exclude from the target
    /Users/gio/Developer/a8c/WordPressUI-iOS/WordPressUI/Extensions/Gravatar/NSString+Gravatar.m

warning: 'wordpressui-ios': found 1 file(s) which are unhandled; explicitly declare them as resources or exclude from the target
    /Users/gio/Developer/a8c/WordPressUI-iOS/WordPressUI/Info.plist

Building for debugging...
In file included from /Users/gio/Developer/a8c/WordPressUI-iOS/WordPressUI/Categories/UIImage+Resize.m:6:
/Users/gio/Developer/a8c/WordPressUI-iOS/WordPressUI/Categories/UIImage+Resize.h:6:9: fatal error: 'UIKit/UIKit.h' file not found
#import <UIKit/UIKit.h>
        ^~~~~~~~~~~~~~~
1 error generated.
In file included from /Users/gio/Developer/a8c/WordPressUI-iOS/WordPressUI/Categories/UILabel+SuggestSize.m:1:
/Users/gio/Developer/a8c/WordPressUI-iOS/WordPressUI/Categories/UILabel+SuggestSize.h:1:9: fatal error: 'UIKit/UIKit.h' file not found
#import <UIKit/UIKit.h>
        ^~~~~~~~~~~~~~~
1 error generated.
In file included from /Users/gio/Developer/a8c/WordPressUI-iOS/WordPressUI/Categories/UIImage+Util.m:1:
/Users/gio/Developer/a8c/WordPressUI-iOS/WordPressUI/Categories/UIImage+Util.h:1:9: fatal error: 'UIKit/UIKit.h' file not found
#import <UIKit/UIKit.h>
        ^~~~~~~~~~~~~~~
1 error generated.
In file included from /Users/gio/Developer/a8c/WordPressUI-iOS/WordPressUI/Categories/UIColor+Helpers.m:1:
/Users/gio/Developer/a8c/WordPressUI-iOS/WordPressUI/Categories/UIColor+Helpers.h:1:9: fatal error: 'UIKit/UIKit.h' file not found
#import <UIKit/UIKit.h>
        ^~~~~~~~~~~~~~~
1 error generated.
[2/10] Compiling WordPressUIGravatarObjC NSString+Gravatar.m
error: fatalError

As far as I understand, the errors with UIKit are part of dealing with SPM and its agnostic nature. I'm sure there are workarounds for that, but given we plan to use these within our UIKit apps to work with, I don't think it's worth looking into them at this time.

The two warnings at the top, however, might be good to address?

common_params:
plugins: &common_plugins
- &bash_cache automattic/bash-cache#2.9.0
- &bash_cache automattic/bash-cache#2.10.0
Copy link
Contributor

Choose a reason for hiding this comment

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

New version with the SPM command. Nice!

),
.target(
name: "WordPressUIGravatarObjC",
path: "WordPressUI/Extensions/Gravatar",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mokagio The warning about NSString+Gravatar.m not included in any target is a bit strange, as it should be included in this target. But I have a follow up PR to reorganize the folder structure, that warning will go away since the targets will also change.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@crazytonyli
Copy link
Contributor Author

@mokagio I think the "swift build/test" CLI is useful for macOS packages, not that much for this iOS package. We should use xcodebuild instead (for example xcodebuild -scheme WordPressUI -destination 'name=iPhone 14,platform=iOS Simulator' test, which is pretty much what fastlane test lane do).

@crazytonyli
Copy link
Contributor Author

Not sure why there is a ongoing "buildkite/wordpress-ui-ios/build-and-test" checks, even though the CI job has finished. I'll merge this PR anyways...
Screen Shot 2022-11-14 at 10 05 17 PM

@crazytonyli
Copy link
Contributor Author

Actually, I don't think I have the permission to merge this PR, with this pending ghost CI check. Can anyone help to merge this PR? 🙇 @wordpress-mobile/apps-infrastructure

@mokagio
Copy link
Contributor

mokagio commented Nov 14, 2022

@crazytonyli

Not sure why there is a ongoing "buildkite/wordpress-ui-ios/build-and-test" checks, even though the CI job has finished.

I'm pretty sure this happened because that step was required in the branch protection settings while this PR removed it in favor of validate-swift-package.

I updated the branch protection to expect the new one and removed build-and-test. Unfortunately, GitHub doesn't retroactively update the checks requirement, so we'll still need to admin-merge.

Actually, I don't think I have the permission to merge this PR

That's odd. I have the permission to merge bypassing branch protections. 🤔

When I looked at the repo settings, I saw that the Platform 9 team has "Read" access. I added apps-infrastructure with Admin access. That should let you admin-merge.

image

Can you confirm you have access now?

@mokagio
Copy link
Contributor

mokagio commented Nov 14, 2022

I think the "swift build/test" CLI is useful for macOS packages, not that much for this iOS package.

For sure, although I'd say for "platform agnostic" packages, not necessarily macOS (I'm being nitpicky)?

I find it useful to run it just in case it shows something surprising or easily actionable. 👍

@mokagio
Copy link
Contributor

mokagio commented Nov 14, 2022

Unfortunately, GitHub doesn't retroactively update the checks requirement, so we'll still need to admin-merge.

I retract that. Maybe it was like that in the past, or maybe I was just wrong. Regardless, I see the checks have been updated:

image

@crazytonyli
Copy link
Contributor Author

@mokagio Thanks for looking into the GitHub permission and stuff!

@crazytonyli crazytonyli merged commit e1e5da8 into trunk Nov 14, 2022
@crazytonyli crazytonyli deleted the swift-package-support branch November 14, 2022 20:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants