From 606f6d55775df2345152cc590e8f914cb02f00a7 Mon Sep 17 00:00:00 2001 From: Ruslan Sayfutdinov Date: Fri, 3 Dec 2021 10:32:47 -0800 Subject: [PATCH] Back out "Reduce Rust compile rule path length" Summary: Original commit changeset: 8ef9cf0fdeef Reverts D31888529. It breaks some tests (T107229114) and makes compilation error messages less helpful: https://fb.workplace.com/groups/rust.language/posts/7389150347800183/ Rust 1.58 has support for long paths on Windows which should solve original issue. https://github.com/rust-lang/rust/pull/89174 It's currently in beta, stable release will happen on Jan 13: https://forge.rust-lang.org/ But I think we can use beta version for some time on Windows. Reviewed By: jsgf fbshipit-source-id: 9af1a5631ab56b8bb285da7cc3044eb8ba1bde85 --- .../buck/features/rust/RustCompileRule.java | 26 +++-- .../buck/features/rust/RustCompileUtils.java | 97 +++++++++---------- .../features/rust/RustLibraryDescription.java | 2 +- .../buck/features/rust/RustCompileTest.java | 9 +- 4 files changed, 65 insertions(+), 69 deletions(-) diff --git a/src/com/facebook/buck/features/rust/RustCompileRule.java b/src/com/facebook/buck/features/rust/RustCompileRule.java index c9d201f47d8..24ad715a651 100644 --- a/src/com/facebook/buck/features/rust/RustCompileRule.java +++ b/src/com/facebook/buck/features/rust/RustCompileRule.java @@ -54,7 +54,6 @@ import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.Maps; import java.nio.file.Path; -import java.nio.file.Paths; import java.util.Optional; /** Generate a rustc command line with all appropriate dependencies in place. */ @@ -91,12 +90,11 @@ protected RustCompileRule( ImmutableList depArgs, ImmutableList linkerArgs, ImmutableSortedMap environment, - ImmutableSortedMap mappedSources, + ImmutableSortedMap> mappedSources, String rootModule, RemapSrcPaths remapSrcPaths, Optional xcrunSdkPath, boolean withDownwardApi) { - super( buildTarget, projectFilesystem, @@ -130,7 +128,7 @@ public static RustCompileRule from( ImmutableList depArgs, ImmutableList linkerArgs, ImmutableSortedMap environment, - ImmutableSortedMap mappedSources, + ImmutableSortedMap> mappedSources, String rootModule, RemapSrcPaths remapSrcPaths, Optional xcrunSdkPath, @@ -186,7 +184,7 @@ static class Impl implements Buildable { @AddToRuleKey private final String rootModule; @AddToRuleKey private final OutputPath output; - @AddToRuleKey private final ImmutableSortedMap mappedSources; + @AddToRuleKey private final ImmutableSortedMap> mappedSources; @AddToRuleKey private final RustBuckConfig.RemapSrcPaths remapSrcPaths; @@ -208,7 +206,7 @@ public Impl( ImmutableSortedMap environment, String rootModule, String outputName, - ImmutableSortedMap mappedSources, + ImmutableSortedMap> mappedSources, RemapSrcPaths remapSrcPaths, Optional xcrunpath, boolean withDownwardApi) { @@ -255,7 +253,20 @@ public ImmutableList getBuildSteps( mappedSources.entrySet().stream() .collect( ImmutableMap.toImmutableMap( - ent -> Paths.get(ent.getValue()), + ent -> { + Path path; + if (ent.getValue().isPresent()) { + path = + buildTarget + .getCellRelativeBasePath() + .getPath() + .toPath(filesystem.getFileSystem()) + .resolve(ent.getValue().get()); + } else { + path = resolver.getCellUnsafeRelPath(ent.getKey()).getPath(); + } + return path; + }, ent -> resolver.getAbsolutePath(ent.getKey()).getPath())))); steps.addAll( CxxPrepareForLinkStep.create( @@ -296,7 +307,6 @@ protected ImmutableList getShellCommandInternal( ImmutableList linkerCmd = linker.getCommandPrefix(resolver); ImmutableList.Builder cmd = ImmutableList.builder(); Path src = scratchDir.resolve(rootModule); - cmd.addAll(compiler.getCommandPrefix(resolver)); if (executionContext.getAnsi().isAnsiTerminal()) { cmd.add("--color=always"); diff --git a/src/com/facebook/buck/features/rust/RustCompileUtils.java b/src/com/facebook/buck/features/rust/RustCompileUtils.java index 802e717f62e..8f2072a2980 100644 --- a/src/com/facebook/buck/features/rust/RustCompileUtils.java +++ b/src/com/facebook/buck/features/rust/RustCompileUtils.java @@ -82,7 +82,6 @@ import com.google.common.hash.Hashing; import java.nio.charset.StandardCharsets; import java.nio.file.Path; -import java.nio.file.Paths; import java.util.Collection; import java.util.List; import java.util.Map; @@ -112,7 +111,7 @@ public static RustCompileRule requireBuild( CrateType crateType, Optional edition, LinkableDepType depType, - ImmutableSortedMap mappedSources, + ImmutableSortedMap> mappedSources, String rootModule, boolean forceRlib, boolean preferStatic, @@ -246,7 +245,7 @@ private static RustCompileRule createBuild( Optional edition, LinkableDepType depType, boolean rpath, - ImmutableSortedMap mappedSources, + ImmutableSortedMap> mappedSources, String rootModule, boolean forceRlib, boolean preferStatic, @@ -642,7 +641,7 @@ public static BinaryWrapperRule createBinaryBuildRule( CxxPlatform cxxPlatform = rustPlatform.getCxxPlatform(); - Pair> rootModuleAndSources = + Pair>> rootModuleAndSources = getRootModuleAndSources( projectFilesystem, buildTarget, @@ -830,15 +829,16 @@ public static Optional getCrateRoot( SourcePathResolverAdapter resolver, String crate, ImmutableSet defaults, - Stream sources) { + Stream sources) { String crateName = String.format("%s.rs", crate); ImmutableList res = sources .filter( src -> { - String name = Paths.get(src).getFileName().toString(); + String name = src.getFileName().toString(); return defaults.contains(name) || name.equals(crateName); }) + .map(src -> src.toString()) .collect(ImmutableList.toImmutableList()); if (res.size() == 1) { @@ -854,7 +854,7 @@ public static Optional getCrateRoot( * module is what's passed to rustc as the input source file, and may not exist until the * symlinking phase happens. */ - static Pair> getRootModuleAndSources( + static Pair>> getRootModuleAndSources( ProjectFilesystem projectFilesystem, BuildTarget target, ActionGraphBuilder graphBuilder, @@ -865,64 +865,55 @@ static Pair> getRootModuleAndSour ImmutableSortedSet srcs, ImmutableSortedMap mappedSrcs) { - ImmutableSortedMap.Builder fixedBuilder = ImmutableSortedMap.naturalOrder(); - - Path buildTargetPath = - target.getCellRelativeBasePath().getPath().toPath(projectFilesystem.getFileSystem()); - - SourcePathResolverAdapter resolver = graphBuilder.getSourcePathResolver(); + ImmutableSortedMap.Builder> fixedBuilder = + ImmutableSortedMap.naturalOrder(); srcs.stream() .map( src -> CxxGenruleDescription.fixupSourcePath(graphBuilder, cxxPlatform.getFlavor(), src)) - .forEach( - src -> { - SourcePath srcPath = - CxxGenruleDescription.fixupSourcePath(graphBuilder, cxxPlatform.getFlavor(), src); - Path candidatePath = resolver.getCellUnsafeRelPath(src).getPath(); - - // These are files with no user-defined virtual path, they are either - // generated (ie with a genrule) or references files in the source - // tree - - // If the path references files in the source tree - if (candidatePath.startsWith(buildTargetPath)) { - // We relativize them to reduce path amplification - mostly for - // windows's sake - fixedBuilder.put(srcPath, buildTargetPath.relativize(candidatePath).toString()); - } else { - - // These are generated files, since these are generated paths with hashes, they are - // rewritable. - fixedBuilder.put( - srcPath, resolver.getAbsolutePath(srcPath).getPath().getFileName().toString()); - } - }); + .forEach(src -> fixedBuilder.put(src, Optional.empty())); mappedSrcs .entrySet() .forEach( - ent -> { - SourcePath sourcePath = - CxxGenruleDescription.fixupSourcePath( - graphBuilder, cxxPlatform.getFlavor(), ent.getKey()); - Path virtualPath = Paths.get(ent.getValue()).normalize(); - // These are rs files with user-defined virtual path, usually these are - // the from mapped_srcs - // Ban remappings that contain paths that go outside the base path - if (virtualPath.startsWith("..")) { - throw new HumanReadableException( - "Mapped sources must be forward paths ie. no ../ "); - } - fixedBuilder.put(sourcePath, virtualPath.toString()); - }); - - ImmutableSortedMap fixed = fixedBuilder.build(); + ent -> + fixedBuilder.put( + CxxGenruleDescription.fixupSourcePath( + graphBuilder, cxxPlatform.getFlavor(), ent.getKey()), + Optional.of(ent.getValue()))); + + ImmutableSortedMap> fixed = fixedBuilder.build(); + + SourcePathResolverAdapter resolver = graphBuilder.getSourcePathResolver(); + Stream filenames = + Stream.concat( + srcs.stream() + .map( + src -> + CxxGenruleDescription.fixupSourcePath( + graphBuilder, cxxPlatform.getFlavor(), src)) + .map(sp -> resolver.getCellUnsafeRelPath(sp).getPath()), + mappedSrcs.values().stream() + .map( + path -> + target + .getCellRelativeBasePath() + .getPath() + .toPath(projectFilesystem.getFileSystem()) + .resolve(path))); Optional rootModule = crateRoot + .map( + name -> + target + .getCellRelativeBasePath() + .getPath() + .toPath(projectFilesystem.getFileSystem()) + .resolve(name) + .toString()) .map(Optional::of) - .orElseGet(() -> getCrateRoot(resolver, crate, defaultRoots, fixed.values().stream())); + .orElseGet(() -> getCrateRoot(resolver, crate, defaultRoots, filenames)); return new Pair<>( rootModule.orElseThrow( diff --git a/src/com/facebook/buck/features/rust/RustLibraryDescription.java b/src/com/facebook/buck/features/rust/RustLibraryDescription.java index 54f7aee3588..3812546ba65 100644 --- a/src/com/facebook/buck/features/rust/RustLibraryDescription.java +++ b/src/com/facebook/buck/features/rust/RustLibraryDescription.java @@ -115,7 +115,7 @@ private RustCompileRule requireLibraryBuild( RustLibraryDescriptionArg args, Iterable deps, ImmutableMap depsAliases) { - Pair> rootModuleAndSources = + Pair>> rootModuleAndSources = RustCompileUtils.getRootModuleAndSources( projectFilesystem, buildTarget, diff --git a/test/com/facebook/buck/features/rust/RustCompileTest.java b/test/com/facebook/buck/features/rust/RustCompileTest.java index 6fa075dc6cc..05b1a6e4aef 100644 --- a/test/com/facebook/buck/features/rust/RustCompileTest.java +++ b/test/com/facebook/buck/features/rust/RustCompileTest.java @@ -289,7 +289,7 @@ private FakeRustCompileRule( srcs.stream() .collect( ImmutableSortedMap.toImmutableSortedMap( - Comparator.naturalOrder(), src -> src, src -> "random")), + Comparator.naturalOrder(), src -> src, src -> Optional.empty())), rootModule, RustBuckConfig.RemapSrcPaths.NO, Optional.empty(), @@ -308,12 +308,7 @@ static FakeRustCompileRule from(String target, ImmutableSortedSet sr ImmutableSet.of("main.rs", "lib.rs"), srcs.stream() .map( - sp -> - ruleFinder - .getSourcePathResolver() - .getCellUnsafeRelPath(sp) - .getPath() - .toString())); + sp -> ruleFinder.getSourcePathResolver().getCellUnsafeRelPath(sp).getPath())); if (!root.isPresent()) { throw new HumanReadableException("No crate root source identified");