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.

Sign up for GitHub

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

Jump to bottom

[#6664]: Invoke blaze mod correctly #6729

Merged
merged 4 commits into from
Sep 6, 2024
Merged

[#6664]: Invoke blaze mod correctly #6729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
23e08ae
[#6664]: Invoke `blaze mod` correctly
mtoader Sep 5, 2024
7c6e25a
Merge branch 'master' into mtoader/invoke-blaze-mod-correctly
mtoader Sep 5, 2024
22b9a42
Reformat with google format
mtoader Sep 5, 2024
63e3e66
Merge branch 'master' into mtoader/invoke-blaze-mod-correctly
mtoader Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 37 additions & 16 deletions base/src/com/google/idea/blaze/base/command/mod/BlazeModRunnerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,37 @@

public class BlazeModRunnerImpl extends BlazeModRunner {

private static final String DUMP_REPO_MAPPING = "dump_repo_mapping";
private static final String ROOT_WORKSPACE = "";
Copy link
Collaborator

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

Copy link
Collaborator

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?


/**
* {@code bazel mod dump_repo_mapping} takes a canonical repository name and will dump a map from
* repoName -> canonicalName of all the external repositories available to that repository The
* 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
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, ok, thank you!

Copy link
Collaborator

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

* 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 between those versions for the command to work well.
*/
@Override
public ListenableFuture<ExternalWorkspaceData> dumpRepoMapping(
Project project,
BuildSystem.BuildInvoker invoker,
BlazeContext context,
BuildSystemName buildSystemName,
List<String> flags) {

// TODO: when 8.0.0 is released add this only if it's disabled explicitly for the repo
flags.add("--noenable_workspace");

return Futures.transform(
runBlazeModGetBytes(project, invoker, context, ImmutableList.of( "dump_repo_mapping", "workspace"), flags),
runBlazeModGetBytes(
project, invoker, context, ImmutableList.of(DUMP_REPO_MAPPING, ROOT_WORKSPACE), flags),
bytes -> {
JsonObject json = JsonParser.parseString(new String(bytes, StandardCharsets.UTF_8).trim()).getAsJsonObject();
JsonObject json =
JsonParser.parseString(new String(bytes, StandardCharsets.UTF_8).trim())
.getAsJsonObject();

ImmutableList<ExternalWorkspace> externalWorkspaces =
json.entrySet().stream()
Expand All @@ -70,21 +90,22 @@ protected ListenableFuture<byte[]> runBlazeModGetBytes(
List<String> args,
List<String> flags) {
return BlazeExecutor.getInstance()
.submit(() -> {
BlazeCommand.Builder builder =
BlazeCommand.builder(invoker, BlazeCommandName.MOD)
.addBlazeFlags(flags);
.submit(
() -> {
BlazeCommand.Builder builder =
BlazeCommand.builder(invoker, BlazeCommandName.MOD).addBlazeFlags(flags);

if (args != null) {
builder.addBlazeFlags(args);
}
if (args != null) {
builder.addBlazeFlags(args);
}

try (BuildResultHelper buildResultHelper = invoker.createBuildResultHelper()) {
BlazeCommandRunner runner = invoker.getCommandRunner();
try (InputStream stream = runner.runBlazeMod(project, builder, buildResultHelper, context)) {
return stream.readAllBytes();
}
}
});
try (BuildResultHelper buildResultHelper = invoker.createBuildResultHelper()) {
BlazeCommandRunner runner = invoker.getCommandRunner();
try (InputStream stream =
runner.runBlazeMod(project, builder, buildResultHelper, context)) {
return stream.readAllBytes();
}
}
});
}
}
12 changes: 6 additions & 6 deletions base/src/com/google/idea/blaze/base/model/ExternalWorkspaceDataProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,12 @@ public static ExternalWorkspaceDataProvider getInstance(Project project) {
}

static Boolean isEnabled(BlazeVersionData blazeVersionData) {
// disable this until a more reliable opt-in mechanism is chosen.
//
// bg: some blaze workspaces with blaze > MINIMUM_BLAZE_VERSION
// have explicitly disabled this bzlmod support and this causes
// `blaze mod` to fail.
return blazeVersionData.bazelIsAtLeastVersion(MINIMUM_BLAZE_VERSION) && Registry.is("bazel.read.external.workspace.data");
if (!Registry.is("bazel.read.external.workspace.data")) {
logger.info("disabled by registry");
return false;
}

return blazeVersionData.bazelIsAtLeastVersion(MINIMUM_BLAZE_VERSION);
}

public ListenableFuture<ExternalWorkspaceData> getExternalWorkspaceData(
Expand Down
4 changes: 2 additions & 2 deletions base/src/com/google/idea/blaze/base/sync/ProjectStateSyncTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ private ExternalWorkspaceData getExternalWorkspaceData(
SyncMode syncMode)
throws SyncCanceledException, SyncFailedException {

List<String> syncFlags =
List<String> blazeModFlags =
BlazeFlags.blazeFlags(
project,
projectViewSet,
Expand All @@ -243,7 +243,7 @@ private ExternalWorkspaceData getExternalWorkspaceData(

ListenableFuture<ExternalWorkspaceData> externalWorkspaceDataFuture =
ExternalWorkspaceDataProvider.getInstance(project)
.getExternalWorkspaceData(context, syncFlags, blazeVersionData, blazeInfo);
.getExternalWorkspaceData(context, blazeModFlags, blazeVersionData, blazeInfo);

FutureResult<ExternalWorkspaceData> externalWorkspaceDataResult =
FutureUtil.waitForFuture(context, externalWorkspaceDataFuture)
Expand Down
Loading