-
Notifications
You must be signed in to change notification settings - Fork 304
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
[#6664]: Invoke blaze mod
correctly
#6729
[#6664]: Invoke blaze mod
correctly
#6729
Conversation
2c28e4c
to
fb3aeef
Compare
@@ -38,6 +38,9 @@ | |||
|
|||
public class BlazeModRunnerImpl extends BlazeModRunner { | |||
|
|||
private static final String DUMP_REPO_MAPPING = "dump_repo_mapping"; | |||
private static final String ROOT_WORKSPACE = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously it was "workspace", not an empty string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the context there: #6664 (comment)
Could you summarize that in a code comment?
@@ -83,6 +83,9 @@ public ListenableFuture<ExternalWorkspaceData> getExternalWorkspaceData( | |||
return Futures.immediateFuture(ExternalWorkspaceData.EMPTY); | |||
} | |||
|
|||
// TODO: when 8.0.0 is released add this conditionally only when it's disabled inside `starLarkSemantics` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you also explain the need for this flag in a comment?
fb3aeef
to
23e08ae
Compare
@agluszak updated comments and moved the flag patch to the execution place (to be near the comment) |
* repository map we would invoke it like {@code bazel mod dump_repo_mapping ""}. | ||
* <p /> | ||
* Additionally the flag {@code --enable_workspace} needs to be off for this to work. The flag is default | ||
* off in bazel 8.0.0 but it is on between 7.1.0 and 8.0.0. So we need to also pass this along in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this flag supported by Bazel <7.1.0? If not, doesn't it break Bazel 6 and 5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not get called at all before 7.1.0 since the dump command itself didn't exist before.
* name {@code ""} is special and considered to be <em>the main workspace</em> so in order to dump | ||
* the main repository map we would invoke it like {@code bazel mod dump_repo_mapping ""}. | ||
* | ||
* <p>Additionally the flag {@code --enable_workspace} needs to be off for this to work. The flag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtoader do you remember what was the reason for this? Why enable workspace would break the call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It returns broken data.
Basically you get all the dependencies visible from WORKSPACE and modules. Currently I pass it with --noenable_workspace
to disable workspace and just look at the module provided dependencies. This is because the flag default value in 7+ is on.
E.g.: a run with --noenable_workspace
on my test repo (the correct run)
➜ bazel mod --noenable_workspace dump_repo_mapping "" | yq -P | wc -l
10
v.s. a run with --enable_workspace
in the same spot:
➜ bazel mod --enable_workspace dump_repo_mapping "" | yq -P | wc -l
checking cached actions
93
all the extras look like:
remote_jdk8_linux: remote_jdk8_linux
bazel_skylib: bazel_skylib
remotejdk11_linux_toolchain_config_repo: remotejdk11_linux_toolchain_config_repo
remotejdk11_macos: remotejdk11_macos
remotejdk11_win_arm64: remotejdk11_win_arm64
remotejdk21_macos: remotejdk21_macos
rules_rust: rules_rust+
remotejdk17_macos_aarch64_toolchain_config_repo: remotejdk17_macos_aarch64_toolchain_config_repo
remote_jdk8_linux_aarch64: remote_jdk8_linux_aarch64
local_config_cc: local_config_cc
remotejdk11_macos_aarch64_toolchain_config_repo: remotejdk11_macos_aarch64_toolchain_config_repo
remotejdk21_linux: remotejdk21_linux
android_gmaven_r8: android_gmaven_r8
remote_java_tools_linux: remote_java_tools_linux
remotejdk21_win: remotejdk21_win
aka bazel toolchain variants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, ok, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked, because I had to introduce bazel mod deps
call, and it crashes with --noenable_workspace, due to a bug in bazel 7.3. So I was wondering what was the purpose in dump_repo_mapping
Checklist
Discussion thread for this change
Issue number: #6664
Description of this change
Invoke
blaze mod dump_repo_mapping
correctly