-
Notifications
You must be signed in to change notification settings - Fork 228
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
Autobuild: Combine and simplify iOS build scripts #2521
Autobuild: Combine and simplify iOS build scripts #2521
Conversation
ios/deploy_ios.sh
Outdated
qmake -spec macx-xcode Jamulus.pro | ||
/usr/bin/xcodebuild -project Jamulus.xcodeproj -scheme Jamulus -configuration Release clean archive -archivePath "build/Jamulus.xcarchive" CODE_SIGN_IDENTITY="" CODE_SIGNING_REQUIRED=NO | ||
# Make a deploy folder | ||
mkdir deploy | ||
|
||
# Generate ipa by copying the .app file from the xcarchive file |
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.
xcarchive file
looks like a directory.
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.
I don't know anything about that part, but based on the cp
invocation, it must be a directory, so I've adjusted the comment as well (cc @ann0see).
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.
On macOS it displays as a file although you can cd into it. It’s similar to .app files. They show up as file, but are essentially folders
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.
A truely broken design...
ios/deploy_ios.sh
Outdated
cd deploy | ||
mkdir build/Payload | ||
cp -r build/Jamulus.xcarchive/Products/Applications/Jamulus.app build/Payload/ | ||
cd build || exit 1 |
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.
set -eu
should mean the || exit 1
isn't needed -- it's effectively the same thing, as far as I'm aware.
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.
Hmm, shellcheck made me do this and I assumed that cd
(as a shell built-in) was special compared to other commands which I know are checked with set -e
.
I've run a quick test and it looks you are right. I'm going to remove the || exit 1
.
This is what happened (probably): I took the script, ran shell check, noted the warning, fixed it and only later noticed that the script did not use set -eu
and added that. So, rather PEBKAC than a shellcheck bug... :)
Co-authored-by: Christian Hoffmann <[email protected]>
Related: jamulussoftware#2503 - Move all autobuild/ios/* scripts to a single .github/autobuild/ios.sh script which is called for the different stages (setup/build/get-artifacts). - Condense redundant parameter parsing into a single step - Create functions with proper names for larger steps - Inline Github artifact output definition as it's shorter that way - Make shellcheck-clean
d7cd9f0
to
f460f5c
Compare
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.
Looks very nice now.
Now that all platforms have been converted to the new scheme (.github/autobuild/*), the autobuild directory only contains the ensure_THIS_JAMULUS_PROJECT_PATH.sh script which is no longer used. Related: jamulussoftware#2521
Now that all platforms have been converted to the new scheme (.github/autobuild/*), the autobuild directory only contains the ensure_THIS_JAMULUS_PROJECT_PATH.sh script which is no longer used. Related: jamulussoftware#2521
Now that all platforms have been converted to the new scheme (.github/autobuild/*), the autobuild directory only contains the ensure_THIS_JAMULUS_PROJECT_PATH.sh script which is no longer used. Related: jamulussoftware#2521
Now that all platforms have been converted to the new scheme (.github/autobuild/*), the autobuild directory only contains the ensure_THIS_JAMULUS_PROJECT_PATH.sh script which is no longer used. Related: jamulussoftware#2521
Now that all platforms have been converted to the new scheme (.github/autobuild/*), the autobuild directory only contains the ensure_THIS_JAMULUS_PROJECT_PATH.sh script which is no longer used. Related: jamulussoftware#2521
Now that all platforms have been converted to the new scheme (.github/autobuild/*), the autobuild directory only contains the ensure_THIS_JAMULUS_PROJECT_PATH.sh script which is no longer used. Related: jamulussoftware#2521
Now that all platforms have been converted to the new scheme (.github/autobuild/*), the autobuild directory only contains the ensure_THIS_JAMULUS_PROJECT_PATH.sh script which is no longer used. Related: jamulussoftware#2521
Short description of changes
Extract the non-autobuild-related parts into an ios/deploy_ios.sh script, which can run standalone, similar to other platforms (by @ann0see)
Move all autobuild/ios/* scripts to a single .github/autobuild/ios.sh script which is called for the different stages (setup/build/get-artifacts).
Condense redundant parameter parsing into a single step
Create functions with proper names for larger steps
Inline Github artifact output definition as it's shorter that way
Make shellcheck-clean
CHANGELOG: Autobuild: Refactored and simplified iOS build scripts.
(will be condensed...)
Context: Fixes an issue?
Related: #2503
Does this change need documentation? What needs to be documented and how?
Status of this Pull Request
Ready for review.
What is missing until this pull request can be merged?
Reviews.
Checklist