-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Add a job for ObjC bazel tests #29599
Conversation
Adhoc test run is here: The duration of the job is only 18min, and further speedup is possible by enabling remote caching for the ObjC stuff (but I'd only do it as a followup) |
set -ex | ||
|
||
# avoid slow finalization after the script has exited. | ||
source $(dirname $0)/../../../tools/internal_ci/helper_scripts/move_src_tree_and_respawn_itself_rc |
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.
should we time these two scripts to get a measurement of how many #sec are spent in the preparation phase ? The prepare_build_macos_rc script currently triggers a number of cocoapod steps which may take up time on a new CI node.
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 looked at the scripts and with the current setup, they should be almost instantaneous.
I think the slow steps you're referring to are protected with PREPARE_BUILD_INSTALL_DEPS_OBJC=true,
but that's not being set in this job (looks like those steps are unnecessary for bazel) and those steps are being skipped.
if [ "${PREPARE_BUILD_INSTALL_DEPS_OBJC}" == "true" ] |
//src/objective-c/tests:UnitTests | ||
) | ||
|
||
# === BEGIN SECTION: run interop_server on the background ==== |
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.
NIT: couple of optimization we can do here e.g. only build & run interop server if the test requires it (e.g. no need for unit test , or remote test etc.). Will follow up on this part
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.
sg, let's look into this as a followup. My main goal here was to simply get SOME well configured bazel build up and running.
|
||
# Location of the continuous shell script in repository. | ||
build_file: "grpc/tools/internal_ci/macos/grpc_objc_bazel_test.sh" | ||
timeout_mins: 90 |
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.
might be good to reduce this down to e.g. 20 ~ 30min so that it fails fast in a bazel test
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.
we've had problems with timeouts for jobs being set too low in various scenarios (e.g. there's some wait to get a worker provisioned if there a queue etc), so I tend to set timeout_mins with some extra buffer to avoid problems.
Value of 90 is consistent with what we do for other jobs (some of which are also much faster then 90mins), so let's leave as is.
The question of how to set timeout_mins seems orthogonal to what's being setup in this PR and it also requires more though (and in my experience spending too much time trying to figure out the "right" value to use wasn't very productive in the past).
Thanks Jan. Yep we noticed this issue with Mac OS X Monterey and bazel 4.2.x. Do we know when we can upgrade to bazel 5.x in gRPC ? Otherwise, we would need to start investigating a workaround since this will be blockage for us to to migrate to bazel iOS test. |
I don't think we should migrate to bazel 5 just because of this. If there are other good reasons to migrate, we can consider doing that, but note that our support policy says that we should support "the two most recent LTS versions of Bazel" (currently that is 4.x and 5.x) |
1efed2a
to
2edf474
Compare
bazel C/C++ failure is an unrelated flake (since this PR only adds a new script for objC tests and doesn't change the existing build at all)l |
* grpc_objc_bazel.sh test script * add a test job for obj bazel tests
Progress on grpc/grpc-ios#74 and related issues.
Create a test job that runs bazel test for targets under
//src/objective-c/
that currently seem to be buildable.Future work (ideally not done by me, others can pick it up from here)
grpc/tools/run_tests/run_tests.py
Lines 984 to 1122 in 1d94aa9
- currently staring the interop server locally is a pain and requires a lot of boilerplate
- currently running locally is a pain with bazel 4.2.1 since it has a bug in python3 support (bazel 5.1.1 works just fine) and installing python2 locally is a pain recent version of OS X (which is what our laptops have). At the same time we don't want to update the bazel version throughout our repository just because of this. For context on the bug (see Error : env: python: No such file or directory google-ai-edge/mediapipe#3184 and bazel fix bazelbuild/bazel@2945ef5)