bazel icon indicating copy to clipboard operation
bazel copied to clipboard

Allow `native.package_relative_label` in initializers

Open fmeum opened this issue 11 months ago • 11 comments

Work towards #21622

RELNOTES: native.package_relative_label can now be used in rule initializers.

fmeum avatar Mar 20 '24 11:03 fmeum

FYI @Wyverald

fmeum avatar Mar 20 '24 11:03 fmeum

I’d feel more comfortable accepting just package_relative_label, because it seems this function doesn’t lead into parsing and concatenating the labels and is covered by a clear usecase. Other functions seem more risky.

comius avatar Mar 21 '24 06:03 comius

I’d feel more comfortable accepting just package_relative_label, because it seems this function doesn’t lead into parsing and concatenating the labels and is covered by a clear usecase. Other functions seem more risky.

I have made the change to only allow this function, but I don't quite follow your reasoning: With native.package_relative_label, I can implement native.repo_name as native.package_relative_label("foo").repo_name. The other functions, with the exception of native.module_version, can also mostly be derived from that label. We wouldn't really be exposing more information that way, but improve consistency between the APIs available to macros and initializers. Which additional risk do you see?

fmeum avatar Mar 21 '24 20:03 fmeum

@comius Friendly ping

fmeum avatar Apr 02 '24 08:04 fmeum

I’d feel more comfortable accepting just package_relative_label, because it seems this function doesn’t lead into parsing and concatenating the labels and is covered by a clear usecase. Other functions seem more risky.

I have made the change to only allow this function, but I don't quite follow your reasoning: With native.package_relative_label, I can implement native.repo_name as native.package_relative_label("foo").repo_name. The other functions, with the exception of native.module_version, can also mostly be derived from that label. We wouldn't really be exposing more information that way, but improve consistency between the APIs available to macros and initializers. Which additional risk do you see?

The problem is that once you have native.repo_name you can implement native.package_relative_label. But in any such implementation you probably need to make assumptions about how a label is composed of different parts.

Also I see package_relative_label backed up by a use-case. Other functions aren't.

comius avatar Apr 09 '24 07:04 comius

The problem is that once you have native.repo_name you can implement native.package_relative_label.

I see your point, just want to remark that this is not possible: native.package_relative_label crucially applies the calling package's repository mapping to resolve the apparent to a canonical repo name, which is not something you can't emulate with any other API.

fmeum avatar Apr 09 '24 08:04 fmeum

@comius I special cased the setup for native.package_relative_label by just providing the LabelConverter as a thread local, thus getting rid of the new phase. PTAL, this is much simpler now.

fmeum avatar Apr 09 '24 09:04 fmeum

@comius Friendly ping

fmeum avatar Apr 18 '24 19:04 fmeum

@bazel-io fork 7.2.0

fmeum avatar Apr 25 '24 06:04 fmeum

@comius @iancha1992 Friendly ping, is this still on track for merging?

fmeum avatar May 03 '24 07:05 fmeum

@comius @iancha1992 Friendly ping, is this still on track for merging?

@fmeum Yes. We are currently working on this :)

iancha1992 avatar May 03 '24 07:05 iancha1992

@fmeum can you please resolve the conflicts from src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java? There is a conflict that I could not resolve on my own from the file. Thanks :)

iancha1992 avatar May 07 '24 18:05 iancha1992

@iancha1992 Done

fmeum avatar May 07 '24 19:05 fmeum