Migrate StackedDownloader to use AndroidSdk

Summary: This diff reduces the dependency of `KnownBuildRuleTypes` on `AndroidDirectoryResolver` by migrating `StackedDownloader` to use `AndroidSdk`. Highlights of the changes: - `StackedDownloader` is now accepting `ToolchainProvider` instead of an optional SDK root. - `ToolchainProvider` was extended to provide information about whether a toolchain is present or not. This is used in `StackedDownloader` before accessing Android SDK root. - `AndroidSdk` was extended to provide access to SDK root directory. - `DefaultAndroidToolchainFactory` was changed to avoid creating a toolchain when Android SDK root is not present. - `ToolchainProvider` is now passed in `CommandRunnerParams`. Test Plan: CI Reviewed By: ttsugriy fbshipit-source-id: 51e524b
fbauzac · Oct 3, 2017 · 989767e · 989767e
1 parent b7370b2
commit 989767e
Show file tree

Hide file tree

Showing 22 changed files with 121 additions and 60 deletions.
diff --git a/src/com/facebook/buck/android/toolchain/AndroidSdk.java b/src/com/facebook/buck/android/toolchain/AndroidSdk.java
@@ -16,5 +16,9 @@
 
 package com.facebook.buck.android.toolchain;
 
+import java.nio.file.Path;
+
 /** Part of Android toolchain that provides access to Android SDK */
-public interface AndroidSdk {}
+public interface AndroidSdk {
+  Path getSdkRootPath();
+}
diff --git a/src/com/facebook/buck/android/toolchain/impl/BUCK b/src/com/facebook/buck/android/toolchain/impl/BUCK
@@ -2,6 +2,7 @@ java_library(
     name = "impl",
     srcs = glob(["*.java"]),
     exported_deps = [
+        "//src/com/facebook/buck/android:utils",
         "//src/com/facebook/buck/android/toolchain:toolchain",
     ],
     visibility = [
@@ -10,7 +11,6 @@ java_library(
     deps = [
         "//src/com/facebook/buck/android:config",
         "//src/com/facebook/buck/android:steps",
-        "//src/com/facebook/buck/android:utils",
         "//src/com/facebook/buck/android/toolchain:toolchain",
         "//src/com/facebook/buck/config:config",
         "//src/com/facebook/buck/cxx/toolchain:toolchain",

diff --git a/src/com/facebook/buck/android/toolchain/impl/DefaultAndroidSdk.java b/src/com/facebook/buck/android/toolchain/impl/DefaultAndroidSdk.java
@@ -16,6 +16,19 @@
 
 package com.facebook.buck.android.toolchain.impl;
 
+import com.facebook.buck.android.AndroidDirectoryResolver;
 import com.facebook.buck.android.toolchain.AndroidSdk;
+import java.nio.file.Path;
 
-public class DefaultAndroidSdk implements AndroidSdk {}
+public class DefaultAndroidSdk implements AndroidSdk {
+  private final AndroidDirectoryResolver androidDirectoryResolver;
+
+  public DefaultAndroidSdk(AndroidDirectoryResolver androidDirectoryResolver) {
+    this.androidDirectoryResolver = androidDirectoryResolver;
+  }
+
+  @Override
+  public Path getSdkRootPath() {
+    return androidDirectoryResolver.getSdkOrThrow();
+  }
+}
diff --git a/src/com/facebook/buck/android/toolchain/impl/DefaultAndroidToolchainFactory.java b/src/com/facebook/buck/android/toolchain/impl/DefaultAndroidToolchainFactory.java
@@ -19,8 +19,6 @@
 import com.facebook.buck.android.AndroidBuckConfig;
 import com.facebook.buck.android.AndroidDirectoryResolver;
 import com.facebook.buck.android.DefaultAndroidDirectoryResolver;
-import com.facebook.buck.android.toolchain.AndroidNdk;
-import com.facebook.buck.android.toolchain.AndroidSdk;
 import com.facebook.buck.android.toolchain.AndroidToolchain;
 import com.facebook.buck.config.BuckConfig;
 import com.facebook.buck.io.filesystem.ProjectFilesystem;
@@ -31,15 +29,7 @@
 
 public class DefaultAndroidToolchainFactory implements ToolchainFactory<AndroidToolchain> {
   @Override
-  public AndroidToolchain createToolchain(
-      ImmutableMap<String, String> environment,
-      BuckConfig buckConfig,
-      ProjectFilesystem filesystem) {
-    return new DefaultAndroidToolchain(
-        createAndroidSdk(), createAndroidNdk(environment, buckConfig, filesystem));
-  }
-
-  private static Optional<AndroidNdk> createAndroidNdk(
+  public Optional<AndroidToolchain> createToolchain(
       ImmutableMap<String, String> environment,
       BuckConfig buckConfig,
       ProjectFilesystem filesystem) {
@@ -52,10 +42,14 @@ private static Optional<AndroidNdk> createAndroidNdk(
             androidBuckConfig.getBuildToolsVersion(),
             androidBuckConfig.getNdkVersion());
 
-    return Optional.of(new DefaultAndroidNdk(androidBuckConfig, androidDirectoryResolver));
-  }
+    if (androidDirectoryResolver.getSdkOrAbsent().isPresent()) {
+      return Optional.of(
+          new DefaultAndroidToolchain(
+              new DefaultAndroidSdk(androidDirectoryResolver),
+              Optional.of(new DefaultAndroidNdk(androidBuckConfig, androidDirectoryResolver))));
 
-  private static AndroidSdk createAndroidSdk() {
-    return new DefaultAndroidSdk();
+    } else {
+      return Optional.empty();
+    }
   }
 }
diff --git a/src/com/facebook/buck/cli/AbstractCommandRunnerParams.java b/src/com/facebook/buck/cli/AbstractCommandRunnerParams.java
@@ -34,6 +34,7 @@
 import com.facebook.buck.rules.keys.RuleKeyCacheRecycler;
 import com.facebook.buck.step.ExecutorPool;
 import com.facebook.buck.timing.Clock;
+import com.facebook.buck.toolchain.ToolchainProvider;
 import com.facebook.buck.util.Console;
 import com.facebook.buck.util.ProcessManager;
 import com.facebook.buck.util.cache.impl.StackedFileHashCache;
@@ -113,4 +114,6 @@ public interface AbstractCommandRunnerParams {
   Optional<RuleKeyCacheRecycler<RuleKey>> getDefaultRuleKeyFactoryCacheRecycler();
 
   ProjectFilesystemFactory getProjectFilesystemFactory();
+
+  ToolchainProvider getToolchainProvider();
 }
diff --git a/src/com/facebook/buck/cli/FetchCommand.java b/src/com/facebook/buck/cli/FetchCommand.java
@@ -16,7 +16,6 @@
 
 package com.facebook.buck.cli;
 
-import com.facebook.buck.android.DefaultAndroidDirectoryResolver;
 import com.facebook.buck.command.Build;
 import com.facebook.buck.event.ConsoleEvent;
 import com.facebook.buck.file.Downloader;
@@ -44,8 +43,6 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
 import java.io.IOException;
-import java.nio.file.Path;
-import java.util.Optional;
 
 public class FetchCommand extends BuildCommand {
 
@@ -166,16 +163,8 @@ public boolean isReadOnly() {
   }
 
   private FetchTargetNodeToBuildRuleTransformer createFetchTransformer(CommandRunnerParams params) {
-    DefaultAndroidDirectoryResolver resolver =
-        new DefaultAndroidDirectoryResolver(
-            params.getCell().getRoot().getFileSystem(),
-            params.getEnvironment(),
-            Optional.empty(),
-            Optional.empty());
-
-    Optional<Path> sdkDir = resolver.getSdkOrAbsent();
-
-    Downloader downloader = StackedDownloader.createFromConfig(params.getBuckConfig(), sdkDir);
+    Downloader downloader =
+        StackedDownloader.createFromConfig(params.getBuckConfig(), params.getToolchainProvider());
     Description<?> description = new RemoteFileDescription(downloader);
     return new FetchTargetNodeToBuildRuleTransformer(ImmutableSet.of(description));
   }

diff --git a/src/com/facebook/buck/cli/Main.java b/src/com/facebook/buck/cli/Main.java
@@ -1085,6 +1085,7 @@ public int runMainWithExitCode(
                         .setDefaultRuleKeyFactoryCacheRecycler(defaultRuleKeyFactoryCacheRecycler)
                         .setBuildInfoStoreManager(storeManager)
                         .setProjectFilesystemFactory(projectFilesystemFactory)
+                        .setToolchainProvider(factory.getToolchainProvider())
                         .build());
           } catch (InterruptedException | ClosedByInterruptException e) {
             buildEventBus.post(CommandEvent.interrupted(startedEvent, INTERRUPTED_EXIT_CODE));

diff --git a/src/com/facebook/buck/file/BUCK b/src/com/facebook/buck/file/BUCK
@@ -15,10 +15,12 @@ java_library(
     ],
     visibility = ["PUBLIC"],
     deps = [
+        "//src/com/facebook/buck/android/toolchain:toolchain",
         "//src/com/facebook/buck/config:config",
         "//src/com/facebook/buck/event:interfaces",
         "//src/com/facebook/buck/event/external:external_lib",
         "//src/com/facebook/buck/log:api",
+        "//src/com/facebook/buck/toolchain:toolchain",
         "//src/com/facebook/buck/util:exceptions",
         "//third-party/java/guava:guava",
     ],

diff --git a/src/com/facebook/buck/file/StackedDownloader.java b/src/com/facebook/buck/file/StackedDownloader.java
@@ -16,10 +16,12 @@
 
 package com.facebook.buck.file;
 
+import com.facebook.buck.android.toolchain.AndroidToolchain;
 import com.facebook.buck.config.BuckConfig;
 import com.facebook.buck.config.DownloadConfig;
 import com.facebook.buck.event.BuckEventBus;
 import com.facebook.buck.log.Logger;
+import com.facebook.buck.toolchain.ToolchainProvider;
 import com.facebook.buck.util.HumanReadableException;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
@@ -51,7 +53,8 @@ public class StackedDownloader implements Downloader {
     this.delegates = delegates;
   }
 
-  public static Downloader createFromConfig(BuckConfig config, Optional<Path> androidSdkRoot) {
+  public static Downloader createFromConfig(
+      BuckConfig config, ToolchainProvider toolchainProvider) {
     ImmutableList.Builder<Downloader> downloaders = ImmutableList.builder();
 
     DownloadConfig downloadConfig = new DownloadConfig(config);
@@ -103,15 +106,18 @@ public static Downloader createFromConfig(BuckConfig config, Optional<Path> andr
       }
     }
 
-    if (androidSdkRoot.isPresent()) {
-      Path androidMavenRepo = androidSdkRoot.get().resolve("extras/android/m2repository");
+    if (toolchainProvider.isToolchainPresent(AndroidToolchain.DEFAULT_NAME)) {
+      AndroidToolchain androidToolchain =
+          toolchainProvider.getByName(AndroidToolchain.DEFAULT_NAME, AndroidToolchain.class);
+      Path androidSdkRootPath = androidToolchain.getAndroidSdk().getSdkRootPath();
+      Path androidMavenRepo = androidSdkRootPath.resolve("extras/android/m2repository");
       try {
         downloaders.add(new OnDiskMavenDownloader(androidMavenRepo));
       } catch (FileNotFoundException e) {
         LOG.warn("Android Maven repo %s doesn't exist", androidMavenRepo.toString());
       }
 
-      Path googleMavenRepo = androidSdkRoot.get().resolve("extras/google/m2repository");
+      Path googleMavenRepo = androidSdkRootPath.resolve("extras/google/m2repository");
       try {
         downloaders.add(new OnDiskMavenDownloader(googleMavenRepo));
       } catch (FileNotFoundException e) {

diff --git a/src/com/facebook/buck/rules/KnownBuildRuleTypes.java b/src/com/facebook/buck/rules/KnownBuildRuleTypes.java
@@ -318,8 +318,7 @@ static KnownBuildRuleTypes createInstance(
     Downloader downloader;
     DownloadConfig downloadConfig = new DownloadConfig(config);
     if (downloadConfig.isDownloadAtRuntimeOk()) {
-      downloader =
-          StackedDownloader.createFromConfig(config, androidDirectoryResolver.getSdkOrAbsent());
+      downloader = StackedDownloader.createFromConfig(config, toolchainProvider);
     } else {
       // Or just set one that blows up
       downloader = new ExplodingDownloader();

diff --git a/src/com/facebook/buck/rules/KnownBuildRuleTypesFactory.java b/src/com/facebook/buck/rules/KnownBuildRuleTypesFactory.java
@@ -50,4 +50,8 @@ public KnownBuildRuleTypes create(BuckConfig config, ProjectFilesystem filesyste
     return KnownBuildRuleTypes.createInstance(
         config, filesystem, executor, toolchainProvider, directoryResolver, sdkEnvironment);
   }
+
+  public ToolchainProvider getToolchainProvider() {
+    return toolchainProvider;
+  }
 }
diff --git a/src/com/facebook/buck/toolchain/ToolchainFactory.java b/src/com/facebook/buck/toolchain/ToolchainFactory.java
@@ -19,9 +19,10 @@
 import com.facebook.buck.config.BuckConfig;
 import com.facebook.buck.io.filesystem.ProjectFilesystem;
 import com.google.common.collect.ImmutableMap;
+import java.util.Optional;
 
 public interface ToolchainFactory<T extends Toolchain> {
-  T createToolchain(
+  Optional<T> createToolchain(
       ImmutableMap<String, String> environment,
       BuckConfig buckConfig,
       ProjectFilesystem filesystem);

diff --git a/src/com/facebook/buck/toolchain/ToolchainProvider.java b/src/com/facebook/buck/toolchain/ToolchainProvider.java
@@ -21,4 +21,6 @@ public interface ToolchainProvider {
   Toolchain getByName(String toolchainName);
 
   <T extends Toolchain> T getByName(String toolchainName, Class<T> toolchainClass);
+
+  boolean isToolchainPresent(String toolchainName);
 }
diff --git a/src/com/facebook/buck/toolchain/impl/BUCK b/src/com/facebook/buck/toolchain/impl/BUCK
@@ -8,5 +8,6 @@ java_library(
     deps = [
         "//src/com/facebook/buck/android/toolchain/impl:impl",
         "//src/com/facebook/buck/io:io",
+        "//src/com/facebook/buck/util:exceptions",
     ],
 )
diff --git a/src/com/facebook/buck/toolchain/impl/DefaultToolchainProvider.java b/src/com/facebook/buck/toolchain/impl/DefaultToolchainProvider.java
@@ -23,10 +23,12 @@
 import com.facebook.buck.toolchain.BaseToolchainProvider;
 import com.facebook.buck.toolchain.Toolchain;
 import com.facebook.buck.toolchain.ToolchainFactory;
+import com.facebook.buck.util.HumanReadableException;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableMap;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Optional;
 
 public class DefaultToolchainProvider extends BaseToolchainProvider {
 
@@ -47,7 +49,7 @@ enum ToolchainDescriptor {
   private final ProjectFilesystem projectFilesystem;
   private final ImmutableMap<String, Class<? extends ToolchainFactory<?>>> toolchainFactories;
 
-  private final Map<String, Toolchain> toolchains = new HashMap<>();
+  private final Map<String, Optional<? extends Toolchain>> toolchains = new HashMap<>();
 
   public DefaultToolchainProvider(
       ImmutableMap<String, String> environment,
@@ -68,22 +70,37 @@ public DefaultToolchainProvider(
 
   @Override
   public synchronized Toolchain getByName(String toolchainName) {
-    if (toolchains.containsKey(toolchainName)) {
-      return toolchains.get(toolchainName);
+    Optional<? extends Toolchain> toolchain = getOrCreate(toolchainName);
+    if (toolchain.isPresent()) {
+      return toolchain.get();
+    } else {
+      throw new HumanReadableException("Unknown toolchain: " + toolchainName);
     }
+  }
 
-    if (!toolchainFactories.containsKey(toolchainName)) {
-      throw new IllegalStateException("Unknown toolchain: " + toolchainName);
-    }
-    Class<? extends ToolchainFactory<?>> toolchainFactoryClass =
-        toolchainFactories.get(toolchainName);
-    Toolchain toolchain = createToolchain(toolchainFactoryClass);
-    toolchains.put(toolchainName, toolchain);
+  @Override
+  public boolean isToolchainPresent(String toolchainName) {
+    return toolchainFactories.containsKey(toolchainName) && getOrCreate(toolchainName).isPresent();
+  }
 
+  private Optional<? extends Toolchain> getOrCreate(String toolchainName) {
+    Optional<? extends Toolchain> toolchain;
+    if (!toolchains.containsKey(toolchainName)) {
+      if (!toolchainFactories.containsKey(toolchainName)) {
+        throw new IllegalStateException("Unknown toolchain: " + toolchainName);
+      }
+      Class<? extends ToolchainFactory<?>> toolchainFactoryClass =
+          toolchainFactories.get(toolchainName);
+      toolchain = createToolchain(toolchainFactoryClass);
+      toolchains.put(toolchainName, toolchain);
+    } else {
+      toolchain = toolchains.get(toolchainName);
+    }
     return toolchain;
   }
 
-  private Toolchain createToolchain(Class<? extends ToolchainFactory<?>> toolchainFactoryClass) {
+  private Optional<? extends Toolchain> createToolchain(
+      Class<? extends ToolchainFactory<?>> toolchainFactoryClass) {
     ToolchainFactory<?> toolchainFactory;
     try {
       toolchainFactory = toolchainFactoryClass.newInstance();

diff --git a/test/com/facebook/buck/android/toolchain/BUCK b/test/com/facebook/buck/android/toolchain/BUCK
@@ -11,5 +11,6 @@ java_library(
         "PUBLIC",
     ],
     deps = [
+        "//test/com/facebook/buck/android:utils",
     ],
 )
diff --git a/test/com/facebook/buck/android/toolchain/TestAndroidToolchain.java b/test/com/facebook/buck/android/toolchain/TestAndroidToolchain.java
@@ -16,13 +16,23 @@
 
 package com.facebook.buck.android.toolchain;
 
+import com.facebook.buck.android.FakeAndroidDirectoryResolver;
 import com.facebook.buck.android.toolchain.impl.DefaultAndroidSdk;
 import com.facebook.buck.android.toolchain.impl.DefaultAndroidToolchain;
+import java.nio.file.Path;
 import java.util.Optional;
 
 public class TestAndroidToolchain extends DefaultAndroidToolchain {
 
   public TestAndroidToolchain() {
-    super(new DefaultAndroidSdk(), Optional.empty());
+    super(new DefaultAndroidSdk(new FakeAndroidDirectoryResolver()), Optional.empty());
+  }
+
+  public TestAndroidToolchain(Path androidSdkRoot) {
+    super(
+        new DefaultAndroidSdk(
+            new FakeAndroidDirectoryResolver(
+                Optional.of(androidSdkRoot), Optional.empty(), Optional.empty(), Optional.empty())),
+        Optional.empty());
   }
 }
diff --git a/test/com/facebook/buck/cli/CleanCommandTest.java b/test/com/facebook/buck/cli/CleanCommandTest.java
@@ -174,6 +174,7 @@ private CommandRunnerParams createCommandRunnerParams(BuckConfig buckConfig, Cel
                 processExecutor, androidDirectoryResolver, sdkEnvironment, toolchainProvider))
         .setSdkEnvironment(sdkEnvironment)
         .setProjectFilesystemFactory(new DefaultProjectFilesystemFactory())
+        .setToolchainProvider(toolchainProvider)
         .build();
   }
 }
diff --git a/test/com/facebook/buck/cli/CommandRunnerParamsForTesting.java b/test/com/facebook/buck/cli/CommandRunnerParamsForTesting.java
@@ -143,6 +143,7 @@ public static CommandRunnerParams createCommandRunnerParamsForTesting(
                 processExecutor, androidDirectoryResolver, sdkEnvironment, toolchainProvider))
         .setSdkEnvironment(sdkEnvironment)
         .setProjectFilesystemFactory(new DefaultProjectFilesystemFactory())
+        .setToolchainProvider(toolchainProvider)
         .build();
   }
 

diff --git a/test/com/facebook/buck/file/BUCK b/test/com/facebook/buck/file/BUCK
@@ -241,6 +241,7 @@ standard_java_test(
         "//src/com/facebook/buck/worker:worker_job_params",
         "//src/com/facebook/buck/worker:worker_pool_factory",
         "//src/com/facebook/buck/worker:worker_process",
+        "//test/com/facebook/buck/android/toolchain:testutil",
         "//test/com/facebook/buck/config:FakeBuckConfig",
         "//test/com/facebook/buck/event:testutil",
         "//test/com/facebook/buck/file:testutil",
@@ -251,6 +252,7 @@ standard_java_test(
         "//test/com/facebook/buck/testutil:testutil",
         "//test/com/facebook/buck/testutil/integration:util",
         "//test/com/facebook/buck/timing:testutil",
+        "//test/com/facebook/buck/toolchain/impl:testutil",
         "//third-party/java/aether:aether-api",
         "//third-party/java/android:ddmlib",
         "//third-party/java/android:tools-sdk-common",