How to handle mixed lockfile presence in non-root modules

[aaliddell]

Since protocolbuffers/protobuf@1999135 (aka 32.0+), the protobuf dependency in BCR uses maven.install with a lockfile on the @maven name instead of @protobuf_maven: https://github.com/protocolbuffers/protobuf/blob/894039e7512e9940095a0bf04214adcba7a37adb/MODULE.bazel#L221-L228

Similarly, following feedback on the same logic, since 5.4.0 rules_proto_grpc pulls its 'user' dependencies into the same @maven repo, without a lockfile: https://github.com/rules-proto-grpc/rules_proto_grpc/blob/e40a70e7375f6dab68d402e78ff540c9d8177a09/modules/java/MODULE.bazel#L22-L25

A top level user may use rules_proto_grpc_java, which has a transitive dependency on protobuf and if they do not specify a lock file (or don't even use maven.install) then the protobuf lock file 'wins' and only its dependencies show up in the @maven repo and attempting to use a dep from outside the lockfile errors with a missing target.

See https://buildkite.com/bazel/rules-proto-grpc-rules-proto-grpc/builds/3744/steps/canvas?sid=0198eb95-044b-439a-9710-7bce4943ba79 for an example error

How is this meant to be handled when multiple non-root modules need to inject deps into the @maven name?

• I cannot also add a lock file to rules_proto_grpc with the additional deps like protobuf has, as only a single non-root lock file is permitted (note also that this code below fails to print the error because lock_files is an array of labels not strings):

rules_jvm_external/private/extensions/maven.bzl

Lines 631 to 636 in 7d00153

	for repo_name, lock_files in repo_to_lock_file.items():
	if len(lock_files) > 1:
	fail("There can only be one lock file for the repo %s. Lock files seen were %s" % (
	repo_name,
	", ".join(lock_files),
	))

• Should the protobuf repo drop their lockfile?

• Or is this additional functionality that needs adding to rules_jvm_external?

[shs96c]

That sounds like a bug.

I think the correct logic would be that if the workspace is defined in the root module, then only the root module's lock file should be used. If the workspace is not defined in the root module, then the one from the non-root module should be used, with multiple lock files being considered an error.

Does that sound right to you?

[shs96c]

As a work-around, specifying a lock file in the root module will work, and will also mean faster builds as you won't need to do the resolution every time the bazel daemon starts.

[aaliddell]

Correction: adding just a dummy empty maven.install() in the root repo causes the build to pass, due to this section where the root module 'always wins' and wipes out all the lockfiles collected above:

rules_jvm_external/private/extensions/maven.bzl

Lines 606 to 613 in 7d00153

	# The root module always wins when it comes to these values
	for mod in mctx.modules:
	if mod.is_root:
	for install in mod.tags.install:
	repo = repos[install.name]
	# We will always have a lock file, so this is fine
	repo_to_lock_file[install.name] = [install.lock_file]

Since the above code effectively causes all transitive lockfiles to be ignored based on the mere presence of an extension tag in the root repo, I'm inclined to say that lockfiles from non-root modules should be ignored in all circumstances and perhaps a warning issued.

[aaliddell]

That sounds like a bug.

I think the correct logic would be that if the workspace is defined in the root module, then only the root module's lock file should be used. If the workspace is not defined in the root module, then the one from the non-root module should be used, with multiple lock files being considered an error.

Does that sound right to you?

This is current behaviour, which is what is problematic I think, as the transitive dep tree can contain at most a single lockfile before an error is hit. Since protobuf in BCR now does this, no other module can and protobuf is widely pulled in transitively by language rules. Since transitive lockfiles are silently ignored in some circumstances anyway, the behaviour could instead be:

• If the root module uses a lockfile, it is used
• Lockfiles from transitive modules are always ignored

The alternative to this is that the lockfiles are somehow merged, which sounds like a world of pain when the inevitable conflicts are hit

Edit: this might be special-cased for the common @maven repo, as that is intended to be shared. I see what you mean about lockfiles being used for per-module deps that aren't being injected into the common repo

[aaliddell]

To allow non-@maven lockfiles, the behaviour could actually be:

• If the root module specifies a lockfile for a repo, it is used unconditionally (as it is today)
• If only a single non-root module contributes to a repo (e.g. @rules_some_lang_maven), then its lockfile will be used if present
• If multiple non-root modules contribute to a repo (i.e @maven), then transitive lockfiles are ignored

This allows modules to have their own local locked maven deps, whilst also allowing the @maven name to be shared.

[aaliddell]

Would you accept a PR implementing the above? I now cannot escape this issue, as rules_java now transitively depends on the version of protobuf that trips this up, see here (which is a testing suite from a PR that only updates rules_java). I expect anyone similarly using the @maven name will start having problems from rules_java 8.16+ and the error message is particularly opaque as maven deps are just randomly missing