search

bazelbuild / bazel-skylib

Common useful functions and rules for Bazel
https://bazel.build/
Apache License 2.0
387 stars 178 forks source link

Build kite says failing because of lint errors #247

Open brandjon opened 4 years ago

brandjon commented 4 years ago

Example failures

These appear to be mostly from analysis-time test suites, which are implemented as macros but do not (and should not) have a name parameter. We should have a way to disable this lint for these files, but it's also a more general problem for anyone using buildifier with this analysis testing paradigm.

aiuto commented 4 years ago

So this is a least 3 distinct bugs. And yet it is not obvious if any are syklib's problem

brandjon commented 4 years ago

These are actually macros, not rules. The starlark test suites are really just macros that instantiate test cases (rules).

aiuto commented 4 years ago

RIght. That's why I said different requirements for rules vs. macros. We can't have rule usage without names. We should be able to have macros without name.

brandjon commented 4 years ago

I think the lint intends to prevent you from creating macros without a name argument, because it's usually considered bad form. Certainly that's something we're trying to discourage in google3. The problem is that this isn't really a "macro" in the usual sense, but is still subject to the same requirement.

One option aside from opting out of the lint is to modify our recommended testing practices to allow for a name argument passed to the test suite.

aiuto commented 4 years ago

I don't really want to debate what buildifier should do here. That might be fun, but it's the wrong forum. For this, what shall we do? Force submit the CL now, or add features to buildifier or CI to make it easier to submit this without a force?

On Fri, May 8, 2020 at 11:15 PM Jon Brandvein notifications@github.com wrote:

I think the lint intends to prevent you from creating macros without a name argument, because it's usually considered bad form. Certainly that's something we're trying to discourage in google3. The problem is that this isn't really a "macro" in the usual sense, but is still subject to the same requirement.

One option aside from opting out of the lint is to modify our recommended testing practices to allow for a name argument passed to the test suite.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/bazel-skylib/issues/247#issuecomment-626096579, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXHHHAUPYKJ7CEAW2M2J4LRQTDD7ANCNFSM4M4HG37A .

brandjon commented 4 years ago

Not sure which CL you mean, I noticed this issue independently of any PR. I'm fine with force-submitting past any lint-only blockers, though if that becomes the habit we should remove the lint from the presubmit.

brandjon commented 4 years ago

This is already tracked upstream by bazelbuild/buildtools#821.

aiuto commented 4 years ago

SGTM. I keep using PR and CL interchangeably.

On Sat, May 9, 2020 at 3:17 PM Jon Brandvein notifications@github.com wrote:

This is already tracked upstream by bazelbuild/buildtools#821 https://github.com/bazelbuild/buildtools/issues/821.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/bazel-skylib/issues/247#issuecomment-626223147, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXHHHBMDBH2KI3PDYHRJYTRQWT47ANCNFSM4M4HG37A .