-
Notifications
You must be signed in to change notification settings - Fork 115
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 incorrect example client checks #1169
Conversation
@@ -125,7 +125,7 @@ do | |||
;; | |||
cpp) | |||
cpp_client="./bazel-client-bin/examples/${EXAMPLE}/client/client" | |||
if [[ -d "${cpp_client}" ]]; then | |||
if [[ -f "${cpp_client}" ]]; 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.
Fine for now to get things moving again, but this has a couple of confusions that it would be good to come back and fix.
- On a minor note,
cpp_client
doesn't actually indicate a C++ client, it indicates a client that's built with Bazel (example:examples/translator/client/client
is built from Go). This is mostly pedantry, up until the point where it isn't. - More significantly, this isn't checking whether there is C++ client source code available, it's checking whether there is an already-built client binary present in the directory that our build scripts happen to put Bazel output into. This means that using the
-s none
option in an environment that hasn't donebuild_example
is going to skip the tests.
For the latter, I wonder if a test for "${SCRIPTS_DIR}/../examples/${EXAMPLE}/client/BUILD"
would be more reliable (and would be more closely analogous to the rust)
case above).
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.
Thanks for your comment.
I agree with you on the cpp
naming, though arguably that issue is not new in this logic and preceded those changes.
My suggestion is to take inspiration from the rust runner and follow this up with another PR that changes that check & renames cpp
to the more accurate bazel
, alongside renaming nodejs
to npm
, aka the build system the scripts will use.
Reproducibility Index:
Reproducibility Index diff: |
Fixes the issue of incorrectly skipping clients, as caught by David: #1148 (comment)
Looking back the forgiving nature of this logic probably isn't ideal. Instead it seems that a type of "manifest" that specifies all clients that should be available for an example (and fails if they are not) is preferable.