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

Create "all_files" filegroup contains all rules_haskell sources #1596

Merged
merged 2 commits into from
Oct 5, 2021

Conversation

k1nkreet
Copy link
Contributor

#951: To be able to use buildifier_test target and integration test targets like go_bazel_build from https://github.com/bazelbuild/rules_go, this PR adds testonly filegroup contains all source files.

BUILD.bazel Outdated
"//nixpkgs:all_files",
"//protobuf:all_files",
"//rule_info:all_files",
"//tests:all_files",
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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

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.

Copy link
Contributor Author

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 :)

Copy link
Contributor Author

@k1nkreet k1nkreet Oct 4, 2021

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
Copy link
Member

@aherrmann aherrmann left a 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!

@aherrmann aherrmann added the merge-queue merge on green CI label Oct 5, 2021
@mergify mergify bot merged commit 5b7375b into master Oct 5, 2021
@mergify mergify bot deleted the ip/add_all_files_filegroup branch October 5, 2021 10:10
@mergify mergify bot removed the merge-queue merge on green CI label Oct 5, 2021
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.

2 participants