Implement --attempt_to_print_relative_paths in the old ui. There's no good reason to not have this feature in the old ui. The old flag name --experimental_ui_attempt_to_print_relative_paths will still work, and will now work for the old ui too. We will remove this old flag name in the future. RELNOTES: None PiperOrigin-RevId: 223351802
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java index 1c52c7d..e4bf647 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
@@ -44,6 +44,7 @@ import com.google.devtools.build.lib.util.io.DelegatingOutErr; import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.common.options.OpaqueOptionsData; import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.OptionsParsingException; @@ -607,19 +608,18 @@ /** Returns the event handler to use for this Blaze command. */ private EventHandler createEventHandler( OutErr outErr, BlazeCommandEventHandler.Options eventOptions) { + Path workspacePath = runtime.getWorkspace().getDirectories().getWorkspace(); + PathFragment workspacePathFragment = workspacePath == null ? null : workspacePath.asFragment(); EventHandler eventHandler; if (eventOptions.experimentalUi) { // The experimental event handler is not to be rate limited, so don't wrap it in a // RateLimitingEventHandler. return new ExperimentalEventHandler( - outErr, - eventOptions, - runtime.getClock(), - runtime.getWorkspace().getDirectories().getWorkspace()); + outErr, eventOptions, runtime.getClock(), workspacePathFragment); } else if ((eventOptions.useColor() || eventOptions.useCursorControl())) { - eventHandler = new FancyTerminalEventHandler(outErr, eventOptions); + eventHandler = new FancyTerminalEventHandler(outErr, eventOptions, workspacePathFragment); } else { - eventHandler = new BlazeCommandEventHandler(outErr, eventOptions); + eventHandler = new BlazeCommandEventHandler(outErr, eventOptions, workspacePathFragment); } return RateLimitingEventHandler.create(eventHandler, eventOptions.showProgressRateLimit);
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandEventHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandEventHandler.java index ce7369e..1e6b033 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandEventHandler.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandEventHandler.java
@@ -13,11 +13,14 @@ // limitations under the License. package com.google.devtools.build.lib.runtime; +import com.google.common.eventbus.Subscribe; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.events.EventKind; import com.google.devtools.build.lib.events.Location; +import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.util.io.OutErr; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.common.options.EnumConverter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; @@ -188,6 +191,17 @@ public boolean forceExternalRepositories; @Option( + name = "attempt_to_print_relative_paths", + oldName = "experimental_ui_attempt_to_print_relative_paths", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.LOGGING, + effectTags = {OptionEffectTag.TERMINAL_OUTPUT}, + help = + "When printing the location part of messages, attempt to use a path relative to the " + + "workspace directory or one of the directories specified by --package_path.") + public boolean attemptToPrintRelativePaths; + + @Option( name = "experimental_ui", defaultValue = "true", documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, @@ -242,16 +256,6 @@ help = "Make the experimental UI deduplicate messages to have a cleaner scroll-back log.") public boolean experimentalUiDeduplicate; - @Option( - name = "experimental_ui_attempt_to_print_relative_paths", - defaultValue = "false", - documentationCategory = OptionDocumentationCategory.LOGGING, - effectTags = {OptionEffectTag.TERMINAL_OUTPUT}, - help = - "When printing the location part of messages, attempt to use a path relative to the " - + "workspace directory or one of the directories specified by --package_path.") - public boolean experimentalUiAttemptToPrintRelativePaths; - public boolean useColor() { return useColorEnum == UseColor.YES || (useColorEnum == UseColor.AUTO && isStderrATty); } @@ -273,7 +277,10 @@ protected final boolean showTimestamp; - public BlazeCommandEventHandler(OutErr outErr, Options eventOptions) { + protected final LocationPrinter locationPrinter; + + public BlazeCommandEventHandler( + OutErr outErr, Options eventOptions, PathFragment workspacePathFragment) { this.outErr = outErr; this.errPrintStream = new PrintStream(outErr.getErrorStream(), true); if (eventOptions.showProgress) { @@ -288,6 +295,8 @@ } eventMask.add(EventKind.SUBCOMMAND); this.showTimestamp = eventOptions.showTimestamp; + this.locationPrinter = + new LocationPrinter(eventOptions.attemptToPrintRelativePaths, workspacePathFragment); } /** See EventHandler.handle. */ @@ -334,7 +343,7 @@ Location location = event.getLocation(); if (location != null) { - buf.append(location.print()).append(": "); + buf.append(locationPrinter.getLocationString(location)).append(": "); } buf.append(event.getMessage()); @@ -382,4 +391,9 @@ protected String timestamp() { return TIMESTAMP_FORMAT.format(ZonedDateTime.now(ZoneId.systemDefault())); } + + @Subscribe + public void packageLocatorCreated(PathPackageLocator packageLocator) { + locationPrinter.packageLocatorCreated(packageLocator); + } }
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalEventHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalEventHandler.java index ac7a2b8..888168f 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalEventHandler.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalEventHandler.java
@@ -13,9 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.runtime; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Splitter; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.eventbus.AllowConcurrentEvents; import com.google.common.eventbus.Subscribe; @@ -53,7 +51,6 @@ import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -97,6 +94,7 @@ private final AnsiTerminal terminal; private final boolean debugAllEvents; private final ExperimentalStateTracker stateTracker; + private final LocationPrinter locationPrinter; private final boolean showProgress; private final boolean progressInTermTitle; private final boolean showTimestamp; @@ -116,10 +114,6 @@ private byte[] stderrBuffer; private final Set<String> messagesSeen; private long deduplicateCount; - private final boolean attemptToPrintRelativePaths; - @Nullable private final PathFragment workspacePathFragment; - private final AtomicReference<ImmutableList<Root>> packagePathRootsRef = - new AtomicReference<>(ImmutableList.of()); private final long outputLimit; private long reservedOutputCapacity; @@ -223,7 +217,7 @@ OutErr outErr, BlazeCommandEventHandler.Options options, Clock clock, - @Nullable Path workspacePath) { + @Nullable PathFragment workspacePathFragment) { this.terminalWidth = (options.terminalColumns > 0 ? options.terminalColumns : 80); this.outputLimit = options.experimentalUiLimitConsoleOutput; this.counter = new AtomicLong(outputLimit); @@ -256,8 +250,8 @@ this.debugAllEvents = options.experimentalUiDebugAllEvents; this.deduplicate = options.experimentalUiDeduplicate; this.messagesSeen = new HashSet<>(); - this.attemptToPrintRelativePaths = options.experimentalUiAttemptToPrintRelativePaths; - this.workspacePathFragment = workspacePath == null ? null : workspacePath.asFragment(); + this.locationPrinter = + new LocationPrinter(options.attemptToPrintRelativePaths, workspacePathFragment); // If we have cursor control, we try to fit in the terminal width to avoid having // to wrap the progress bar. We will wrap the progress bar to terminalWidth - 1 // characters to avoid depending on knowing whether the underlying terminal does the @@ -478,10 +472,7 @@ incompleteLine = true; Location location = event.getLocation(); if (location != null) { - String locationString = attemptToPrintRelativePaths - ? getRelativeLocationString(location) - : location.toString(); - terminal.writeString(locationString + ": "); + terminal.writeString(locationPrinter.getLocationString(location) + ": "); } if (event.getMessage() != null) { terminal.writeString(event.getMessage()); @@ -525,37 +516,6 @@ } } - @Subscribe - public void packageLocatorCreated(PathPackageLocator packageLocator) { - packagePathRootsRef.set(packageLocator.getPathEntries()); - } - - private String getRelativeLocationString(Location location) { - return getRelativeLocationString(location, workspacePathFragment, packagePathRootsRef.get()); - } - - @VisibleForTesting - static String getRelativeLocationString( - Location location, - @Nullable PathFragment workspacePathFragment, - ImmutableList<Root> packagePathRoots) { - PathFragment relativePathToUse = null; - PathFragment locationPathFragment = location.getPath(); - if (locationPathFragment.isAbsolute()) { - if (workspacePathFragment != null && locationPathFragment.startsWith(workspacePathFragment)) { - relativePathToUse = locationPathFragment.relativeTo(workspacePathFragment); - } else { - for (Root packagePathRoot : packagePathRoots) { - if (packagePathRoot.contains(locationPathFragment)) { - relativePathToUse = packagePathRoot.relativize(locationPathFragment); - break; - } - } - } - } - return relativePathToUse == null ? location.print() : location.printWithPath(relativePathToUse); - } - private void setEventKindColor(EventKind kind) throws IOException { switch (kind) { case ERROR: @@ -751,6 +711,11 @@ } @Subscribe + public void packageLocatorCreated(PathPackageLocator packageLocator) { + locationPrinter.packageLocatorCreated(packageLocator); + } + + @Subscribe public void noBuild(NoBuildEvent event) { if (event.showProgress()) { synchronized (this) {
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/FancyTerminalEventHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/FancyTerminalEventHandler.java index 0b7d8a2..8a328b8 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/FancyTerminalEventHandler.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/FancyTerminalEventHandler.java
@@ -25,6 +25,7 @@ import com.google.devtools.build.lib.util.io.LineCountingAnsiTerminalWriter; import com.google.devtools.build.lib.util.io.LineWrappingAnsiTerminalWriter; import com.google.devtools.build.lib.util.io.OutErr; +import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.time.Duration; @@ -113,8 +114,9 @@ private boolean previousLineErasable = false; private int numLinesPreviousErasable = 0; - public FancyTerminalEventHandler(OutErr outErr, BlazeCommandEventHandler.Options options) { - super(outErr, options); + public FancyTerminalEventHandler( + OutErr outErr, BlazeCommandEventHandler.Options options, PathFragment workspacePathFragment) { + super(outErr, options, workspacePathFragment); this.terminal = new AnsiTerminal(outErr.getErrorStream()); this.terminalWidth = (options.terminalColumns > 0 ? options.terminalColumns : 80); useColor = options.useColor(); @@ -463,7 +465,7 @@ terminal.writeString(timestamp()); } if (event.getLocation() != null) { - terminal.writeString(event.getLocation() + ": "); + terminal.writeString(locationPrinter.getLocationString(event.getLocation()) + ": "); } }
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/LocationPrinter.java b/src/main/java/com/google/devtools/build/lib/runtime/LocationPrinter.java new file mode 100644 index 0000000..e7b52a2 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/runtime/LocationPrinter.java
@@ -0,0 +1,69 @@ +// Copyright 2018 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.runtime; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.events.Location; +import com.google.devtools.build.lib.pkgcache.PathPackageLocator; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.Root; +import java.util.concurrent.atomic.AtomicReference; +import javax.annotation.Nullable; + +class LocationPrinter { + private final boolean attemptToPrintRelativePaths; + @Nullable private final PathFragment workspacePathFragment; + private final AtomicReference<ImmutableList<Root>> packagePathRootsRef = + new AtomicReference<>(ImmutableList.of()); + + LocationPrinter( + boolean attemptToPrintRelativePaths, + @Nullable PathFragment workspacePathFragment) { + this.attemptToPrintRelativePaths = attemptToPrintRelativePaths; + this.workspacePathFragment = workspacePathFragment; + } + + void packageLocatorCreated(PathPackageLocator packageLocator) { + packagePathRootsRef.set(packageLocator.getPathEntries()); + } + + String getLocationString(Location location) { + return attemptToPrintRelativePaths + ? getRelativeLocationString(location, workspacePathFragment, packagePathRootsRef.get()) + : location.toString(); + } + + @VisibleForTesting + static String getRelativeLocationString( + Location location, + @Nullable PathFragment workspacePathFragment, + ImmutableList<Root> packagePathRoots) { + PathFragment relativePathToUse = null; + PathFragment locationPathFragment = location.getPath(); + if (locationPathFragment.isAbsolute()) { + if (workspacePathFragment != null && locationPathFragment.startsWith(workspacePathFragment)) { + relativePathToUse = locationPathFragment.relativeTo(workspacePathFragment); + } else { + for (Root packagePathRoot : packagePathRoots) { + if (packagePathRoot.contains(locationPathFragment)) { + relativePathToUse = packagePathRoot.relativize(locationPathFragment); + break; + } + } + } + } + return relativePathToUse == null ? location.print() : location.printWithPath(relativePathToUse); + } +}
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/ExperimentalEventHandlerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/LocationPrinterTest.java similarity index 89% rename from src/test/java/com/google/devtools/build/lib/runtime/ExperimentalEventHandlerTest.java rename to src/test/java/com/google/devtools/build/lib/runtime/LocationPrinterTest.java index b2ace7c..a4386ad 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/ExperimentalEventHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/LocationPrinterTest.java
@@ -26,15 +26,15 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** Tests for {@link ExperimentalEventHandler} static methods. */ +/** Tests for {@link LocationPrinter} static methods. */ @RunWith(JUnit4.class) -public class ExperimentalEventHandlerTest { +public class LocationPrinterTest { private final FileSystem fileSystem = new InMemoryFileSystem(); @Test public void getRelativeLocationString_PathIsAlreadyRelative() { assertThat( - ExperimentalEventHandler.getRelativeLocationString( + LocationPrinter.getRelativeLocationString( Location.fromPathAndStartColumn( PathFragment.create("relative/path"), 0, 0, new LineAndColumn(4, 2)), PathFragment.create("/this/is/the/workspace"), @@ -46,7 +46,7 @@ @Test public void getRelativeLocationString_PathIsAbsoluteAndWorkspaceIsNull() { assertThat( - ExperimentalEventHandler.getRelativeLocationString( + LocationPrinter.getRelativeLocationString( Location.fromPathAndStartColumn( PathFragment.create("/absolute/path"), 0, 0, new LineAndColumn(4, 2)), null, @@ -58,7 +58,7 @@ @Test public void getRelativeLocationString_PathIsAbsoluteButNotUnderWorkspaceOrPackagePathRoots() { assertThat( - ExperimentalEventHandler.getRelativeLocationString( + LocationPrinter.getRelativeLocationString( Location.fromPathAndStartColumn( PathFragment.create("/absolute/path"), 0, 0, new LineAndColumn(4, 2)), PathFragment.create("/this/is/the/workspace"), @@ -70,7 +70,7 @@ @Test public void getRelativeLocationString_PathIsAbsoluteAndUnderWorkspace() { assertThat( - ExperimentalEventHandler.getRelativeLocationString( + LocationPrinter.getRelativeLocationString( Location.fromPathAndStartColumn( PathFragment.create("/this/is/the/workspace/blah.txt"), 0, @@ -85,7 +85,7 @@ @Test public void getRelativeLocationString_PathIsAbsoluteAndUnderPackagePathRoot() { assertThat( - ExperimentalEventHandler.getRelativeLocationString( + LocationPrinter.getRelativeLocationString( Location.fromPathAndStartColumn( PathFragment.create("/this/is/a/package/path/root3/blah.txt"), 0,
diff --git a/src/test/shell/integration/experimental_ui_test.sh b/src/test/shell/integration/experimental_ui_test.sh index d8911ba..213a576 100755 --- a/src/test/shell/integration/experimental_ui_test.sh +++ b/src/test/shell/integration/experimental_ui_test.sh
@@ -501,7 +501,9 @@ expect_log "dropped.*console" } -function test_experimental_ui_attempt_to_print_relative_paths_failing_action() { +function run_test_attempt_to_print_relative_paths_failing_action() { + local ui="$1" + # On the BazelCI Windows environment, `pwd` returns a string that uses a # lowercase drive letter and unix-style path separators (e.g. '/c/') with # a lowercase drive letter. But internally in Bazel, Path#toString @@ -514,40 +516,60 @@ [[ "$is_windows" == "true" ]] && return 0 bazel clean || fail "${PRODUCT_NAME} clean failed" + bazel build \ - --experimental_ui \ - --experimental_ui_attempt_to_print_relative_paths=false \ + "$ui" \ + --attempt_to_print_relative_paths=false \ error:failwitherror > "${TEST_log}" 2>&1 && fail "expected failure" expect_log "^ERROR: $(pwd)/error/BUILD:1:1: Executing genrule" bazel build \ - --experimental_ui \ - --experimental_ui_attempt_to_print_relative_paths=true \ + "$ui" \ + --attempt_to_print_relative_paths=true \ error:failwitherror > "${TEST_log}" 2>&1 && fail "expected failure" expect_log "^ERROR: error/BUILD:1:1: Executing genrule" expect_not_log "$(pwd)/error/BUILD" } -function test_experimental_ui_attempt_to_print_relative_paths_package_loading_error() { +function test_experimental_ui_attempt_to_print_relative_paths_failing_action() { + run_test_attempt_to_print_relative_paths_failing_action "--experimental_ui" +} + +function test_noexperimental_ui_attempt_to_print_relative_paths_failing_action() { + run_test_attempt_to_print_relative_paths_failing_action "--noexperimental_ui" +} + +function run_test_attempt_to_print_relative_paths_pkg_error() { + local ui="$1" + # See the note in the test case above for why this is disabled on Windows. # TODO(nharmata): Fix this. [[ "$is_windows" == "true" ]] && return 0 bazel clean || fail "${PRODUCT_NAME} clean failed" + bazel build \ - --experimental_ui \ - --experimental_ui_attempt_to_print_relative_paths=false \ + "$ui" \ + --attempt_to_print_relative_paths=false \ pkgloadingerror:all > "${TEST_log}" 2>&1 && fail "expected failure" expect_log "^ERROR: $(pwd)/bzl/bzl.bzl:1:5: name 'invalidsyntax' is not defined" bazel build \ - --experimental_ui \ - --experimental_ui_attempt_to_print_relative_paths=true \ + "$ui" \ + --attempt_to_print_relative_paths=true \ pkgloadingerror:all > "${TEST_log}" 2>&1 && fail "expected failure" expect_log "^ERROR: bzl/bzl.bzl:1:5: name 'invalidsyntax' is not defined" expect_not_log "$(pwd)/bzl/bzl.bzl" } +function test_experimental_ui_attempt_to_print_relative_paths_pkg_error() { + run_test_attempt_to_print_relative_paths_pkg_error "--experimental_ui" +} + +function test_noexperimental_ui_attempt_to_print_relative_paths_pkg_error() { + run_test_attempt_to_print_relative_paths_pkg_error "--noexperimental_ui" +} + function test_fancy_symbol_encoding() { bazel build //fancyOutput:withFancyOutput > "${TEST_log}" 2>&1 \ || fail "expected success"