-
Notifications
You must be signed in to change notification settings - Fork 81
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
Create "all_files" filegroup contains all rules_haskell sources #1596
Conversation
BUILD.bazel
Outdated
"//nixpkgs:all_files", | ||
"//protobuf:all_files", | ||
"//rule_info:all_files", | ||
"//tests:all_files", |
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.
For the buildifer use-case we'll want to include all files under tests
to make sure that they are format tested as well. But, for the go_bazel_test
use-case perhaps not. Including them means that go_bazel_test
type integration tests are re-run not only when rules_haskell's implementation changes, but also when any of its tests changes.
AFAIK rules_go does not include the tests tree in the all_files
filegroups.
A simple option might be to omit //tests:all_files
here and then add it to the buildifier test explicitly. What do you think?
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.
A related instance is //vendor
. It's needed for functionality, so will be needed for go_bazel_test
, but it would be preferable to skip it for buildifier tests.
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.
Probably it makes sense to enumerate all necessary filegroups in buildifier_test
srcs explicitly, because otherwise we'll inevitably end up with some kind of "sources for buildifer_test" filegroup. In this case name all_files
is a bit odd for filegroup dedicated for integration_tests. rules_go
for example doesn't include tests in all_files
but use it both for integration tests and buildifier which is a bit kludgy imho. Some bazelbuild projects like bazel-skylib
or rules_python
uses name distribution
for such filegroup, probably it makes sense to follow
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.
Some bazelbuild projects like
bazel-skylib
orrules_python
uses namedistribution
for such filegroup, probably it makes sense to follow
Nice find! Yes, that sounds like a good precedent to follow.
Probably it makes sense to enumerate all necessary filegroups in
buildifier_test
srcs explicitly, [...]
You mean at the top-level something like this?
buildifier_test(
srcs = [
"//haskell:all_files",
"//nixpkgs:all_files",
"//tests:all_files",
...
],
Yes, that sounds like a good solution to the problem.
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.
You mean at the top-level something like this?
Yes, that's exactly what I meant :)
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.
Some bazelbuild projects like bazel-skylib or rules_python uses name distribution for such filegroup, probably it makes sense to follow
Done
* remove tests from distribution filegroup * add //vendor files into distribution filegroup
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.
Thank you, looks good!
#951: To be able to use
buildifier_test
target and integration test targets likego_bazel_build
from https://github.com/bazelbuild/rules_go, this PR adds testonly filegroup contains all source files.