script should handle archives as well as regular builds #34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can you describe the changes? |
Sure! How's the new description ☝️? |
|
@sindresorhus pls consider this a friendly “ping” to let me know if you want any changes to accept this. |
sindresorhus
requested changes
Apr 7, 2020
LaunchAtLogin/copy-helper.sh
Outdated
| else | ||
| WHERE="${BUILT_PRODUCTS_DIR}" | ||
| origin_helper_path="${WHERE}/$FRAMEWORKS_FOLDER_PATH/LaunchAtLogin.framework/Resources/LaunchAtLoginHelper.app" | ||
| helper_dir="${WHERE}/$CONTENTS_FOLDER_PATH/Library/LoginItems" |
There was a problem hiding this comment.
This one is the same for both branches, so it could be outside the if-statement.
There was a problem hiding this comment.
Unfortunately, it can't be set outside the if bc it's used inside the if to set origin_helper_path (and again outside to set helper_path), but it also depends on the value of WHERE which is set differently in the if and else cases.
Perhaps you have a different setup in mind?
There was a problem hiding this comment.
Unfortunately, it can't be set outside the if bc it's used inside the if to set origin_helper_path (and again outside to set helper_path), but it also depends on the value of WHERE which is set differently in the if and else cases.
Perhaps you have a different setup in mind?
LaunchAtLogin/copy-helper.sh
Outdated
| @@ -1,7 +1,15 @@ | |||
| #!/bin/bash | |||
|
|
|||
| origin_helper_path="$BUILT_PRODUCTS_DIR/$FRAMEWORKS_FOLDER_PATH/LaunchAtLogin.framework/Resources/LaunchAtLoginHelper.app" | |||
| helper_dir="$BUILT_PRODUCTS_DIR/$CONTENTS_FOLDER_PATH/Library/LoginItems" | |||
| if [ "${DEPLOYMENT_LOCATION}" = "YES" ] ; then | |||
There was a problem hiding this comment.
Suggested change
| if [ "${DEPLOYMENT_LOCATION}" = "YES" ] ; then | |
| if [ "${DEPLOYMENT_LOCATION}" = "YES" ]; then |
There was a problem hiding this comment.
This could use a code comment to make it clear what the if check does.
LaunchAtLogin/copy-helper.sh
Outdated
| origin_helper_path="$BUILT_PRODUCTS_DIR/$FRAMEWORKS_FOLDER_PATH/LaunchAtLogin.framework/Resources/LaunchAtLoginHelper.app" | ||
| helper_dir="$BUILT_PRODUCTS_DIR/$CONTENTS_FOLDER_PATH/Library/LoginItems" | ||
| if [ "${DEPLOYMENT_LOCATION}" = "YES" ] ; then | ||
| WHERE="${DSTROOT}" |
There was a problem hiding this comment.
WHERE is too generic. Make a descriptive variable name. Should also be lowercase.
| @@ -18,5 +26,5 @@ fi | |||
|
|
|||
| if [[ $CONFIGURATION == "Release" ]]; then | |||
| rm -rf "$origin_helper_path" | |||
| rm "$(dirname "$origin_helper_path")/copy-helper.sh" | |||
| rm "$(dirname "$origin_helper_path")/copy-helper.sh" || true | |||
There was a problem hiding this comment.
Why are you silencing errors?
There was a problem hiding this comment.
If there's no copy-helper.sh file in that location, that command will return a non-zero value and - bc it's the last command - the script will return that value. When the script returns a non-zero value, it causes that build step to fail.
There was a problem hiding this comment.
If there's no copy-helper.sh file in that location, the rm command will return a non-zero value and - bc it's the last command - the script will return that value. When the script returns a non-zero value, it causes that build step to fail.
Are you sure about this? I've never had any problems using "Archive", only "Profile". |
It was causing problems for me when trying to make an Archive. |
sweetleon
commented
Apr 8, 2020
There was a problem hiding this comment.
@sindresorhus I believe the latest commit addresses all your suggestions. Please have another look.
|
@sindresorhus pls have another look at this PR. I believe everything you've asked for has either been implemented or addressed if it can't be implemented. |
|
I've tried using your fork @sweetleon and my Archiving process stopped working... |
|
Actually what stopped working is the Hardening & Signing process I have with this script: Here's the error: |
nodeful
reviewed
Apr 29, 2020
| @@ -18,5 +26,5 @@ fi | |||
|
|
|||
| if [[ $CONFIGURATION == "Release" ]]; then | |||
| rm -rf "$origin_helper_path" | |||
There was a problem hiding this comment.
I think an || true also needs to be added here. Basically what happens is that on first "Release" or "Archive" build the script deletes the files, any consecutive build thinks that the Frameworks has already been copied over and skips copying the framework again, hence the files are already missing from there...
There was a problem hiding this comment.
If that folder isn't present, there'd be a message printed to stderr but it shouldn't cause the script to fail. (Since it's not the last command in the script, it doesn't determine the script's exit code.) Are you seeing the script fail bc of this?
LOCATION="$BUILT_PRODUCTS_DIR/$CONTENTS_FOLDER_PATH/Library/LoginItems/LaunchAtLoginHelper.app" ...
Seems like you're trying to codesign the exact thing the script is designed to delete. Once it's deleted, you won't have to codesign it. So I'd maybe chance the last line of the script to first check if it exists: |
|
Idk guys I've forked the repo and just done this: |
|
I tried this out now in my app and when archiving, I'm getting errors. Xcode 11.4.1. |
Does this cause the build to fail? The changes should cause the build to ignore these errors. |
|
It did build successfully yes, but the helper app ended up at: ( Instead of: |
|
@sweetleon Was your error that prompted this PR the following? Because I just got and turned out it was because the LaunchAtLogin run script had moved to before the "Embed Framework" script in "Build Phases". Can you check if that's the case for you too? |
|
@sindresorhus that sounds familiar, but I'm no longer working on that project so I can't be sure. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #7 by (a) deleting
LaunchAtLogin.appfrom the correct location during Archive/Profiling builds, which differs from where it's during Debug/Run builds.IssueHunt Summary
Referenced issues
This pull request has been submitted to:
IssueHunt has been backed by the following sponsors. Become a sponsor