-
Notifications
You must be signed in to change notification settings - Fork 10
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
SPM Support #2
SPM Support #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this is very cool! Thank you for your contribution!
Sorry for not seeing this sooner, but I wasn't even watching my own repo and I only noticed there were PRs after pushing my first attempt at an SPM approach 😅 Glad I didn't get too far though because this is an excellent take!
I have a couple comments, but it's all pretty minor stuff. Let me know if you aren't able to change/discuss these and I would be happy to merge and make a couple tweaks myself.
Thanks!
public struct IconPreviews<Icon: View>: View { | ||
var icon: () -> Icon | ||
|
||
public init(icon: @autoclosure @escaping () -> Icon) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 Nice wrapper! Very helpful 👍
fi | ||
} | ||
|
||
[[ -s "$SCRIPT_OUTPUT_FILE_0" ]] || { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the Assets.xcassets as an output. I think that makes a lot of sense 👍
build-script.sh
Outdated
exit 1 | ||
} | ||
|
||
HELPER=$PWD/Icon/Icon+PreviewHelpers.swift |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️ One tweak here could be to get the location of the script itself instead of relying on the pwd. Stack Overflow tells me we could do this with:
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
build-script.sh
Outdated
GENERATOR=$PWD/IconGenerator/IconGenerator.swift | ||
MAIN=$PWD/IconGenerator/main.swift | ||
|
||
cat $SCRIPT_INPUT_FILE_0 $HELPER $GENERATOR $MAIN > $TMPFILE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️ Minor thing, but you could do the grep -v
here in one line, like:
# Concatenate all files and remove import that is most likely in the input file
cat $SCRIPT_INPUT_FILE_0 $HELPER $GENERATOR $MAIN | grep -v "import\sSwiftUIcon" > $TMPFILE
cat ${SCRIPT_INPUT_FILE_0%/*}/*.swift > $TMPFILE | ||
|
||
xcrun -sdk macosx swift $TMPFILE | ||
cd "${BUILD_ROOT}"/../../SourcePackages/checkouts/SwiftUIcon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 This is much simpler than before! But I think we might be able to simplify more.
I was also able to run the build script in the package without having to +x via chmod, so we might be able to remove that.
And if coupled with the comment below to get the PWD from within the script (and a little trick to trim the path back up before "Build"), we could make the Run Script phase a one-liner:
"${BUILD_ROOT%Build/*}SourcePackages/checkouts/SwiftUIcon/build-script.sh"
It sounds like you're a lot more fluent in bash than I am so please feel free to make changes on this. Is the best way to add you as maintainer to the fork? |
I think at this point if you just resolve the current conflicts by pulling over the Mac idiom support just merged, then we can merge this PR and I can do a follow-up with my tweaks and cut a SPM release (most likely 0.2.0). Let me know if that works for you. Thanks! |
@Amzd Just to follow-up on this, I did the merge locally and have attached a diff that you could apply to your fork to get it ready to merge. I'd be happy to then take it from there and get an SPM release out, with credit to you of course! |
I applied the diff before merging, whoops >.< Hope it still works like this? |
@Amzd Yep, all good. Thanks! I'll ping you when I'm able to make the release 👍 |
Only have to change some images adn description in the readme and update the example project.
The build phase method doesn't work with local packages; In that case you have to copy the contents of build-script.sh into your build phase.
Other than that;
You can now use this with SPM;