runfiles: Implement Runfiles::CurrentRepository for C++ by BoleynSu · Pull Request #17887 · bazelbuild/bazel

BoleynSu · 2023-03-26T13:10:33Z

No description provided.

fmeum · 2023-03-26T15:12:19Z

tools/cpp/runfiles/runfiles_src.cc

+    defined(BAZEL_RUNFILES_MSVC_HAS_BUILTIN_FILE)
+std::string Runfiles::CurrentRepository(std::string file) {
+  std::string repo;
+  if (file[0] == 'e' && file[1] == 'x' && file[2] == 't' && file[3] == 'e' &&


Could this use startswith? In any case it would need bounds checking as file may be too short.

Good catch! Is path always using forward slash, i.e., even on Windows?

I don't know, but the tests should tell us.

fmeum · 2023-03-26T15:12:37Z

src/test/shell/bazel/cc_integration_test.sh

  expect_log "in external/other_repo/pkg/test.cpp: 'other_repo'"
  expect_log "in pkg/library.cpp: ''"
+
+  # test Runfiles::CurrentRepository()


Could you split this out into a new test?

fmeum · 2023-03-26T15:13:28Z

src/test/shell/bazel/cc_integration_test.sh

+}
+EOF
+
+  bazel run //pkg:binary &>"$TEST_log" || fail "Run should succeed"


I think that the test fails because Bazel uses C++11 by default. You probably have to add --copt=c++20.

Currently, using CurrentRepository will fail when the cc toolchain does not support __builtin_FILE. I guess it is failing in such cases. Could we just check if it is supported and skip the rest of not?

We could, but I think that the test just wouldn't run if we did: Since Bazel passes --std=0x on Unix by default, it just won't be available on any of the runners.

It is more related to compiler version. Some older compiler does not support __builtin_FILE. I added a cc_test if test if __builtin_FILE is supported or not and skip the test if it is not supported.

BoleynSu · 2023-03-26T16:54:39Z

tools/cpp/runfiles/runfiles_src.h

 //         // Important:
 //         //   If this is a test, use
-//         //   Runfiles::CreateForTest(BAZEL_CURRENT_REPOSITORY, &error).
+//         //   Runfiles::CreateForTest(Runfiles::CurrentRepository(), &error).


We can also make the repo parameter default to CurrentRepo such that user can use Create() or CreatForTest() to use the current repo where these function get called. However, this is a breaking change as currently it defaults to the main repo. Do we want to add a new function for this?

BoleynSu · 2023-04-16T11:02:34Z

I just found that cc_integration_test is disable for windows. Do we want to test it for windows?

fmeum · 2023-04-25T13:00:09Z

I think we do. You could move the test to src/test/py/bazel/bzlmod/bazel_repo_mapping_test.py, which should be running on an all platforms and already contains related tests.

BoleynSu · 2023-04-25T15:51:57Z

I think we do. You could move the test to src/test/py/bazel/bzlmod/bazel_repo_mapping_test.py, which should be running on an all platforms and already contains related tests.

I have split the test to a new file which has passed the test on all platforms. PTAL

BoleynSu · 2023-05-02T23:51:30Z

Friendly ping

fmeum · 2023-05-03T17:50:34Z

tools/cpp/runfiles/runfiles_src.h

+  // (also known as the workspace).
+  static std::string CurrentRepository(const std::string& file = __builtin_FILE());
+ private:
+  static std::string GetPrefixHint() {


Could you add a comment here explaining the purpose of this function? Doesn't have to be user-facing as it is private, but dev-facing.

fmeum · 2023-05-03T17:51:00Z

tools/cpp/runfiles/runfiles_src.h

+#if defined(BAZEL_TOOLS_CPP_RUNFILES_HAS_BUILTIN_FILE)
+  // Returns the repository name.
+  //
+  // Use this from within `cc_test` rules.


This and the next two paragraphs seem outdated.

fmeum · 2023-05-03T17:51:40Z

tools/cpp/runfiles/runfiles_src.h

 //       using bazel::tools::cpp::runfiles::Runfiles;
 //
 // 3.  Create a Runfiles object and use rlocation to look up runfile paths:
+//     Note that if Runfiles::CurrentRepository() is not supported for the cc


It could make sense to mention that C++20 is a requirement (if that's actually true) to give users an idea when they should expect this to be available.

fmeum · 2023-05-03T17:53:02Z

tools/cpp/runfiles/runfiles_src.cc

  return s.rfind(suffix) == s.size() - suffix.size();
 }

+std::vector<std::string> split(const std::string& s, const std::string& sep) {


If sep is always "/\\", we could inline that value, remove the parameter and call the function split_path.

fmeum · 2023-05-03T17:54:35Z

tools/cpp/runfiles/runfiles_src.cc

+  }();
+  std::vector<std::string> paths = split(file, "/\\");
+  if (paths.size() > prefixes.size()) {
+    if (std::equal(prefixes.begin(), prefixes.end(), paths.begin())) {


Could you give some examples of prefixes this typically strips away? Assuming Bazel passes in relative paths, I would have expected the prefix to be empty or bazel-out/<something>/bin, in which case we could potentially simplify this logic.

This logic is used on MSVC where absolute path is returned from builtin_FILE for headers. Let's add some comments on why we need GetPrefixHint.

That sounds like a potential source for non-hermetic builds. Do you know whether there is a way to get MSVC to return relative paths from builtin_FILE? That would solve the hermeticity issue and simplify the logic here.

Not really. I do not have MSVC at hand to dig deeper. Seems this only happens we call builtin_FILE inside headers.

I will see whether I can find anything and if not, we can still go with the current approach.

fmeum · 2023-05-13T11:04:29Z

src/test/shell/bazel/cc_runfiles_test.sh

+
+  bazel build pkg:feature_test &>"$TEST_log" || fail "Build should succeed"
+  if ! bazel test pkg:feature_test &>"$TEST_log"; then
+    echo "test skipped because __builtin_FILE is not supported" >&2


Do you know which CI runners actually end up running this test? I'm still a bit concerned that it's not running on at least one runner per OS

Not sure. At least, the runners that passes before adding this guard should still be running it. Seems to me it is better to add tags to exclude this test from some runners than the current approach. However, I do not know if such mechanism exists or how it is like here.

github-actions · 2025-03-03T01:33:14Z

Thank you for contributing to the Bazel repository! This pull request has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this PR is still relevant and should stay open, please post any comment here and the PR will no longer be marked as stale.

satyanandak · 2025-05-12T10:53:04Z

@BoleynSu Could you provide an update on this PR?

meisterT · 2025-09-16T09:19:04Z

@BoleynSu please re-open if you are still interested

BoleynSu mentioned this pull request Mar 26, 2023

Provide BAZEL_CURRENT_REPOSITORY local define in cc_* rules #16216

Closed

fmeum reviewed Mar 26, 2023

View reviewed changes

BoleynSu commented Mar 26, 2023

View reviewed changes

sgowroji added team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels Mar 27, 2023

buildbreaker2021 self-assigned this Apr 3, 2023

BoleynSu force-pushed the master branch 3 times, most recently from 6fb9077 to ee0893d Compare April 16, 2023 11:02

BoleynSu force-pushed the master branch 15 times, most recently from b5d518e to 237644f Compare April 16, 2023 13:56

runfiles: Implement Runfiles::CurrentRepository for C++

bb273bb

BoleynSu force-pushed the master branch from 237644f to ab9ed7f Compare April 16, 2023 14:01

runfiles: split cc_runfiles_test from cc_integration_test

b6c2372

BoleynSu force-pushed the master branch from ab9ed7f to b6c2372 Compare April 16, 2023 14:31

fmeum reviewed May 3, 2023

View reviewed changes

fmeum reviewed May 13, 2023

View reviewed changes

comius added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Dec 28, 2023

github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Mar 3, 2025

github-actions bot removed the stale Issues or PRs that are stale (no activity for 30 days) label May 13, 2025

meisterT closed this Sep 16, 2025

Conversation

BoleynSu commented Mar 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BoleynSu Mar 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BoleynSu commented Apr 16, 2023

Uh oh!

fmeum commented Apr 25, 2023

Uh oh!

BoleynSu commented Apr 25, 2023

Uh oh!

BoleynSu commented May 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 3, 2025

Uh oh!

satyanandak commented May 12, 2025

Uh oh!

meisterT commented Sep 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

BoleynSu Mar 26, 2023 •

edited

Loading