Update "Locating Runfiles with Bzlmod"

[fmeum]

Updates the proposal with:

• changes suggested in a discussion with @lberki and @Wyverald
• changes required due to canonical repository names no longer starting with @
• the observation that non-tool toolchain dependencies also have to have their repository mappings tracked

[fmeum]

@lberki @Wyverald Could you review?

[fmeum]

@cushon This proposal relies on the ability to inject a generated class containing a single static final String constant into the compile-time classpath of all Java compilation actions. Do you see any potential issues with that?

[cushon]

This proposal relies on the ability to inject a generated class containing a single static final String constant into the compile-time classpath of all Java compilation actions. Do you see any potential issues with that?

Can you expand on the rationale for making it an implicit neverlink dep of every Java compilation action, vs. an explicit dep of the targets that need it?

[fmeum]

@cushon With Bzlmod, any kind of runfiles lookup (e.g. using @bazel_tools//tools/java/runfiles) requires knowing the canonical repository name of the target performing the lookup as a reference point. Since these canonical repository names are more or less dynamic (something like rules_go~0.33.0~bzl_function~some_name), they can't be hardcoded, they have to be injected at compile time. Unfortunately, we can't tell which libraries use runfiles paths, so we have to assume it's all of them.

Ideally, we would have one neverlink java_library exposing a CURRENT_REPOSITORY_NAME constant per repository and an implicit dependency of every Java target in that repository on that library. That doesn't seem feasible in Starlark though since it would essentially require the ability to inject a target into every top-level BUILD file (please correct me if I'm wrong!). Since adding actions to generate the library and compile it to every Java target would prohibitively increase the action count, @lberki suggested to build this logic right into buildjar.

I'm happy to provide more context if needed.

[lberki]

There are two things to figure out:

1. the semantics of what @fmeum calls "non-strict runfiles dependencies": what happens if rule A creates a Runfiles object from an artifact from its direct dependency B which is in fact created by a transitive dependency of B called C. This is interesting because C might be in a repository that does not have a mapping for the package of A.

2. Whether we can do any better than my simplistic proposal "just all the repo mappings in the transitive closure"

I was thinking about (2) for a bit but I couldn't think of a good alternative that doesn't require iterating over all Artifacts in a Runfiles when constructing the latter. This might still be acceptable because I could imagine handling Artifacts that go into a Runfiles by way of #addTransitiveArtifacts() differently than ones that get there by way of #merge: this is OK because the latter kind has already gone through the processing to extract repo mappings from it since it's already in a Runfiles.

In addition, even if one iterated over nested sets passed in through #addTransitiveArtifacts() and looked at their owners, they would presumably be canonical repositories, which doesn't help because we are looking for the apparent repository they are seen through and one can't get from the canonical repository to the apparent one, can they? (it's not even necessarily a one-to-one mapping)

Given this, we either do a complicated thing like this about (2) and try to cull the repo mappings written to the runfiles manifest or give up and do the full transitive closure and then (1) doesn't need to be answered. I'm leaning towards the latter because it's simpler, but I haven't spent a lot of time thinking about bzlmod so it's mostly due to the "prefer simplicity" heuristic than deep thought about the ramifications of this decision.

[fmeum]

I think that we can (at least for the beginning) just emit the repo mappings for the full transitive closure. There is already some amount of filtering in the proposal since we only emit repo mapping entries mapping to canonical repo names of repos that actually provide runfiles.

I don't see how we could fit "non-strict runfiles dependencies" in this scheme though. AFAIU, the problem isn't that we don't collect enough information, it's that we don't have an identifier to refer to these files except by their canonical labels - there is no apparent name for them users could provide to look up the canonical name.

This situation is similar to what was discussed about canonical label literals in general, for example here. The recursive label idea seems too complicated for me to introduce for this niche use case - not having users rely on the contents of targets they don't have repo visibility in sounds more like a plus to me. Note that targets could always propagate runfiles library paths in code if any consumer of a library needs to look up a file they may not have repo visibility in and thus a name for.

@lberki I'm not sure I fully understood your points above, please let me know if you had something else in mind.

[cushon]

Ideally, we would have one neverlink java_library exposing a CURRENT_REPOSITORY_NAME constant per repository and an implicit dependency of every Java target in that repository on that library. That doesn't seem feasible in Starlark though since it would essentially require the ability to inject a target into every top-level BUILD file (please correct me if I'm wrong!).

Would it be theoretically possible to have an explicit rule per repository that exposed CURRENT_REPOSITORY_NAME, and make that an explicit dep of other rules in that repository that needed it?

That requires more typing than implicitly creating the rule and adding it as a dep, and it's potentially error-prone if a target depends on one of those rules from a different repository.

[fmeum]

That would be possible. A macro could help by automatically limiting visibility to //:__subpackages__. The downside of this pure-Starlark approach is that using a runfile would now require quite a number of steps:

1. Adding a new target (once per repo).
2. Depending on that target.
3. Depending on @bazel_tools//tools/java/runfiles.
4. Importing CURRENT_REPOSITORY_NAME and passing it to the Rlocation function.

@lberki What do you think, would the above make for an acceptable user experience?

[lberki]

@cushon how is this extra magic rule better than having a Starlark symbol contain it? (I'm not sure which approach is better, I'm trying to map out the design space here)

[lberki]

@fmeum you got my worries about runfiles semantics right; I also think that the recursive label syntax as proposed in the thread you linked is a lot of complexity for not a lot of benefit, or at least not a lot of benefit that's obvious now and therefore I'd rather not do that right now.

I thought that the solution to non-strict dependencies is to have custom logic that propagates the apparent repository names in the code itself. For example, if binary A has a transitive dependency on library B that depends on runfiles from its direct dependency C, B may need to say things like "I'd like runfiles from C and I am B". But that needs one of the following two things:

1. the transitive repository mapping information in the runfiles manifest of A. Then B could directly ask "I am canonical-B, where is apparent-C?" from the runfiles library.
2. The direct repository mapping information accessible from Starlark in B and some language-specific way (implementable in Starlark) to plumb that information to runtime. Then the question to the runfiles would be "I am canonical-B, where is canonical-C?" and "canonical-C" would be computed by Starlark code and compiled into B.

IOW, the binary at runtime does need transitive repository mappings to be able to find its runfiles. The questions are:

1. Whether that is carried in the runfiles manifest or transmitted to the binary by some language-specific logic
2. Whether we want all the mappings from the whole transitive closure or just the subset of it that's actually used

These are coupled because it looks like there isn't an automatic way for Bazel to figure out which runfiles are requested, so it must either assume "everything" (in (1)) or rely on it being told that (in (2))

I originally dismissed (2) since y'all seem to think that it's too onerous,.

[fmeum]

@lberki I don't see how would a Starlark symbol would help here - the difficult part is getting the content of that symbol into Java code. If that requires knowing about the "constants are inlined at compile-time" trick, I think that we shouldn't leave users to do this themselves rather than providing a rule that does it for them.

[lberki]

Huh, I'm confused. I thought we agreed upon:

1. Plumbing the name of the current canonical repository somehow to the rule being analyzed (I have my preferences how, but I don't mind doing something else than my heart's desire)
2. That it's possible for Java rules to embed the canonical repository name into the code (I thought the JavaBuilder hack worked well enough, but I don't mind other solutions as long as it's not a CPU/RAM usage hit)

And now we are discussing between these two options:

1. Embedding the full transitive repository mappings into the runfiles manifest and the runfiles library getting a (canonical repo, apparent repo of direct dep) pair
2. Embedding only the top-level repository mappings into the runfiles manifest and embedding the apparent path -> actual runfiles path mapping into the code just like we are planning to embed the canonical repository name

and that we think (1) is the best alternative and we are exploring (2) in order to explore the design space.

What did I get wrong here?

[fmeum]

@lberki I'm confused as well, so let's go over the points and gain clarity.

1. Plumbing the name of the current canonical repository somehow to the rule being analyzed (I have my preferences how, but I don't mind doing something else than my heart's desire)

Assuming a Starlark rule, isn't that just ctx.label.workspace_name?

2. That it's possible for Java rules to embed the canonical repository name into the code (I thought the JavaBuilder hack worked well enough, but I don't mind other solutions as long as it's not a CPU/RAM usage hit)

It should be possible one way or the other. @cushon inquired about alternatives to the JavaBuilder hack and we came up with one more (the target-per-repo approach), with different tradeoffs: Worse UX, lower complexity on the Bazel side of things and in particular no changes to JavaBuilder.

1. Embedding the full transitive repository mappings into the runfiles manifest and the runfiles library getting a (canonical repo, apparent repo of direct dep) pair

Yep, that is the current state of the proposal and the only one I have considered so far for general use.

2. Embedding only the top-level repository mappings into the runfiles manifest and embedding the apparent path -> actual runfiles path mapping into the code just like we are planning to embed the canonical repository name

Could you elaborate on what you mean by "top-level repository mappings"? Is an accurate example of this point the rules_runfiles approach of emitting a ::runfiles::my_protobuf::protoc constant with value protobuf~3.19.2/protoc? Or are you thinking of representing the "apparent path" as an actual string rather than an identifier?

[lberki]

Good point about ctx.workspace_name! I totally forgot that it exists and then I thought it's the name of the main repository, but a peek a the code reveals that this is not the case.

It appears that we are on the same page about the feasibility of plumbing the name of the current canonical repository to Java code. If you need someone to make a decision, I'd be happy to, but for now, I assume that between you, @cushon and @Wyverald , there is more shared knowledge than in my head so y'all are in a better position to do so.

re: my proposal about "top-level repository mappings", forget about it, I think I had a brain glitch; I thought that with more code in Starlark, one can avoid the complexities of a (current canonical repo name, direct dep apparent repo name) -> direct dep canonical repo name) mapping by calculating that in Starlark, but that doesn't simplify the compiled code in any way because one would still need a (current canonical repo name, <something>) -> runfiles path mapping at runtime.

[fmeum]

Good point about ctx.workspace_name! I totally forgot that it exists and then I thought it's the name of the main repository, but a peek a the code reveals that this is not the case.

ctx.workspace_name is indeed the name of the main repository, but ctx.label.workspace_name is the name of the repository the current target is declared in.

I will some day use the fact that @lberki himself got confused this as a very compelling argument to deprecate and replace it with a more aptly named one. :-)

[cushon]

re: 'constants are inlined at compile-time', one thing I remembered about this is that modern versions of javac emit constant pool entries for inlined constants. In the following example MAX_VALUE is inlined, but there's still a symbol reference to Integer in the constant pool.

So dependency tooling could notice that the definition of the class that contains the repository name is missing.

class T {
  public static void main(String[] args) {
    System.err.println(Integer.MAX_VALUE);
  }
}

$ javap -v -p T
...
  #13 = Class              #14            // java/lang/Integer
  #14 = Utf8               java/lang/Integer
...
         3: ldc           #15                 // int 2147483647
         5: invokevirtual #16                 // Method java/io/PrintStream.println:(I)V

Anyway, from my perspective, doing this in JavaBuilder feels high-magic and I'd like to avoid solving it at that level. But I'm open to hearing out arguments that the target-per-repo alternative isn't good enough (for ergonomics or other reasons).

[fmeum]

@cushon I will prototype the alternative and then we can assess the usability.

@lberki @Wyverald Do you think we could merge the update to the proposal in this form? The section on rules changes is more of a collection of ideas anyway and not meant to be binding.

[Wyverald]

The "manually declare a target per repo" thing feels really clunky to use. But if I'm reading the thread correctly, we don't currently have a better alternative? (Or in case I missed something, could someone describe/link to the alternative?)

The PR looks fine to merge to me.

I will some day use the fact that @lberki himself got confused this as a very compelling argument to deprecate and replace it with a more aptly named one. :-)

Don't get me started...

[fmeum]

The "manually declare a target per repo" thing feels really clunky to use. But if I'm reading the thread correctly, we don't currently have a better alternative? (Or in case I missed something, could someone describe/link to the alternative?)

None that's both more usable and not more complicated to implement and maintain. Overall, we could, in order from most usable to least usable:

1. Build custom logic into JavaBuilder.
2. Revive some Java-specific version of RunfilesLibraryInfo to make the java_* rules add the additional actions when they see a runfiles library.
3. Provide the java_current_repository rule and let users instantiate and depend on it themselves.

[Wyverald]

Thanks for the summary. So it sounds like Option 1 is basically off the table. Option 3 is rather unwieldy IMO and I doubt Bazel users will be very happy about this. @lberki @cushon do you think Option 2 is an acceptable middleground?

[fmeum]

To quote @cushon:

But I'm open to hearing out arguments that the target-per-repo alternative isn't good enough (for ergonomics or other reasons).

I wouldn't call Option 1 off the table just yet. :-) I do agree with @Wyverald that we should try hard to avoid Option 3 if possible.

[fmeum]

The prototype for Option 3 including a test that shows how to use it is available at bazelbuild/bazel#16281.

[Wyverald]

Just restating my opinion that Option 3 is very, very unattractive. If anyone wants to use runfiles, they have to declare a special target and remember to include it in deps -- this is a terrible user experience.

[lberki]

@cushon is there maybe any other clever way to inject some data from the compiler command line into the code to be compiled? This is the perfect use case for C++-style preprocessor defines; I don't harbor a lot of love or them for all the usual reasons, but this is the perfect use case...

The reason why I'm strongly against (3) is usability: one would both need to remember to declare the target and to depend on it.

Would it be possible to work around the dangling reference problem brought up by @cushon by artificially injecting that class in every java_binary with some stub value (or even java_library; they are not used anyway...)

[cushon]

is there maybe any other clever way to inject some data from the compiler command line into the code to be compiled?

Annotation processing is about as close as Java gets to c-style preprocessing. You could have a javac flag -ArepositoryName=... that an annotation processor read and then generated code based on, but the generated code is actually linked in to the output so you either end up with ODR violations or need to deal with the generated code being in a different package for each repository.

We currently stamp jar manifests with target labels, we could potentially include the repo name in the manifest and then it would be accessible at runtime. That doesn't play nicely with _deploy.jars, though. Are most of the uses of this going to be in tests?

one would both need to remember to declare the target and to depend on it

Maybe this speaks to a need for better dependency management tooling

[fmeum]

We currently stamp jar manifests with target labels, we could potentially include the repo name in the manifest and then it would be accessible at runtime. That doesn't play nicely with _deploy.jars, though.

Depending on how you stringify the label, it may actually already contain the canonical repository name. But as you say, this wouldn't work for deploy jars or really any kind of custom post-processing.

Are most of the uses of this going to be in tests?

Not necessarily, we internally rely on runfiles quite heavily even in code shipped to users.

I'm investigating a new idea:

• Turn the java_* rules into macros (there is precedent for that with cc_test) that provide the current canonical repository name as a rule attribute via native.repository_name.
• Add implicit rule attributes referencing a single java_current_repository target in @bazel_tools through a transition that sets a custom build setting to the value of the repo name rule attribute.
• Read and use the build setting in java_current_repository.

I will report back on whether I got this to work.

[fmeum]

I got this to work for Starlark java_library: https://github.com/bazelbuild/bazel/pull/16281/files
I would expect this to translate to native code in case Java rules aren't Starlarkified by default in Bazel 6, but haven't tried building that yet.

@lberki @cushon Do you see potential performance issues? It could make sense to run this by gregestren and comius (not mentioning them here yet as that may lack context).

Edit: This would result in DumpPlatformClassPath rerunning for every repository using Java, but we should be able to avoid that overhead by invoking javac directly on the generated class rather than going through java_common.compile, similar to https://github.com/bazelbuild/bazel/blob/92f5e8f305262d097d692b09a08f7a655595c00f/tools/jdk/default_java_toolchain.bzl#L181.

We could add a stub of the generated class with Javadocs to the runtime_deps of @bazel_tools//tools/java/runfiles to help IDEs. I would consider adding a class to the runtime classpath of all Java targets too intrusive.

[Wyverald]

@lberki @cushon any concerns with this new approach? The usability seems fine (this is basically completely transparent to users).

[cushon]

@comius

[fmeum]

I implemented the approach described above both in Starlark (for java_library) and Java (for java_binary and java_test) in bazelbuild/bazel#16281. It does work and keeps the number of additional actions linear in the number of repos using Java (rather than Java targets), but I haven't performed any benchmarks on how the additional transition affects analysis time and memory usage.

[lberki]

I was hoping @comius would chime in (or @hvadehra ) but in their absence, my opinion will have to do.

This approach relies on final constant inlining, just like the JavaBuilder one. It feels way more convoluted than what we thought we'd settle on, but since no one has come up with a better one, I guess it'll have to be this one? Mirroring @cushon 's opinion that doing this in JavaBuilder is high magic, I have a similar opinion about doing this within Bazel, but as long as we agree that the final API will be a constant that'll then be inlined, we are just talking about implementation details that can be later changed at will so I don't particularly mind either way.

I have a number of issues with the code in bazelbuild/bazel#16281 ,but they are not absolute blockers:

1. The use of @bazel_tools. We want to eventually remove code from there, but the alternative is @rules_java and adding a mandatory dependency on that repository that cannot be stubbed out seems like a big step I'd rather not take as a side effect of adding a feature in the dash towards Bazel 6.0 .
2. Turning java_library into a macro is reasonable as long as bazel query --output=label_kind returns java_library. Do I understand correctly that this is because the name of the current repository cannot be injected into the configuration transition in any other way? If so, I would consider extending the API of either configuration transitions (/cc @gregestren ) or computed defaults so that they are informed about the label of the current rule, but (again), that's an implementation detail.

[fmeum]

@lberki Re 2.: This is possible, it just requires some mild trickery and splitting up .bzl files (see @comius commit bazelbuild/bazel@2cde45e). I can take care of that, but would prefer to get the general idea approved first.

There are a number of tests that assert something about the compile-time classpath of Java rules. Should I add the generated jar to them?

Re doing this in Bazel: This is only necessary as long as the Java rules haven't been fully starlarkified. Once that's done, all of this can be done in rules_java using public API.

[lberki]

@fmeum for lack of a better alternative, as long as @comius and @Wyverald are fine with this plan, I am fine with it, too.

What's important for me is that the programmatic API remains as simple as "import this magic class, use this magic public static final field from it; then the rest can remain implementation detail and be changed at will.