-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Fix wrong XHARNESS_CLI_COMMAND in wasm README #46294
Conversation
The variable is actually `XHARNESS_CLI_PATH`. Also added it to the AppleRunnerTemplate.sh script since it was missing there.
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
@@ -56,7 +56,15 @@ else | |||
export XHARNESS_OUT="$HELIX_WORKITEM_UPLOAD_ROOT/xharness-output" | |||
fi | |||
|
|||
dotnet xharness ios test \ | |||
if [ ! -z "$XHARNESS_CLI_PATH" ]; then |
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.
Wait.. this script is used in CI?
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.
It is for WASM where I copied this code block from 😄
Looks like Android has it too even though it's not true there...
We should look at reusing the script logic from your arcade helix sdk like we talked about a while ago.
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.
The script in Helix SDK has several different functions by now so I am not sure it is really what we want:
- accept env vars because of
launchctl
- calling
ios test
/ios run
command based on arguments - collecting logs and result XML and placing them in the Helix upload folder
- rebooting the machine / retry of the work item when install hangs
- disabling colors / enabling timestamps for logs
- (soon) signing the app using the Keychain present on Helix agents
Many of these steps are probably not desired for local flows. Recently, I even split the script into 2 to fix the retry/reboot flow. Overall I consider the script to be very Helix-infra-agnostic so it really only makes sense to use it inside Helix jobs
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.
Ok then I guess I'll instead put a notice at the top of the AppleRunnerTemplate.sh that it isn't used in CI.
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.
That's a good idea! There was quite a lot of confusion around this already.
Also afaik Android might get its own script in Helix SDK soon too. I think it's how Matt will reboot the emulators
3ece927
to
9abfbcf
Compare
Also simplify the iOS/Android scripts a bit.
9abfbcf
to
36c7509
Compare
The variable is actually
XHARNESS_CLI_PATH
. Also added it to the AppleRunnerTemplate.sh script since it was missing there.Also simplify iOS/Android scripts a bit.