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

tests: Reference sh test script to be run with ./ #10977

Merged
merged 2 commits into from
Apr 28, 2020
Merged

tests: Reference sh test script to be run with ./ #10977

merged 2 commits into from
Apr 28, 2020

Conversation

sunjayBhatia
Copy link
Member

@sunjayBhatia sunjayBhatia commented Apr 28, 2020

Commit Message: Enable sh tests to run on Windows by referencing scripts with ./
Additional Description:

Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

- without it, we get '<testscript>: command not found'
- will enable Windows to run sh tests once we get bazel 3.1.0 which
  contains the fix for this issue: bazelbuild/bazel#10959

Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
@@ -6,4 +6,4 @@

cd $(dirname "$0")

"$@"
"./$@"
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this won't be necessary after bazel 3.1.0. If I've got that right, we should a todo here (in our standard style with a github username) to revert this once that's in place.

Copy link
Member Author

Choose a reason for hiding this comment

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

@zuercher actually, this will be necessary after bazel 3.1.0, currently with bazel 3.0.0 and below the sh_test_wrapper.sh does not have the correct execution environment/runfiles setup/symlinks so the test file the envoy_sh_test rule sets up cannot find the test script to invoke at all (even with ./), with bazel 3.1.0 and above, the runfiles are set up correctly for the sh_test rule, but running the sh_test_wrapper.sh script as-is causes the command not found error, adding ./ fixes that for bazel 3.1.0

I can re-clarify in the issue description if needed to make that clearer

Copy link
Member Author

Choose a reason for hiding this comment

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

this is a minimal example that demonstrates the issue (linked in the bazel issue above): https://github.com/greenhouse-org/bazel-issue-repro/blob/c7ebe740e6cf12b58c7c7e156d9f5d2670329aad/sh_test_symlinks/README.md

@zuercher zuercher self-assigned this Apr 28, 2020
macOS still runs the hot restart test but effectively skips
it with an empty set of srcs that become args to the sh_test_wrapper.sh

Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>
@sunjayBhatia
Copy link
Member Author

sunjayBhatia commented Apr 28, 2020

interestingly, on Linux I just noticed . is added to the PATH and that is not the case when bash is invoked on Windows, so that might be something to look into as well

@sunjayBhatia
Copy link
Member Author

interestingly, on Linux I just noticed . is added to the PATH and that is not the case when bash is invoked on Windows, so that might be something to look into as well

this general issue could be solved by adding . to the path in the wrapper script (to ensure the execution environment is correct regardless of any PATH variables set by the shell) or the ./ solution

@sunjayBhatia
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10977 (comment) was created by @sunjayBhatia.

see: more, trace.

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.

None yet

2 participants