Redact absolute root module file path in MODULE.bazel.lock

[fmeum]

The absolute path of the root module file in Starlark Locations is replaced with a constant when writing to the lock file.

Work towards #18936

[fmeum]

The new test is failing on Windows:

======================================================================
FAIL: testNoAbsoluteRootModuleFilePath (__main__.BazelLockfileTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\temp\Bazel.runfiles_ugd2_j1m\runfiles\io_bazel\src\test\py\bazel\bzlmod\bazel_lockfile_test.py", line 553, in testNoAbsoluteRootModuleFilePath
    self.assertIn(
AssertionError: 'ERROR: module extension "other_ext" from "//:extension.bzl" does not generate repository "hello", yet it is imported as "other_ext_hello" in the usage at C:/b/u4ube5d5/execroot/io_bazel/_tmp/b19cb386875f88e3fcf9655c858fde97\\tests_root\\tmpu1fpr48_\\MODULE.bazel:4:26' not found in ["$TEST_TMPDIR defined: output root default is 'c:\\b\\u4ube5d5\\execroot\\io_bazel\\_tmp\\b19cb386875f88e3fcf9655c858fde97' and max_idle_secs default is '15'.", 'Starting local Bazel server and connecting to it...', 'Loading:', 'Loading:', 'Loading: 0 packages loaded', 'ERROR: module extension "other_ext" from "//:extension.bzl" does not generate repository "hello", yet it is imported as "other_ext_hello" in the usage at C:/b/u4ube5d5/execroot/io_bazel/_tmp/b19cb386875f88e3fcf9655c858fde97/tests_root/tmpu1fpr48_/MODULE.bazel:4:26', 'INFO: Elapsed time: 3.178s', 'INFO: 0 processes.', 'ERROR: Build did NOT complete successfully']

This could be seen as a "real" issue: The location path doesn't use the native path separator. What do you think, should we fix that or just ignore it in the test for now?

Edit: Nvm, this appears to be the default in all Starlark stack traces. I will just adapt the test, which generates a weird mix of separators anyway.

[meteorcloudy]

Nice, thanks for the fix!

@SalmaSamy please take a final look!

[Wyverald]

thank you for the fix, but I was thinking that we could just actually use the label in error messages too (i.e. change the logic in ModuleFileFunction). IMO it's pretty clear which file @@//:MODULE.bazel (or better, if we use Label#getDisplayForm(), //:MODULE.bazel) is referring to. That would also remove this special-casing at lockfile read/write time. WDYT?

[fmeum]

I think that absolute paths still provide the best usability here: You can often just copy&paste the path together with the line number to go to the correct place in your editor. All other Starlark stack traces also contain absolute paths in this place. Building //:MODULE.bazel or querying for it also results in an error about the file not being exported.

I view the switch to labels as a tradeoff that reduces implementation complexity at the cost of usability. This is definitely warranted for non-registry overrides, which are a bit more of an "advanced" concept anyway. But if we can keep absolute paths for the root module file, which I expect to cause most errors arising in practice, with a reasonable amount of complexity, then we should consider doing that.

I am actually not sure anymore whether the display form is better for non-root modules: The canonical form has the advantage that it more directly corresponds to the external path on disk, which is ultimately what the user wants to find if they face an error.

[Wyverald]

This is definitely warranted for non-registry overrides, which are a bit more of an "advanced" concept anyway.

To be clear, is your preference to use labels for non-registry overrides in error messages?

I am actually not sure anymore whether the display form is better for non-root modules: The canonical form has the advantage that it more directly corresponds to the external path on disk, which is ultimately what the user wants to find if they face an error.

I don't really mind it either way. The difference would only be @foo//:MODULE.bazel vs @@foo~override//:MODULE.bazel, which is not a huge deal IMO.

[fmeum]

This is definitely warranted for non-registry overrides, which are a bit more of an "advanced" concept anyway.

To be clear, is your preference to use labels for non-registry overrides in error messages?

Ideally we would also keep absolute paths for them. But I can see how that would introduce too much complexity (tracking and replacing the output base) for too little gain (non-registry overrides were explicitly added by the user, so they should have a better idea where their module files live). That's why I consider labels a decent tradeoff in this case.

I am actually not sure anymore whether the display form is better for non-root modules: The canonical form has the advantage that it more directly corresponds to the external path on disk, which is ultimately what the user wants to find if they face an error.

I don't really mind it either way. The difference would only be @foo//:MODULE.bazel vs @@foo~override//:MODULE.bazel, which is not a huge deal IMO.

Couldn't it be @my_gazelle//:MODULE.bazel vs @@gazelle~override//:MODULE.bazel? In that case, the former could make it harder to locate the directory in external. But of course it's also more easily recognized by the user, so I'm not completely sure which I like better.

[Wyverald]

Alright. This is holding up 6.3.0, and while I'm not a huge fan of the special de/serialization logic, I don't have a better idea right now. So let's get this submitted (and I'll fast track the label fix for non-registry overrides). Thanks!

[iancha1992]

@bazel-io fork 6.3.0

[iancha1992]

@Wyverald Does this need to be cherry-picked to release-6.3.0?
If so, then how should we go about cherry-picking it because looks like there's a conflict with the "src/test/py/bazel/bzlmod/bazel_lockfile_test.py"

cc: @bazelbuild/triage

[Wyverald]

yes we should cherry-pick it. I'm no longer at work but can send a PR tomorrow morning.