Fix #837: worker_verbose flag has no effect.
Due to reusing an old Reporter instead of grabbing the current one for each build, verbose messages were lost and not printed to the console. Also adds a test for this feature.
--
MOS_MIGRATED_REVID=122639383
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
index 5deafce..3c444a2 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
@@ -25,11 +25,12 @@
* Factory used by the pool to create / destroy / validate worker processes.
*/
final class WorkerFactory extends BaseKeyedPooledObjectFactory<WorkerKey, Worker> {
- private Path logDir;
+ private final Path logDir;
private Reporter reporter;
private boolean verbose;
- public void setLogDirectory(Path logDir) {
+ public WorkerFactory(Path logDir) {
+ super();
this.logDir = logDir;
}
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
index 5e77542..e16bd38 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
@@ -37,7 +37,8 @@
* A module that adds the WorkerActionContextProvider to the available action context providers.
*/
public class WorkerModule extends BlazeModule {
- private WorkerPool workers;
+ private WorkerFactory workerFactory;
+ private WorkerPool workerPool;
private CommandEnvironment env;
private BuildRequest buildRequest;
@@ -55,7 +56,7 @@
this.env = env;
env.getEventBus().register(this);
- if (workers == null) {
+ if (workerFactory == null) {
Path logDir = env.getOutputBase().getRelative("worker-logs");
try {
logDir.createDirectory();
@@ -65,6 +66,12 @@
.handle(Event.error("Could not create directory for worker logs: " + logDir));
}
+ workerFactory = new WorkerFactory(logDir);
+ }
+
+ workerFactory.setReporter(env.getReporter());
+
+ if (workerPool == null) {
GenericKeyedObjectPoolConfig config = new GenericKeyedObjectPoolConfig();
// It's better to re-use a worker as often as possible and keep it hot, in order to profit
@@ -85,23 +92,21 @@
// worker.
config.setMaxTotal(-1);
- workers = new WorkerPool(new WorkerFactory(), config);
- workers.setReporter(env.getReporter());
- workers.setLogDirectory(logDir);
+ workerPool = new WorkerPool(workerFactory, config);
}
}
@Subscribe
public void buildStarting(BuildStartingEvent event) {
- Preconditions.checkNotNull(workers);
+ Preconditions.checkNotNull(workerPool);
this.buildRequest = event.getRequest();
WorkerOptions options = buildRequest.getOptions(WorkerOptions.class);
- workers.setMaxTotalPerKey(options.workerMaxInstances);
- workers.setMaxIdlePerKey(options.workerMaxInstances);
- workers.setMinIdlePerKey(options.workerMaxInstances);
- workers.setVerbose(options.workerVerbose);
+ workerPool.setMaxTotalPerKey(options.workerMaxInstances);
+ workerPool.setMaxIdlePerKey(options.workerMaxInstances);
+ workerPool.setMinIdlePerKey(options.workerMaxInstances);
+ workerFactory.setVerbose(options.workerVerbose);
this.verbose = options.workerVerbose;
}
@@ -109,10 +114,10 @@
public Iterable<ActionContextProvider> getActionContextProviders() {
Preconditions.checkNotNull(env);
Preconditions.checkNotNull(buildRequest);
- Preconditions.checkNotNull(workers);
+ Preconditions.checkNotNull(workerPool);
return ImmutableList.<ActionContextProvider>of(
- new WorkerActionContextProvider(env, buildRequest, workers));
+ new WorkerActionContextProvider(env, buildRequest, workerPool));
}
@Override
@@ -122,7 +127,7 @@
@Subscribe
public void buildComplete(BuildCompleteEvent event) {
- if (workers != null && buildRequest != null
+ if (workerPool != null && buildRequest != null
&& buildRequest.getOptions(WorkerOptions.class) != null
&& buildRequest.getOptions(WorkerOptions.class).workerQuitAfterBuild) {
if (verbose) {
@@ -130,8 +135,8 @@
.getReporter()
.handle(Event.info("Build completed, shutting down worker pool..."));
}
- workers.close();
- workers = null;
+ workerPool.close();
+ workerPool = null;
}
}
@@ -140,14 +145,14 @@
// for them to finish.
@Subscribe
public void buildInterrupted(BuildInterruptedEvent event) {
- if (workers != null) {
+ if (workerPool != null) {
if (verbose) {
env
.getReporter()
.handle(Event.info("Build interrupted, shutting down worker pool..."));
}
- workers.close();
- workers = null;
+ workerPool.close();
+ workerPool = null;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java
index bf5e458..d1eb29e 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java
@@ -14,8 +14,6 @@
package com.google.devtools.build.lib.worker;
import com.google.common.base.Throwables;
-import com.google.devtools.build.lib.events.Reporter;
-import com.google.devtools.build.lib.vfs.Path;
import org.apache.commons.pool2.impl.GenericKeyedObjectPool;
import org.apache.commons.pool2.impl.GenericKeyedObjectPoolConfig;
@@ -32,23 +30,9 @@
*/
@ThreadSafe
final class WorkerPool extends GenericKeyedObjectPool<WorkerKey, Worker> {
- final WorkerFactory workerFactory;
public WorkerPool(WorkerFactory factory, GenericKeyedObjectPoolConfig config) {
super(factory, config);
- this.workerFactory = factory;
- }
-
- public void setLogDirectory(Path logDir) {
- this.workerFactory.setLogDirectory(logDir);
- }
-
- public void setReporter(Reporter reporter) {
- this.workerFactory.setReporter(reporter);
- }
-
- public void setVerbose(boolean verbose) {
- this.workerFactory.setVerbose(verbose);
}
@Override
diff --git a/src/test/shell/bazel/bazel_worker_test.sh b/src/test/shell/bazel/bazel_worker_test.sh
index 8ad58c8..38f768d 100755
--- a/src/test/shell/bazel/bazel_worker_test.sh
+++ b/src/test/shell/bazel/bazel_worker_test.sh
@@ -27,7 +27,7 @@
example_worker=$(find $BAZEL_RUNFILES -name ExampleWorker_deploy.jar)
function set_up() {
- bazel build --worker_quit_after_build
+ bazel build --worker_quit_after_build &> $TEST_log
assert_workers_not_running
}
@@ -90,7 +90,8 @@
function test_compiles_hello_library_using_persistent_javac() {
write_hello_library_files
- bazel build --strategy=Javac=worker //java/main:main || fail "build failed"
+ bazel build --strategy=Javac=worker //java/main:main &> $TEST_log \
+ || fail "build failed"
bazel-bin/java/main/main | grep -q "Hello, Library!;Hello, World!" \
|| fail "comparison failed"
assert_workers_running
@@ -99,7 +100,7 @@
function test_workers_quit_after_build() {
write_hello_library_files
- bazel build --worker_quit_after_build --strategy=Javac=worker //java/main:main \
+ bazel build --worker_quit_after_build --strategy=Javac=worker //java/main:main &> $TEST_log \
|| fail "build failed"
assert_workers_not_running
}
@@ -176,12 +177,12 @@
)
EOF
- bazel build --strategy=Work=worker :hello_world \
+ bazel build --strategy=Work=worker :hello_world &> $TEST_log \
|| fail "build failed"
assert_equals "hello world" "$(cat bazel-bin/hello_world.out)"
assert_workers_running
- bazel build --worker_quit_after_build --strategy=Work=worker :hello_world_uppercase \
+ bazel build --worker_quit_after_build --strategy=Work=worker :hello_world_uppercase &> $TEST_log \
|| fail "build failed"
assert_equals "HELLO WORLD" "$(cat bazel-bin/hello_world_uppercase.out)"
assert_workers_not_running
@@ -198,14 +199,14 @@
) for idx in range(10)]
EOF
- bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_1 \
+ bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_1 &> $TEST_log \
|| fail "build failed"
worker_uuid_1=$(cat bazel-bin/hello_world_1.out | grep UUID | cut -d' ' -f2)
work_count=$(cat bazel-bin/hello_world_1.out | grep COUNTER | cut -d' ' -f2)
assert_equals "1" $work_count
assert_workers_running
- bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_2 \
+ bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_2 &> $TEST_log \
|| fail "build failed"
worker_uuid_2=$(cat bazel-bin/hello_world_2.out | grep UUID | cut -d' ' -f2)
work_count=$(cat bazel-bin/hello_world_2.out | grep COUNTER | cut -d' ' -f2)
@@ -215,7 +216,7 @@
# Check that the same worker was used twice.
assert_equals "$worker_uuid_1" "$worker_uuid_2"
- bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_3 \
+ bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_3 &> $TEST_log \
|| fail "build failed"
worker_uuid_3=$(cat bazel-bin/hello_world_3.out | grep UUID | cut -d' ' -f2)
work_count=$(cat bazel-bin/hello_world_3.out | grep COUNTER | cut -d' ' -f2)
@@ -236,14 +237,14 @@
) for idx in range(10)]
EOF
- bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_1 \
+ bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_1 &> $TEST_log \
|| fail "build failed"
worker_uuid_1=$(cat bazel-bin/hello_world_1.out | grep UUID | cut -d' ' -f2)
work_count=$(cat bazel-bin/hello_world_1.out | grep COUNTER | cut -d' ' -f2)
assert_equals "1" $work_count
assert_workers_running
- bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_2 \
+ bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_2 &> $TEST_log \
|| fail "build failed"
worker_uuid_2=$(cat bazel-bin/hello_world_2.out | grep UUID | cut -d' ' -f2)
work_count=$(cat bazel-bin/hello_world_2.out | grep COUNTER | cut -d' ' -f2)
@@ -258,7 +259,7 @@
zip worker_lib.jar dummy_file
rm dummy_file
- bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_3 \
+ bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_3 &> $TEST_log \
|| fail "build failed"
worker_uuid_3=$(cat bazel-bin/hello_world_3.out | grep UUID | cut -d' ' -f2)
work_count=$(cat bazel-bin/hello_world_3.out | grep COUNTER | cut -d' ' -f2)
@@ -279,14 +280,14 @@
) for idx in range(10)]
EOF
- bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_1 \
+ bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_1 &> $TEST_log \
|| fail "build failed"
worker_uuid_1=$(cat bazel-bin/hello_world_1.out | grep UUID | cut -d' ' -f2)
work_count=$(cat bazel-bin/hello_world_1.out | grep COUNTER | cut -d' ' -f2)
assert_equals "1" $work_count
assert_workers_running
- bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_2 \
+ bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_2 &> $TEST_log \
|| fail "build failed"
worker_uuid_2=$(cat bazel-bin/hello_world_2.out | grep UUID | cut -d' ' -f2)
work_count=$(cat bazel-bin/hello_world_2.out | grep COUNTER | cut -d' ' -f2)
@@ -298,7 +299,7 @@
echo "changeddata" > worker_data.txt
- bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_3 \
+ bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_3 &> $TEST_log \
|| fail "build failed"
worker_uuid_3=$(cat bazel-bin/hello_world_3.out | grep UUID | cut -d' ' -f2)
work_count=$(cat bazel-bin/hello_world_3.out | grep COUNTER | cut -d' ' -f2)
@@ -323,12 +324,12 @@
) for idx in range(10)]
EOF
- bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_1 \
+ bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_1 &> $TEST_log \
|| fail "build failed"
worker_uuid_1=$(cat bazel-bin/hello_world_1.out | grep UUID | cut -d' ' -f2)
assert_workers_running
- bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_2 \
+ bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_2 &> $TEST_log \
|| fail "build failed"
worker_uuid_2=$(cat bazel-bin/hello_world_2.out | grep UUID | cut -d' ' -f2)
assert_workers_running
@@ -349,13 +350,13 @@
EOF
echo "hello world" > input.txt
- bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_1 \
+ bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_1 &> $TEST_log \
|| fail "build failed"
worker_uuid_1=$(cat bazel-bin/hello_world_1.out | grep UUID | cut -d' ' -f2)
hash1=$(fgrep "INPUT input.txt " bazel-bin/hello_world_1.out | cut -d' ' -f3)
assert_workers_running
- bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_2 \
+ bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_2 &> $TEST_log \
|| fail "build failed"
worker_uuid_2=$(cat bazel-bin/hello_world_2.out | grep UUID | cut -d' ' -f2)
hash2=$(fgrep "INPUT input.txt " bazel-bin/hello_world_2.out | cut -d' ' -f3)
@@ -366,7 +367,7 @@
echo "changeddata" > input.txt
- bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_3 \
+ bazel build --strategy=Work=worker --worker_max_instances=1 :hello_world_3 &> $TEST_log \
|| fail "build failed"
worker_uuid_3=$(cat bazel-bin/hello_world_3.out | grep UUID | cut -d' ' -f2)
hash3=$(fgrep "INPUT input.txt " bazel-bin/hello_world_3.out | cut -d' ' -f3)
@@ -376,4 +377,21 @@
assert_not_equals "$hash2" "$hash3"
}
+function test_worker_verbose() {
+ prepare_example_worker
+ cat >>BUILD <<'EOF'
+[work(
+ name = "hello_world_%s" % idx,
+ worker = ":worker",
+ args = ["--write_uuid", "--write_counter"],
+) for idx in range(10)]
+EOF
+
+ bazel build --strategy=Work=worker --worker_max_instances=1 --worker_verbose --worker_quit_after_build :hello_world_1 &> $TEST_log \
+ || fail "build failed"
+ expect_log "Created new Work worker (id [0-9]\+)"
+ expect_log "Destroying Work worker (id [0-9]\+)"
+ expect_log "Build completed, shutting down worker pool..."
+}
+
run_suite "Worker integration tests"
