Provide BAZEL_CURRENT_REPOSITORY local define in cc_* rules

[fmeum]

BAZEL_CURRENT_REPOSITORY contains the canonical name of the repository
containing the current target and is required to look up runfiles using
apparent repository names when repository mappings are used, e.g. with
Bzlmod.

Work towards #16124

[fmeum]

@oquenchil I went with the slightly less verbose BAZEL_CURRENT_REPOSITORY instead of BAZEL_CURRENT_REPOSITORY_NAME as suggested in the proposal. Which name do you like best?

Do you happen to have an idea where I need to add cc_runfiles_library.bzl to fix the bootstrap dist test on Windows? Fixed.

[fmeum]

Converted to Draft due to the possible changes to the proposal: bazelbuild/proposals#274

[oquenchil]

LGTM

[oquenchil]

Converted to Draft due to the possible changes to the proposal: bazelbuild/proposals#274

Saw this too late. Let me know if I need to take another look.

[fmeum]

@oquenchil After a discussion with @lberki, we decided to ditch RunfilesLibraryInfo. That greatly simplifies the PR - please take another look. Do you see a problem with having additional local_defines for all cc_* targets?

[fmeum]

I will flesh out the test a bit more.

[oquenchil]

For literally all cc_* targets? Do you mean adding the attribute to all cc_* rules and the targets that need it would use it?

[fmeum]

For literally all cc_* targets? Do you mean adding the attribute to all cc_* rules and the targets that need it would use it?

Yes, literally all of them. But by implicitly adding to the local_defines parameter of cc_common.compile rather than literally adding to the local_defines rule attribute.

[oquenchil]

Ah then the latter is totally fine.

[fmeum]

@oquenchil I improved the test. Since it relies on running an external test, I temporarily stacked this on #16214. When that has been merged, I will rebase on master and mark as ready. But you can already review.

[fmeum]

@oquenchil Test failures are fixed and this PR is no longer stacked.

[Wyverald]

Looks good :) Is this ready for merge?

[fmeum]

Looks good :) Is this ready for merge?

Yes, it's ready.

[BoleynSu]

Wouldn't this violate ODR for some cc_libaray?

Suppose we have cc_library(hdr) that use this macro through runfiles API and it then is depended in two different repo?

To resolve this, we need to ask users to never use runfiles API in the header.

[BoleynSu]

@fmeum for visibility

[fmeum]

I would have said no: As this is a local_define, it's only set when compiling the sources of the current cc_library and not exposed transitively.

@BoleynSu Could you provide an example of a situation you are worried about?

[BoleynSu]

For example, a header uses the macro in a inline function/function template and then this header get used by two source files in two different repo. Sent from https://boleyn.su/phone

…

On Sun, Mar 12, 2023, 16:34 Fabian Meumertzheim ***@***.***> wrote: I would have said no: As this is a local_define, it's only set when compiling the sources of the current cc_library and not exposed transitively. @BoleynSu <https://github.com/BoleynSu> Could you provide an example of a situation you are worried about? — Reply to this email directly, view it on GitHub <#16216 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXVSKAFOM5A6FRYMB4BHHDW3WDCXANCNFSM6AAAAAAQEM5JQM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[BoleynSu]

A concrete example can be get_repo_name.h with inline const char* attribute (noinline) get_repo_name() { return the_macro; } If this function get called from two repos, we violet ODR. Typically linker will choose the first definition. Thus, it returns the same value for both source files. The noinline attribute is added to force the compiler to not inline the function which compiler will automatically do for more complex inline functions. Sent from https://boleyn.su/phone

…

On Mon, Mar 13, 2023, 01:29 Boleyn Su ***@***.***> wrote: For example, a header uses the macro in a inline function/function template and then this header get used by two source file in two different repo. Sent from https://boleyn.su/phone On Sun, Mar 12, 2023, 16:34 Fabian Meumertzheim ***@***.***> wrote: > I would have said no: As this is a local_define, it's only set when > compiling the sources of the current cc_library and not exposed > transitively. > > @BoleynSu <https://github.com/BoleynSu> Could you provide an example of > a situation you are worried about? > > — > Reply to this email directly, view it on GitHub > <#16216 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAXVSKAFOM5A6FRYMB4BHHDW3WDCXANCNFSM6AAAAAAQEM5JQM> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

[BoleynSu]

@fmeum friendly ping

[fmeum]

@BoleynSu I agree that that situation would be UB, but is there more to it than "exposing macros via inline functions is potentially dangerous"? Wouldn't the same apply to e.g. the __FILE__ macro?

[BoleynSu]

I don't think so. FILE macro in a header won't change its value wherever you include it from. Sent from https://boleyn.su/phone

…

On Tue, Mar 14, 2023, 20:31 Fabian Meumertzheim ***@***.***> wrote: @BoleynSu <https://github.com/BoleynSu> I agree that that situation would be UB, but is there more to it than "exposing macros via inline functions is potentially dangerous"? Wouldn't the same apply to e.g. the __FILE__ macro? — Reply to this email directly, view it on GitHub <#16216 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXVSKCHLSHKXYQEMP7KJL3W4BQLPANCNFSM6AAAAAAQEM5JQM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[fmeum]

True. Maybe we should prominently explain in the runfiles library documentation that BAZEL_CURRENT_REPOSITORY takes different values depending on the cc_library that includes it? Or do you see a way to improve the way the current repository name is passed in so that it would work in the body of an inline function?

[BoleynSu]

The goal is more to implement a function that can return the current repo name.

Seems like it can be safely implemented with std::source_location (lets assume its avalible for now). Let me try if I could make it work first.

[BoleynSu]

std::string_view current_repo_name(std::string_view file=__builtin_FILE()) {
   if (file.startswith("external/")) return x; // where file = external/x/whatever
   else return REPO_NAME; // where REPO_NAME is the name of the main repo. Thus, the macro will be the same across different repos.
}

We could check if __builtin_FILE exists, if not we could fall back to other solutions.

[fmeum]

Something like that looks pretty good as a modern C++ replacement for what we are doing here. If it requires C++20, we probably have to hide it behind std level checks.

@BoleynSu Since you seem to have this mostly figured out, would you be willing to submit a PR adding this to https://github.com/bazelbuild/bazel/blob/master/tools/cpp/runfiles/runfiles_src.h?

[BoleynSu]

Sure. Will do.

[BoleynSu]

@fmeum Please check #17887