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

Move foo/test/BUILD content into foo/BUILD #4459

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Dec 12, 2016

This is intended to be shuffle-only changes, without any novel code:

  • Paste the foo/test/BUILD content at the end of foo/BUILD.
  • Replace DEPS shared variable with copy-pasting into uses.
  • Add test/ prefix to paths as needed.
  • Use local ":libname" paths instead of "//full/paths" where possible.
  • Buildifier reformatting.

Closes #4457.


This change is Reviewable

@liangfok
Copy link
Contributor

What is the motivation behind moving the contents of foo/test/BUILD into foo/BUILD? Isn't it better to keep a BUILD files closer to the files it is building? Also, per #4455, is it odd for a single BUILD file to cover code that spans a wide difference in terms its place in the overall dependency chain (tests are always last whereas foo may be far from last)?


Review status: 0 of 33 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

The motivation is that Bazel is slightly more convenient to work with when the library rules and test rules are in the same BUILD file.

The #4455 topology is about what deps = [] says, not about where BUILD files are located.

@liangfok
Copy link
Contributor

Cool. Thanks!

* Paste the foo/test/BUILD content at the end of foo/BUILD
* Replace DEPS shared varibale with copy-pasting into uses
* Add test/ prefix to paths as needed
* Use local ":libname" paths instead of "//full/paths" where possible
* Buildifier reformatting
@jwnimmer-tri jwnimmer-tri force-pushed the bazel-refactor_test_rules branch from 95a270e to d9d85f4 Compare December 12, 2016 16:46
@jwnimmer-tri
Copy link
Collaborator Author

@drake-jenkins-bot linux-gcc-bazel-experimental please

@rpoyner-tri
Copy link
Contributor

+@rpoyner-tri


Review status: 0 of 33 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@rpoyner-tri rpoyner-tri self-assigned this Dec 12, 2016
@rpoyner-tri
Copy link
Contributor

:lgtm:


Reviewed 33 of 33 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

+@david-german-tri do you want to weigh in on this (at a high level, at least)?


Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@david-german-tri
Copy link
Contributor

Only to gripe that #2182 was a suboptimal decision, and this illustrates why: we now have a mismatch between our test target paths in Bazel and the paths to the source code on the filesystem. It's not worth revisiting, though.

I've confirmed locally that the list of tests is unchanged. :lgtm:


Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@david-german-tri
Copy link
Contributor

I'm going to go ahead and merge this so that I can proceed with #4462.


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@david-german-tri david-german-tri merged commit 20db06d into RobotLocomotion:master Dec 12, 2016
@jwnimmer-tri jwnimmer-tri deleted the bazel-refactor_test_rules branch December 12, 2016 19:00
david-german-tri added a commit to david-german-tri/drake that referenced this pull request Dec 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update BUILD files for test-declaration consistency
4 participants