Add buildifier_test target

[k1nkreet]

#951: Create buildifier_test target target instead of running build run //:buildifer from RunTests.hs

Depends on #1596

[aherrmann]

I think this also checked files under examples and tutorial which are not covered by the new buidifier_test with //:all_files. Any way to keep those checked as well?

[k1nkreet]

I would suggest to add them as a local_repositories providing same all_files targets and use them as srcs of buildifier_test

[aherrmann]

👍 Sounds like a good approach.

[k1nkreet]

Done

[aherrmann]

I see that CI is failing on Windows with

ERROR(tools/test/windows/tw.cc:1288) ERROR: src/main/native/windows/process.cc(202): CreateProcessW("C:\users\runneradmin\_bazel_runneradmin\minshlu6\execroot\rules_haskell\bazel-out\x64_windows-fastbuild\bin\tests\buildifier_test.bash"): %1 is not a valid Win32 application.

This happens when a Bazel rule emits a script like a bash script as its executable. This works on Unix, but not on Windows where such files are not valid executables. I suspect this may be the issue here.

One way to fix this is to split the rule into two parts, one that fills in the script template and a separate native.sh_binary. We do this e.g. in cc_wrapper.

That said, I don't think we need to block this PR on that. Only checking formatting on Unix is sufficient and we can skip this test on Windows.

[k1nkreet]

I've missed buildtools readme mentions explicitly Bazel target is not supported on Windows. This is actually true for run target used in RunTests.hs as well :)

[aherrmann]

I've missed buildtools readme mentions explicitly Bazel target is not supported on Windows. This is actually true for run target used in RunTests.hs as well :)

Ah, good to know. Thanks for clarifying!

[k1nkreet]

I've tagged this test as "dont_test_on_windows" so it should not be failing now.

[dpulls]

🎉 All dependencies have been resolved !

[k1nkreet]

@aherrmann Is there anything else should be done here?

[aherrmann]

Sorry for the delay.

One small comment. Otherwise, this looks great, thank you!

This may need a manual rebase. It looks like some changes from the prerequisite PR are still in here. I think dpulls only rebases if the PR's target branch is the dependency's branch.

[aherrmann]

Suggested change

	tags = ["dont_test_on_windows"],
	mode = ["diff"],
	tags = ["dont_test_on_windows"],

I've tested this locally on a file with broken formatting and without this mode attribute I'm getting the error

buildifier: open ./external/examples-arm/BUILD.bazel: read-only file system

It looks like the default mode is fix and in test-mode these files are read-only.

The fix mode seems to only work with bazel run instead of bazel test.

[k1nkreet]

You're right, I was deceived by buildtools source code which is probably ahead :) https://github.com/bazelbuild/buildtools/blob/f575192a9d0644850302a9ac33e0129979f37e4a/buildifier/internal/factory.bzl#L49.

[aherrmann]

Rebase on master should fix the CI error. #1606 is merged.