Support overridden repositories.

[ksnox]

The repository lookup logic no longer fails to find paths that have been altered by using overrides such as local_path_override or git_override.

[achew22]

This is very interesting. I've not kept informed about what the current state of Bazel is on overrides. Two things I would like to see before I will merge this.

1. Please add some comments that link to documentation of this behavior in the override context
2. I would also like to see some testing of this functionality to ensure it behaves in the right way

[ksnox]

I cannot find documentation stating the "non-porcelain" structure of the external/ directory. I can only find askance references that mention the possible presence of a "+" character[0]. Would that be sufficient?

I've added a rudimentary test that ensures the new lookup function falls back on the previous behavior. I would need an end-to-end integration test without mocking bazel to ensure conformance. The only existing tests are mocks e2e tests and these would require me to fake bazel's behavior.

[0] The Bazel Query Reference

[achew22]

There are a whole bunch of e2e tests that you could model your test off of as well

[achew22]

Looking at The Bazel Query Reference I'm not sure this gives me confidence on the translation scheme from + to actual filenames. Sorry, this is just a part of Bazel I have no familiarity with. Is there anything that might provide more context?

[ksnox]

Thank you for the reference to the whole bunch of e2e examples. Here's your reward[0]. It looks like the sh_binary rule which does not use a runfiles library must rummage through the file system. Lo and behold the + modifier is present on the path to the local_path_override-n library.

Additionally, I've run the new e2e test with and without my feature and I've observed the expected results.

0

[ksnox]

The e2e test fails in the CI environment because the configured bazel version [0] does not use the MODULE.bazel required by the test[1].

Can I fix this using a .bazelversion configuration?

Relevant error:

ERROR: /var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/bazel_testing/bazel_go_test/main/BUILD.bazel:1:10: no such package '@secondary//': The repository '@secondary' could not be resolved: Repository '@secondary' is not defined and referenced by '//:test'

0
1

[achew22]

Trying to be as helpful as I can here, but I know very little about windows development. First let's grab the germain part of the log.

C:\\b\\gx4a3jy3\\execroot\\__main__\\_tmp\\720a5f703af25788c71c4e9c9c043e06\\_bazel_b\\mj7aq3mf\\execroot\\_main\\bazel-out\\x64_windows-fastbuild\\bin\\test: line 3: ../secondary*/lib.sh: No such file or directory
     C:\\b\\gx4a3jy3\\execroot\\__main__\\_tmp\\720a5f703af25788c71c4e9c9c043e06\\_bazel_b\\mj7aq3mf\\execroot\\_main\\bazel-out\\x64_windows-fastbuild\\bin\\test: line 5: ..\secondary*\lib.sh: No such file or directory
       C:\\b\\gx4a3jy3\\execroot\\__main__\\_tmp\\720a5f703af25788c71c4e9c9c043e06\\_bazel_b\\mj7aq3mf\\execroot\\_main\\bazel-out\\x64_windows-fastbuild\\bin\\test: line 6: say_hello: command not found

https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/0196df81-453c-4400-9e24-c74153970d43/internal%5Ce2e%5Clocal_path_override%5Clocal_path_override_test%5Ctest_attempts%5Cattempt_1.log

[achew22]

Reading through the docs, I wonder if the buildkite runners have mingw or related installed

[achew22]

What if instead of running a bash script which seems to be a bit difficult here for some reason, we did something like:

genrule(
    name = "test",
    srcs = ["@secondary//:file"],
    outs = ["test.txt"],
    cmd = "cp $(location @secondary//:file) $@",
)

then you could ibazel build the file in the test, confirm the original value, overwrite the file and read the resulting output to confirm the new value. Since this is basically the "my first genrule" tutorial which I've seen a couple dozen times I'm pretty confident it'll work by whatever interpreter is failing on your slightly more complex (but still so simple it should work) script.

I don't believe that there is a commensurate operation to ExpectOutput for reading an output file, but I think something in /internal/e2e/ibazel.go like this may be useful.

func (i *IBazelTester) ExpectOutputFile(path string, matcher *regexp.Regexp, delay ...time.Duration) {
	i.t.Helper()

	d := defaultDelay
	if len(delay) == 1 {
		d = delay[0]
	}

  var lastErr error
	stopAt := time.Now().Add(d)
	for time.Now().Before(stopAt) {
		time.Sleep(5 * time.Millisecond)

		b, err := os.ReadFile(path)
    if err != nil {
      lastErr = fmt.Errorf("os.ReadFile(%q): %v", path, err)
      continue
    }
    if matcher.Match(b) {
      return
    }
    lastErr = fmt.Errorf("ExpectOutputFile(%q): %v did not match the file. contents:\n\n%s", path, matcher, string(b))
	}

  if lastErr != nil {
    i.t.Error(err)
    return
  }
}

[ksnox]

I appreciate all of the help here, but I don't think the feature actually works under Windows, so further work to correct the sh_binary behavior is actually futile. This would explain why the similar test in override_repository_test[3] skips over Windows.

If you check the same log, both invocations of the ibazel.ExpectOutput [0] [1] are given the same logging[3] from ibazeltester. You can verify this checking by the timestamps. This indicates to me that Bazel is not actually being invoked because of the file watcher logic.

0: local_path_override_test.go:116
1: local_path_override_test.go:121
2: bazel-watcher/internal/e2e/override_repository/override_repository_test.go at master · bazelbuild/bazel-watcher
3: storage.googleapis.com/bazel-untrusted-buildkite-artifacts/0196df81-453c-4400-9e24-c74153970d43/internal%5Ce2e%5Clocal_path_override%5Clocal_path_override_test%5Ctest_attempts%5Cattempt_1.log

[ksnox]

Are Windows tests a hard requirement for landing this change?