Aspects and testonly don't interact well

[endobson]

Testonly is an attribute that is applied to rules, and is enforced on aspect extended versions of the rule even if that is not appropriate.

I want to build an aspect which needs a testonly file as a dependency to build a aspect which is being added by a testonly rule. But I cannot as the aspect is being (recursively) applied to a non testonly rule, and the enforcement currently is that all aspect extensions of a non-testonly rule are non-testonly.

Example error message:

ERROR: /Users/endobson/proj/racket/yaspl2/libraries/algorithms/BUILD:46:1: in //tools:yaspl-missing-imports.bzl%yaspl_missing_imports aspect on filegroup rule //libraries/algorithms:package_binaries: non-test target '//libraries/algorithms:package_binaries' depends on testonly target '//:module_index' and doesn't have testonly attribute set

I would think that the solution is that aspect extended rules would have a seperate testonly bit and that aspects can be marked as testonly which means that anything they apply to gets the testonly bit set.

Code to reproduce: https://github.com/endobson/yaspl2/tree/aspect-testonly bazel build //:missing_imports

endobson@yggdrasil () ~/proj/racket/yaspl2 % bazel version                                                                                                                                                                                (1)
Build label: 0.16.1- (@non-git)
Build target: bazel-out/darwin-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Thu Aug 16 04:42:18 2018 (1534394538)
Build timestamp: 1534394538
Build timestamp as int: 1534394538

[aiuto]

Handing off to Configurability to take a first look and see if it is your problem.

[programmablereya]

Hmmm, so, if I'm understanding correctly, the problem is that given a testonly rule //toplevel which uses an aspect aspect which depends on a testonly rule //aspectdata, then the application of aspect to a non-testonly rule //normalrule will fail because the testonly validation is looking at the testonly bits for //normalrule (false) and //aspectdata (true). What you would like is for //toplevel's testonly bit to be used instead.

Note that the rule which launched the aspect is not associated with the aspect, so we can't just use the testonly bit of the rule which sent the aspect out.

This is a problem for visibility, too, as the access of the rule the aspect is being applied to is used instead of the access of the rule which created the aspect.

I think the Starlark team might have to weigh in on this, as it's something of an API question for defining aspects. If an aspect depends on a testonly rule, can we just say that aspect itself is testonly, and forbid using it from non-testonly rules? Should we use the aspect's package for aspect attributes' visibility? All of these would work, but what do we want this to look like for aspect authors?

[laurentlb]

Not sure who's responsible for aspects core behavior. Lukács, any opinion on this issue?

[lberki]

@dslomov it is (sorry for the late reply)

[dslomov]

Visibility: the only rule that makes sense is that visibility of aspect's package is used.

testonly: I think it would make sense to mark aspects at testonly - then only testonly rules can originate them.

[dslomov]

/cc @c-parsons

[comius]

cc @mai93