From 49287cb4cf497d85f007e1b9860ade8ff80c226f Mon Sep 17 00:00:00 2001 From: David Aurelio Date: Wed, 6 Jun 2018 07:29:33 -0700 Subject: [PATCH] JsLibrary: Don't remove macro deps from build deps Summary: For legacy reasons, we were removing *all* but the worker dependency from build rule params when creating a `JsBundle`, where in reality we only wanted to remove declared deps and query deps. This lead to dependencies coming from `extraJson` being unintentionally removed. Here, we add new tests to guarantee the behavior we want, and only remove js_library dependencies from build rule params. Reviewed By: fromcelticpark fbshipit-source-id: cad3b34 --- .../buck/js/JsLibraryDescription.java | 58 ++++++++++++++----- .../facebook/buck/js/JsLibraryBuilder.java | 6 ++ .../buck/js/JsLibraryDescriptionTest.java | 34 ++++++++++- test/com/facebook/buck/js/JsTestScenario.java | 51 +++++++++++++--- 4 files changed, 126 insertions(+), 23 deletions(-) diff --git a/src/com/facebook/buck/js/JsLibraryDescription.java b/src/com/facebook/buck/js/JsLibraryDescription.java index 128a8b8395b..28cac16df39 100644 --- a/src/com/facebook/buck/js/JsLibraryDescription.java +++ b/src/com/facebook/buck/js/JsLibraryDescription.java @@ -41,8 +41,13 @@ import com.facebook.buck.core.util.immutables.BuckStyleImmutable; import com.facebook.buck.io.file.MorePaths; import com.facebook.buck.io.filesystem.ProjectFilesystem; +import com.facebook.buck.rules.macros.BuildTargetMacro; +import com.facebook.buck.rules.macros.MacroContainer; +import com.facebook.buck.rules.macros.StringWithMacros; +import com.facebook.buck.rules.query.Query; import com.facebook.buck.rules.query.QueryUtils; import com.facebook.buck.shell.WorkerTool; +import com.facebook.buck.util.RichStream; import com.facebook.buck.util.types.Either; import com.facebook.buck.util.types.Pair; import com.google.common.base.Preconditions; @@ -61,6 +66,7 @@ import java.util.Optional; import java.util.concurrent.ExecutionException; import java.util.function.Function; +import java.util.function.Predicate; import java.util.stream.Stream; import javax.annotation.Nullable; import org.immutables.value.Value; @@ -112,14 +118,30 @@ public BuildRule createBuildRule( ProjectFilesystem projectFilesystem = context.getProjectFilesystem(); CellPathResolver cellRoots = context.getCellPathResolver(); - WorkerTool worker = graphBuilder.getRuleWithType(args.getWorker(), WorkerTool.class); + BuildTarget workerTarget = args.getWorker(); + WorkerTool worker = graphBuilder.getRuleWithType(workerTarget, WorkerTool.class); // this params object is used as base for the JsLibrary build rule, but also for all dynamically // created JsFile rules. // For the JsLibrary case, we want to propagate flavors to library dependencies // For the JsFile case, we only want to depend on the worker, not on any libraries + Predicate isWorker = workerTarget::equals; + Predicate extraDepsFilter = + args.getExtraJson() + .map(StringWithMacros::getMacros) + .map(RichStream::from) + .map(JsLibraryDescription::getMacroTargets) + .map(macroTargets -> isWorker.or(macroTargets::contains)) + .orElse(isWorker); + ImmutableSortedSet workerAndMacrosExtraDeps = + params + .getExtraDeps() + .get() + .stream() + .filter(x -> extraDepsFilter.test(x.getBuildTarget())) + .collect(ImmutableSortedSet.toImmutableSortedSet(Ordering.natural())); BuildRuleParams baseParams = - JsUtil.paramsWithDeps(params, graphBuilder.getRule(args.getWorker())); + params.withoutDeclaredDeps().withExtraDeps(workerAndMacrosExtraDeps); if (file.isPresent()) { return buildTarget.getFlavors().contains(JsFlavors.RELEASE) @@ -141,19 +163,17 @@ public BuildRule createBuildRule( .setSources(args.getSrcs()) .build(projectFilesystem, worker); } else { - Stream deps = args.getDeps().stream(); - if (args.getDepsQuery().isPresent()) { - // We allow the `deps_query` to contain different kinds of build targets, but filter out - // all targets that don't refer to a JsLibrary rule. - // That prevents users from having to wrap every query into "kind(js_library, ...)". - Stream jsLibraryTargetsInQuery = - args.getDepsQuery() - .get() - .getResolvedQuery() - .stream() - .filter(target -> JsUtil.isJsLibraryTarget(target, context.getTargetGraph())); - deps = Stream.concat(deps, jsLibraryTargetsInQuery); - } + // We allow the `deps_query` to contain different kinds of build targets, but filter out + // all targets that don't refer to a JsLibrary rule. + // That prevents users from having to wrap every query into "kind(js_library, ...)". + Stream queryDeps = + args.getDepsQuery() + .map(Query::getResolvedQuery) + .orElseGet(ImmutableSortedSet::of) + .stream() + .filter(target -> JsUtil.isJsLibraryTarget(target, context.getTargetGraph())); + Stream declaredDeps = args.getDeps().stream(); + Stream deps = Stream.concat(declaredDeps, queryDeps); return new LibraryBuilder(context.getTargetGraph(), graphBuilder, buildTarget, baseParams) .setLibraryDependencies(deps) .build(projectFilesystem, worker); @@ -405,6 +425,14 @@ private static BuildTarget withFileFlavorOnly(BuildTarget target) { return builder.build(); } + private static ImmutableSet getMacroTargets(RichStream containers) { + return containers + .map(MacroContainer::getMacro) + .filter(BuildTargetMacro.class) + .map(BuildTargetMacro::getTarget) + .collect(ImmutableSet.toImmutableSet()); + } + private static Path changePathPrefix( SourcePath sourcePath, String basePath, diff --git a/test/com/facebook/buck/js/JsLibraryBuilder.java b/test/com/facebook/buck/js/JsLibraryBuilder.java index 8db614267db..2b515b8450b 100644 --- a/test/com/facebook/buck/js/JsLibraryBuilder.java +++ b/test/com/facebook/buck/js/JsLibraryBuilder.java @@ -20,6 +20,7 @@ import com.facebook.buck.core.model.targetgraph.AbstractNodeBuilder; import com.facebook.buck.core.sourcepath.SourcePath; import com.facebook.buck.io.filesystem.ProjectFilesystem; +import com.facebook.buck.rules.macros.StringWithMacros; import com.facebook.buck.rules.query.Query; import com.facebook.buck.test.selectors.Nullable; import com.facebook.buck.util.types.Either; @@ -47,6 +48,11 @@ JsLibraryBuilder setDepsQuery(@Nullable Query query) { return this; } + JsLibraryBuilder setExtraJson(Optional extraJson) { + getArgForPopulating().setExtraJson(extraJson); + return this; + } + JsLibraryBuilder setSrcs(ImmutableSet>> srcs) { getArgForPopulating().setSrcs(srcs); return this; diff --git a/test/com/facebook/buck/js/JsLibraryDescriptionTest.java b/test/com/facebook/buck/js/JsLibraryDescriptionTest.java index 551101c05f9..64e1671285c 100644 --- a/test/com/facebook/buck/js/JsLibraryDescriptionTest.java +++ b/test/com/facebook/buck/js/JsLibraryDescriptionTest.java @@ -40,6 +40,7 @@ import com.facebook.buck.js.JsLibrary.Files; import com.facebook.buck.model.BuildTargetFactory; import com.facebook.buck.rules.FakeSourcePath; +import com.facebook.buck.rules.macros.LocationMacro; import com.facebook.buck.rules.query.Query; import com.facebook.buck.util.RichStream; import com.facebook.buck.util.types.Pair; @@ -369,7 +370,10 @@ public void supportsDepsQuery() { .appleLibraryWithDeps(l, a, c) .bundleWithDeps(x, l) .bundleWithDeps(BuildTargetFactory.newInstance("//query-deps:bundle")) - .library(target, Query.of(String.format("deps(%s)", x))) + .library( + target, + Query.of(String.format("deps(%s)", x)), + FakeSourcePath.of("arbitrary/source")) .build(); TargetNode node = scenario.targetGraph.get(target); @@ -386,6 +390,10 @@ public void supportsDepsQuery() { lib.getLibraryDependencies()); assertThat(deps, everyItem(not(in(internalFileRule(scenario.graphBuilder).getBuildDeps())))); + findJsFileRules(scenario.graphBuilder) + .peek(jsFile -> assertThat(deps, everyItem(not(in(jsFile.getBuildDeps()))))) + .findAny() + .orElseThrow(() -> new IllegalStateException("No JsFile dependencies found for " + target)); } @Test @@ -428,6 +436,30 @@ public void libraryRulesDoesNotDependOnGeneratedSources() { assertThat(generatedSourcesRules, everyItem(not(in(libRule.getBuildDeps())))); } + @Test + public void locationMacrosInExtraJsonAddBuildDeps() { + BuildTarget referencedTarget = BuildTargetFactory.newInstance("//:ref"); + JsTestScenario scenario = + scenarioBuilder + .arbitraryRule(referencedTarget) + .library( + target, + ImmutableList.of(FakeSourcePath.of("a/file"), FakeSourcePath.of("another/file")), + "[\"%s\"]", + LocationMacro.of(referencedTarget)) + .build(); + + BuildRule referenced = scenario.graphBuilder.getRule(referencedTarget); + + assertThat(referenced, in(scenario.graphBuilder.getRule(target).getBuildDeps())); + + RichStream jsFileRules = findJsFileRules(scenario.graphBuilder); + jsFileRules + .peek(jsFile -> assertThat(referenced, in(jsFile.getBuildDeps()))) + .findAny() + .orElseThrow(() -> new IllegalStateException("No JsFile dependencies found for " + target)); + } + private JsTestScenario buildScenario(String basePath, SourcePath source) { return scenarioBuilder.library(target, basePath, source).build(); } diff --git a/test/com/facebook/buck/js/JsTestScenario.java b/test/com/facebook/buck/js/JsTestScenario.java index c1cca22544e..11a95532a65 100644 --- a/test/com/facebook/buck/js/JsTestScenario.java +++ b/test/com/facebook/buck/js/JsTestScenario.java @@ -27,6 +27,9 @@ import com.facebook.buck.io.filesystem.ProjectFilesystem; import com.facebook.buck.model.BuildTargetFactory; import com.facebook.buck.parser.exceptions.NoSuchBuildTargetException; +import com.facebook.buck.rules.macros.Macro; +import com.facebook.buck.rules.macros.StringWithMacros; +import com.facebook.buck.rules.macros.StringWithMacrosUtils; import com.facebook.buck.rules.query.Query; import com.facebook.buck.shell.ExportFileBuilder; import com.facebook.buck.shell.FakeWorkerBuilder; @@ -38,6 +41,7 @@ import com.google.common.collect.Ordering; import java.util.Collection; import java.util.LinkedHashSet; +import java.util.Optional; import java.util.Set; import java.util.function.Function; import java.util.stream.Stream; @@ -119,14 +123,31 @@ public Builder bundle(BuildTarget target, ImmutableSortedSet deps) } public Builder library(BuildTarget target, BuildTarget... libraryDependencies) { - addLibrary(target, null, Stream.of(libraryDependencies), null, Stream.of()); + addLibrary(target, null, Stream.of(libraryDependencies), null, Stream.of(), null); return this; } public Builder library( BuildTarget target, Query libraryDependenciesQuery, BuildTarget... libraryDependencies) { addLibrary( - target, null, Stream.of(libraryDependencies), libraryDependenciesQuery, Stream.of()); + target, + null, + Stream.of(libraryDependencies), + libraryDependenciesQuery, + Stream.of(), + null); + return this; + } + + public Builder library( + BuildTarget target, Query libraryDependenciesQuery, SourcePath... sources) { + addLibrary( + target, + null, + Stream.of(), + libraryDependenciesQuery, + Stream.of(sources).map(Either::ofLeft), + null); return this; } @@ -136,24 +157,38 @@ public Builder library(BuildTarget target, SourcePath first, SourcePath... sourc null, Stream.of(), null, - Stream.concat(Stream.of(first), Stream.of(sources)).map(Either::ofLeft)); + Stream.concat(Stream.of(first), Stream.of(sources)).map(Either::ofLeft), + null); return this; } public Builder library(BuildTarget target, String basePath, SourcePath... sources) { - addLibrary(target, basePath, Stream.of(), null, Stream.of(sources).map(Either::ofLeft)); + addLibrary(target, basePath, Stream.of(), null, Stream.of(sources).map(Either::ofLeft), null); return this; } public Builder library( BuildTarget target, String basePath, Pair... sources) { - addLibrary(target, basePath, Stream.of(), null, Stream.of(sources).map(Either::ofRight)); + addLibrary( + target, basePath, Stream.of(), null, Stream.of(sources).map(Either::ofRight), null); return this; } public Builder library( BuildTarget target, Collection libDeps, Collection sources) { - addLibrary(target, null, libDeps.stream(), null, sources.stream().map(Either::ofLeft)); + addLibrary(target, null, libDeps.stream(), null, sources.stream().map(Either::ofLeft), null); + return this; + } + + public Builder library( + BuildTarget target, Collection sources, String macroFormat, Macro... macros) { + addLibrary( + target, + null, + Stream.of(), + null, + sources.stream().map(Either::ofLeft), + StringWithMacrosUtils.format(macroFormat, macros)); return this; } @@ -167,13 +202,15 @@ private void addLibrary( @Nullable String basePath, Stream libraries, @Nullable Query libraryDependenciesQuery, - Stream>> sources) { + Stream>> sources, + @Nullable StringWithMacros extraJson) { nodes.add( new JsLibraryBuilder(target, filesystem) .setBasePath(basePath) .setDeps( libraries.collect(ImmutableSortedSet.toImmutableSortedSet(Ordering.natural()))) .setDepsQuery(libraryDependenciesQuery) + .setExtraJson(Optional.ofNullable(extraJson)) .setSrcs(sources.collect(ImmutableSet.toImmutableSet())) .setWorker(workerTarget) .build());