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

ref(ci): use sentry-xcodebuild.sh to wrap more xcodebuild usages #4818

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

armcknight
Copy link
Member

We can simplify some places where there's a long complicated xcodebuild invocation, and instead place that in the existing wrapper script we have for it, where it can benefit from some other tricks we've learned over time.

There's still a few clunky invocations of the wrapper script itself that should improve after #4808

I stared at the one in the fastfile for a while but i'm not immediately sure what to do with it. I kinda feel like that step just shouldn't be done with fastlane, but i'll save that for another PR.

#skip-changelog

@armcknight armcknight changed the title ref(ci): rename xcode-test.sh to sentry-xcodebuild.sh and use it to wrap more xcodebuild usages ref(ci): use sentry-xcodebuild.sh to wrap more xcodebuild usages Feb 8, 2025
@armcknight armcknight force-pushed the armcknight/ci/rename-xcode-test branch 4 times, most recently from 5c3ce20 to be6ae95 Compare February 8, 2025 07:28
@armcknight armcknight force-pushed the armcknight/ci/wrap-xcodebuild branch from a0f8f73 to bd51ce0 Compare February 8, 2025 07:30
Copy link

codecov bot commented Feb 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.405%. Comparing base (be6ae95) to head (a27134e).

Additional details and impacted files

Impacted file tree graph

@@                          Coverage Diff                          @@
##           armcknight/ci/rename-xcode-test     #4818       +/-   ##
=====================================================================
+ Coverage                           91.399%   91.405%   +0.005%     
=====================================================================
  Files                                  627       627               
  Lines                                74569     74569               
  Branches                             26802     26804        +2     
=====================================================================
+ Hits                                 68156     68160        +4     
+ Misses                                6314      6310        -4     
  Partials                                99        99               

see 4 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be6ae95...a27134e. Read the comment docs.

;;
esac

if [ "$CI" == true ]; then
if [ "${CI-false}" == true ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

l: why the fallback to false? isn't "" == true also falsy?

Copy link
Member Author

Choose a reason for hiding this comment

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

the script fails here if you run it locally if your machine doesn't define the CI environment variable at all

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it fail when a variable is not set? shouldn't it just evaluate to an empty string?

Copy link
Member Author

Choose a reason for hiding this comment

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

hm, after trying another minimal example it does. i was getting an unbound variable error at some point. i'll fix this.

RUBY_ENV_ARGS=""
else
RUBY_ENV_ARGS="rbenv exec bundle exec"
fi

if [ $RUN_BUILD == true ]; then
set -o pipefail && env NSUnbufferedIO=YES xcodebuild \
set -o pipefail && env NSUnbufferedIO=YES CODE_SIGN_IDENTITY=\"\" CODE_SIGNING_REQUIRED=NO xcodebuild \
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if we can add these env values here, as we are also building signed versions of the sample apps.

| tee thread-sanitizer.log \
| xcpretty -t

testStatus=$?
Copy link
Contributor

Choose a reason for hiding this comment

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

m: Correct me if I am wrong, but I think this will not be executed because the entire script will exit if the xcodebuild command fails

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@philipphofmann it looks like you introduced the test-with-thread-sanitizer.sh. Any insights here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could it be that it used to work, but then the addition of && set -o pipefail at the beginning of the pipeline would cause it to no longer propagate the $? the same way? we also already had that at the top of the script, but i think that wouldn't have applied to the pipeline, which is why it was added there?

Copy link
Member

Choose a reason for hiding this comment

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

I can't recall any specific reasons why I added it like that. If it's broken, feel free to fix it.

@armcknight armcknight force-pushed the armcknight/ci/rename-xcode-test branch from be6ae95 to dc54c51 Compare February 13, 2025 19:30
@armcknight armcknight force-pushed the armcknight/ci/wrap-xcodebuild branch from a27134e to e36230a Compare February 13, 2025 20:00
Base automatically changed from armcknight/ci/rename-xcode-test to armcknight/ci/brewfiles February 13, 2025 21:03
Base automatically changed from armcknight/ci/brewfiles to main February 13, 2025 21:30
@armcknight
Copy link
Member Author

honestly every time i make an edit to sentry-xcodebuild.sh at this point, it feels wrong to continue with it as a bash script. rewriting it in swift would be a huge improvement.

@armcknight armcknight marked this pull request as draft February 14, 2025 01:38
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.

3 participants