Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MODULE.bazel.lock file contains user specific paths #18936

Closed
jsharpe opened this issue Jul 14, 2023 · 7 comments
Closed

MODULE.bazel.lock file contains user specific paths #18936

jsharpe opened this issue Jul 14, 2023 · 7 comments
Assignees
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug

Comments

@jsharpe
Copy link
Contributor

jsharpe commented Jul 14, 2023

Description of the bug:

The lock file includes user specific paths in the location part of the extension usage sections.
This is a side effect of the change made to improve error messages by @fmeum.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

No response

Which operating system are you running Bazel on?

No response

What is the output of bazel info release?

6.3.0rc1

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

Yes this is a regression due to #18612

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@fmeum
Copy link
Collaborator

fmeum commented Jul 14, 2023

@Wyverald @SalmaSamy The absolute paths come from Location's file field. Do you think we need to keep Location information in the JSON? If it is only used in error messages and the lockfile has been generated, maybe we can just drop that information?

If we want to preserve it, we would probably need to make the path relative to its root. But this doesn't look that easy since the root can vary (workspace root, external repo root, ...).

@Pavank1992 Pavank1992 added the team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. label Jul 14, 2023
@SalmaSamy
Copy link
Contributor

In some cases the location is required in the error messages after reading the data from the lockfile. I am not sure if it's okay to drop it. @Wyverald what do you think?

@Wyverald
Copy link
Member

We shouldn't just completely drop the location, since the lockfile might not contain an extension that 1) hasn't been evaluated yet and 2) throws on evaluation. In this case we will need the location to report the error.

I was thinking that we should maybe just store a label-y thing for locations in MODULE.bazel files of modules with non-registry overrides. So @@foo~override//:MODULE.bazel instead of $OUTPUT_BASE/external/foo~override/MODULE.bazel. (This is the only case where we store a file path for MODULE.bazel IIRC; for modules without non-registry overrides we store the registry URL.) Then we could augment the bazel mod show_repo command a bit to show where the repo is actually located, and maybe also think about some sort of helper command that just straight up converts a label to a local file path. WDYT?

@Wyverald
Copy link
Member

@bazel-io fork 6.3.0

@Wyverald Wyverald added P1 I'll work on this now. (Assignee required) area-Bzlmod Bzlmod-specific PRs, issues, and feature requests labels Jul 14, 2023
@fmeum
Copy link
Collaborator

fmeum commented Jul 14, 2023

I was thinking that we should maybe just store a label-y thing for locations in MODULE.bazel files of modules with non-registry overrides. So @@foo~override//:MODULE.bazel instead of $OUTPUT_BASE/external/foo~override/MODULE.bazel.

Sounds good! If we use Label#getDisplayForm, that label will be reasonably friendly in the likely case where a module with a non-registry override is a direct dependency of the root module.

Then we could augment the bazel mod show_repo command a bit to show where the repo is actually located, and maybe also think about some sort of helper command that just straight up converts a label to a local file path. WDYT?

bazel cquery --output=files comes somewhat close, but doesn't quite get there: It only outputs paths relative to the output base and can't directly refer to MODULE.bazel unless its an exported file. So yes, something like this would be needed, but not urgently.

@fmeum
Copy link
Collaborator

fmeum commented Jul 15, 2023

Having thought about this some more, I like canonical labels more than the display form as they are closer to the actual location on disk.

We should probably also handle the root module specially as it is the one where errors are most likely to originate from. I am working on a PR for this aspect here.

fmeum added a commit to fmeum/bazel that referenced this issue Jul 17, 2023
The absolute paths of the root module file path in Starlark `Location`s
is replaced with a constant when written to the lock file and replaced
back when reading from the file.

Work towards bazelbuild#18936
fmeum added a commit to fmeum/bazel that referenced this issue Jul 17, 2023
The absolute paths of the root module file path in Starlark `Location`s
is replaced with a constant when written to the lock file and replaced
back when reading from the file.

Work towards bazelbuild#18936
fmeum added a commit to fmeum/bazel that referenced this issue Jul 17, 2023
The absolute paths of the root module file path in Starlark `Location`s
is replaced with a constant when written to the lock file and replaced
back when reading from the file.

Work towards bazelbuild#18936
fmeum added a commit to fmeum/bazel that referenced this issue Jul 17, 2023
The absolute paths of the root module file path in Starlark `Location`s
is replaced with a constant when written to the lock file and replaced
back when reading from the file.

Work towards bazelbuild#18936
fmeum added a commit to fmeum/bazel that referenced this issue Jul 17, 2023
The absolute paths of the root module file path in Starlark `Location`s
is replaced with a constant when written to the lock file and replaced
back when reading from the file.

Work towards bazelbuild#18936
fmeum added a commit to fmeum/bazel that referenced this issue Jul 17, 2023
The absolute paths of the root module file path in Starlark `Location`s
is replaced with a constant when written to the lock file and replaced
back when reading from the file.

Work towards bazelbuild#18936
copybara-service bot pushed a commit that referenced this issue Jul 18, 2023
The absolute path of the root module file in Starlark `Location`s is replaced with a constant when writing to the lock file.

Work towards #18936

Closes #18949.

PiperOrigin-RevId: 549118781
Change-Id: Ie689f7b5edf92296772c605845d694d872074214
@Wyverald
Copy link
Member

I have a change in flight to fix non-registry overrides.

Wyverald added a commit that referenced this issue Jul 19, 2023
The absolute path of the root module file in Starlark `Location`s is replaced with a constant when writing to the lock file.

Work towards #18936

Closes #18949.

PiperOrigin-RevId: 549118781
Change-Id: Ie689f7b5edf92296772c605845d694d872074214
Wyverald added a commit that referenced this issue Jul 19, 2023
…overrides

Fixes #18936

PiperOrigin-RevId: 549310245
Change-Id: I852570fbc81c1592c2fc0b3848d4b58e1c9ffb7d
iancha1992 pushed a commit that referenced this issue Jul 19, 2023
* Redact absolute root module file path in `MODULE.bazel.lock`

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

Work towards #18936

Closes #18949.

PiperOrigin-RevId: 549118781
Change-Id: Ie689f7b5edf92296772c605845d694d872074214

* Use a label as the location of the MODULE.bazel file of non-registry overrides

Fixes #18936

PiperOrigin-RevId: 549310245
Change-Id: I852570fbc81c1592c2fc0b3848d4b58e1c9ffb7d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug
Projects
None yet
Development

No branches or pull requests

6 participants