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.

Sign up for GitHub

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

Jump to bottom

file_test, rule_test: now as sh_test rules #8352

Closed
wants to merge 1 commit into from
Closed

file_test, rule_test: now as sh_test rules #8352

wants to merge 1 commit into from

Conversation

laszlocsomor
Copy link
Contributor

All test rules in
@bazel_tools//tools/build_rule:test_rules.bzl are
now macros around sh_test.

This allows running them on Windows with the
Windows-native test wrapper.

Fixes #8203
Unblocks #6622

@laszlocsomor
file_test, rule_test: are now sh_test rules
c5d4b76 
All test rules in
@bazel_tools//tools/build_rule:test_rules.bzl are
now macros around sh_test.

This allows running them on Windows with the
Windows-native test wrapper.

Fixes bazelbuild#8203
Unblocks bazelbuild#6622
@laszlocsomor laszlocsomor requested a review from laurentlb May 16, 2019 10:44
@laszlocsomor
Copy link
Contributor Author

all green

laurentlb
laurentlb reviewed May 16, 2019
View reviewed changes
Copy link
Contributor

@laurentlb laurentlb left a comment

Choose a reason for hiding this comment

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

It's a lot of Bash-specific code, how does it work on Windows?

tools/build_rules/test_rules.bzl
@@ -14,83 +14,153 @@
# See the License for the specific language governing permissions and
# limitations under the License.

_INIT_BASH_RUNFILES = "\n".join([
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment? It's not obvious to see what it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should the comment say? The variable's name is descriptive and the first line of the code says what it does.

tools/build_rules/test_rules.bzl
_INIT_BASH_RUNFILES = "\n".join([
"#!/bin/bash",
"# --- begin runfiles.bash initialization ---",
"# Copy-pasted from Bazel Bash runfiles library (tools/bash/runfiles/runfiles.bash).",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why copy-pasted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bazel/tools/bash/runfiles/runfiles.bash

Lines 34 to 36 in 616481a

# The runfiles library itself defines rlocation which you would need to look
# up the library's runtime location, thus we have a chicken-and-egg problem.
# Insert the following code snippet to the top of your main script:

@laszlocsomor
Copy link
Contributor Author

It's a lot of Bash-specific code, how does it work on Windows?

Via sh_test.

Previously, the executable of these test rules was a naked .sh file.
Now they are sh_test rules which produces an .exe file.

Therefore these tests, while still depend on Bash, they don't depend on the Bash-based test wrapper, and that's what I wanted to achieve, see #8203 (comment)

laurentlb
laurentlb approved these changes May 16, 2019
View reviewed changes
Copy link
Contributor

@laurentlb laurentlb left a comment

Choose a reason for hiding this comment

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

lgtm

@laszlocsomor
Copy link
Contributor Author

Thank you for the quick review!

@bazel-io bazel-io closed this in 76583ee May 17, 2019
@laszlocsomor laszlocsomor deleted the test-rules-with-tw-2 branch May 17, 2019 08:31
@laszlocsomor
Copy link
Contributor Author

I need to roll this back, because it breaks some Google-internal tests. :(

bazel-io pushed a commit that referenced this pull request May 17, 2019
@laszlocsomor @copybara-github
Automated rollback of commit 76583ee.
28f8af7 
*** Reason for rollback ***

Breaks some of Google's own tests

*** Original change description ***

file_test, rule_test: now as sh_test rules

All test rules in
@bazel_tools//tools/build_rule:test_rules.bzl are
now macros around sh_test.

This allows running them on Windows with the
Windows-native test wrapper.

Fixes #8203
Unblocks #6622

Closes #8352.

PiperOrigin-RevId: 248682341
bazel-io pushed a commit that referenced this pull request May 17, 2019
@laszlocsomor @copybara-github
file_test, rule_test: now as sh_test rules
77b9875 
This commit rolls forward commit 76583ee,
which was rolled back by commit 28f8af7.

Differences:
- test_rules.bzl:success_target and failure_target
  no longer dump 'msg' into a file and print that.
  Instead, they print it as a heredoc.
- The same functions no longer need runfiles.bash.
  The breakage the original commit caused was that
  some Starlark rule used success_target but
  itself didn't depend on the Bash runfiles
  library, and success_target returns a
  DefaultInfo without the runfiles library in it.
  To keep existing success_target and
  failure_target calls intact, I opted to remove
  the dependency on runfiles.bash
- marked all "_impl" rules as testonly=1

Original commit message follows.

All test rules in
@bazel_tools//tools/build_rule:test_rules.bzl are
now macros around sh_test.

This allows running them on Windows with the
Windows-native test wrapper.

Fixes #8203
Unblocks #6622

Closes #8352.

PiperOrigin-RevId: 248691640
irengrig pushed a commit to irengrig/bazel that referenced this pull request Jun 18, 2019
@laszlocsomor @irengrig
file_test, rule_test: now as sh_test rules
98cdf25 
All test rules in
@bazel_tools//tools/build_rule:test_rules.bzl are
now macros around sh_test.

This allows running them on Windows with the
Windows-native test wrapper.

Fixes bazelbuild#8203
Unblocks bazelbuild#6622

Closes bazelbuild#8352.

PiperOrigin-RevId: 248680587
irengrig pushed a commit to irengrig/bazel that referenced this pull request Jun 18, 2019
@laszlocsomor @irengrig
Automated rollback of commit 76583ee.
ad39a87 
*** Reason for rollback ***

Breaks some of Google's own tests

*** Original change description ***

file_test, rule_test: now as sh_test rules

All test rules in
@bazel_tools//tools/build_rule:test_rules.bzl are
now macros around sh_test.

This allows running them on Windows with the
Windows-native test wrapper.

Fixes bazelbuild#8203
Unblocks bazelbuild#6622

Closes bazelbuild#8352.

PiperOrigin-RevId: 248682341
irengrig pushed a commit to irengrig/bazel that referenced this pull request Jun 18, 2019
@laszlocsomor @irengrig
file_test, rule_test: now as sh_test rules
218c1ca 
This commit rolls forward commit 76583ee,
which was rolled back by commit 28f8af7.

Differences:
- test_rules.bzl:success_target and failure_target
  no longer dump 'msg' into a file and print that.
  Instead, they print it as a heredoc.
- The same functions no longer need runfiles.bash.
  The breakage the original commit caused was that
  some Starlark rule used success_target but
  itself didn't depend on the Bash runfiles
  library, and success_target returns a
  DefaultInfo without the runfiles library in it.
  To keep existing success_target and
  failure_target calls intact, I opted to remove
  the dependency on runfiles.bash
- marked all "_impl" rules as testonly=1

Original commit message follows.

All test rules in
@bazel_tools//tools/build_rule:test_rules.bzl are
now macros around sh_test.

This allows running them on Windows with the
Windows-native test wrapper.

Fixes bazelbuild#8203
Unblocks bazelbuild#6622

Closes bazelbuild#8352.

PiperOrigin-RevId: 248691640
@laszlocsomor laszlocsomor mentioned this pull request Jul 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurentlb laurentlb laurentlb approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
3 participants
@laszlocsomor @laurentlb @googlebot