From ec58d49b2764ef59d981bd65b31d7dc394db1bd9 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Wed, 20 Jul 2016 17:04:39 +0000 Subject: Consolidate Bazel-specific javacopt handling -- MOS_MIGRATED_REVID=127957458 --- .../java/com/google/devtools/build/buildjar/BUILD | 21 ++- .../build/buildjar/javac/JavacOptions.java | 82 ++++++++--- .../com/google/devtools/build/java/bazel/BUILD | 7 +- .../build/java/bazel/BazelJavaCompiler.java | 151 +++++++++------------ .../build/java/bazel/JavaBuilderConfig.java | 18 +-- .../google/devtools/build/java/turbine/javac/BUILD | 1 + .../build/java/turbine/javac/JavacTurbine.java | 29 +--- .../build/java/turbine/javac/JavacTurbineTest.java | 23 ---- 8 files changed, 162 insertions(+), 170 deletions(-) (limited to 'src') diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BUILD b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BUILD index c9c6b24983..8dc934e8ac 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BUILD +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BUILD @@ -21,11 +21,17 @@ java_library( java_library( name = "javac", - srcs = glob([ - "javac/*.java", - ]), + srcs = glob( + [ + "javac/*.java", + ], + exclude = [ + "javac/JavacOptions.java", + ], + ), deps = [ ":invalid_command_line_exception", + ":javac_options", "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins", "//third_party:guava", "//third_party:jsr305", @@ -45,6 +51,7 @@ java_library( deps = [ ":invalid_command_line_exception", ":javac", + ":javac_options", "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar/jarhelper", "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins", "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins:classloader", @@ -69,8 +76,13 @@ java_library( srcs = [ "javac/JavacOptions.java", ], - visibility = ["//java/com/google/devtools/build/buildjar:__pkg__"], + visibility = [ + "//java/com/google/devtools/build/buildjar:__pkg__", + "//src/java_tools/buildjar/java/com/google/devtools/build/java/bazel:__pkg__", + "//src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac:__pkg__", + ], deps = [ + "//third_party:auto_value", "//third_party:guava", ], ) @@ -91,6 +103,7 @@ load("//tools/build_rules:java_rules_skylark.bzl", "bootstrap_java_library", "bo bootstrap_java_library( name = "skylark-deps", jars = [ + "//third_party:auto_value-jars", "//third_party:error_prone-jars", "//third_party:guava-jars", "//third_party:jsr305-jars", diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/JavacOptions.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/JavacOptions.java index e4452a36a0..4f7e749c7e 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/JavacOptions.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/JavacOptions.java @@ -14,45 +14,85 @@ package com.google.devtools.build.buildjar.javac; +import com.google.auto.value.AutoValue; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; - import java.util.ArrayList; +import java.util.Arrays; import java.util.LinkedHashSet; import java.util.List; import java.util.Set; /** - * Preprocess javac -Xlint options. We - * also need to make the different versions of javac treat -Xlint options - * uniformly. - * - *

Some versions of javac now process the -Xlint options without allowing - * later options to override earlier ones on the command line. For example, - * {@code -Xlint:All -Xlint:None} results in all warnings being enabled. + * Preprocess javac -Xlint options. We also need to make the different versions of javac treat + * -Xlint options uniformly. * - *

This class preprocesses the -Xlint options within the javac options to - * achieve a command line that is sensitive to ordering. That is, with this - * preprocessing step, {@code -Xlint:all -Xlint:none} results in no - * warnings being enabled. + *

Some versions of javac now process the -Xlint options without allowing later options to + * override earlier ones on the command line. For example, {@code -Xlint:All -Xlint:None} results in + * all warnings being enabled. * - *

TODO(bazel-team): Convince the current javac owners that the javac options - * need to behave the way this class behaves. :-) + *

This class preprocesses the -Xlint options within the javac options to achieve a command line + * that is sensitive to ordering. That is, with this preprocessing step, {@code -Xlint:all + * -Xlint:none} results in no warnings being enabled. */ public final class JavacOptions { + /** Returns an immutable list containing all the non-Bazel specific Javac flags. */ + public static ImmutableList removeBazelSpecificFlags(String[] javacopts) { + return removeBazelSpecificFlags(Arrays.asList(javacopts)); + } + + /** Returns an immutable list containing all the non-Bazel specific Javac flags. */ + public static ImmutableList removeBazelSpecificFlags(Iterable javacopts) { + return filterJavacopts(javacopts).standardJavacopts(); + } + + /** A collection of javac flags, divided into Bazel-specific and standard options. */ + @AutoValue + public abstract static class FilteredJavacopts { + /** Bazel-specific javac flags, e.g. Error Prone's -Xep: flags. */ + public abstract ImmutableList bazelJavacopts(); + + /** Standard javac flags. */ + public abstract ImmutableList standardJavacopts(); + + /** Creates a {@link FilteredJavacopts}. */ + public static FilteredJavacopts create( + ImmutableList bazelJavacopts, ImmutableList standardJavacopts) { + return new AutoValue_JavacOptions_FilteredJavacopts(bazelJavacopts, standardJavacopts); + } + } + + /** Filters a list of javac flags into Bazel-specific and standard flags. */ + public static FilteredJavacopts filterJavacopts(Iterable javacopts) { + ImmutableList.Builder bazelJavacopts = ImmutableList.builder(); + ImmutableList.Builder standardJavacopts = ImmutableList.builder(); + for (String opt : javacopts) { + if (isBazelSpecificFlag(opt)) { + bazelJavacopts.add(opt); + } else { + standardJavacopts.add(opt); + } + } + return FilteredJavacopts.create(bazelJavacopts.build(), standardJavacopts.build()); + } + + private static boolean isBazelSpecificFlag(String opt) { + return opt.startsWith("-Werror:") || opt.startsWith("-Xep"); + } + private static final XlintOptionNormalizer XLINT_OPTION_NORMALIZER = new XlintOptionNormalizer(); /** * Interface to define an option normalizer. For instance, to group all -Xlint: option into one * place. - * - *

All normalizers used by the JavacOptions class will be started by calling the - * {@link #start()} method when starting the parsing of a list of option. For each option, the - * first option normalized whose {@link #processOption(String)} method returns true stops its - * parsing and the option is supposed to be added at the end to the normalized list of option with - * the {@link #normalize(List)} method. Options not handled by a normalizer will be returned as - * such in the normalized option list. + * + *

All normalizers used by the JavacOptions class will be started by calling the {@link + * #start()} method when starting the parsing of a list of option. For each option, the first + * option normalized whose {@link #processOption(String)} method returns true stops its parsing + * and the option is supposed to be added at the end to the normalized list of option with the + * {@link #normalize(List)} method. Options not handled by a normalizer will be returned as such + * in the normalized option list. */ public static interface JavacOptionNormalizer { /** Resets the state of the normalizer to start a new option parsing. */ diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BUILD b/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BUILD index 5b2b0b7955..3ede677953 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BUILD +++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BUILD @@ -91,7 +91,11 @@ java_library( name = "JavaBuilderConfig", srcs = ["JavaBuilderConfig.java"], data = ["@bazel_tools//tools/jdk:JavaBuilder_deploy.jar"], - deps = [":javabuilder-javacopts-lib"], + deps = [ + ":javabuilder-javacopts-lib", + "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar:javac_options", + "//third_party:guava", + ], ) # Provides programmatic access to a bazel compatible javac. @@ -108,6 +112,7 @@ java_library( ":JavaBuilderConfig", ":JavaLangtools", ":JavacBootclasspath", + "//third_party:guava", ], ) diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BazelJavaCompiler.java b/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BazelJavaCompiler.java index 6a4156ad9f..206d0b66e1 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BazelJavaCompiler.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BazelJavaCompiler.java @@ -14,6 +14,7 @@ package com.google.devtools.build.java.bazel; +import com.google.common.collect.ImmutableList; import java.io.File; import java.io.IOException; import java.io.InputStream; @@ -23,8 +24,7 @@ import java.net.MalformedURLException; import java.net.URL; import java.net.URLClassLoader; import java.nio.charset.Charset; -import java.util.ArrayList; -import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Locale; import java.util.Set; @@ -49,27 +49,18 @@ import javax.tools.StandardLocation; public class BazelJavaCompiler { // The default blessed javac options. - - private static final String DEFAULT_BOOTCLASSPATH = JavacBootclasspath.asString(); - private static final String[] DEFAULT_JAVACOPTS; - static { - List defaultJavacopts = new ArrayList<>(); - for (String javacopt : JavaBuilderConfig.defaultJavacOpts()) { - if (javacopt.startsWith("-Xep")) { - // ignore Error Prone-specific flags accepted by JavaBuilder - continue; - } - defaultJavacopts.add(javacopt); - } + private static final String DEFAULT_BOOTCLASSPATH = JavacBootclasspath.asString(); - // The bootclasspath must be specified both via an invocation option and - // via fileManager.setLocation(PLATFORM_CLASS_PATH), to work around what - // appears to be a bug in jdk[6,8] javac. - defaultJavacopts.addAll(Arrays.asList("-bootclasspath", DEFAULT_BOOTCLASSPATH)); - - DEFAULT_JAVACOPTS = defaultJavacopts.toArray(new String[defaultJavacopts.size()]); - } + private static final ImmutableList DEFAULT_JAVACOPTS = + ImmutableList.builder() + .addAll(JavaBuilderConfig.defaultJavacOpts()) + // The bootclasspath must be specified both via an invocation option and + // via fileManager.setLocation(PLATFORM_CLASS_PATH), to work around what + // appears to be a bug in jdk[6,8] javac. + .add("-bootclasspath") + .add(DEFAULT_BOOTCLASSPATH) + .build(); private static final Class JAVA_COMPILER_CLASS = getJavaCompilerClass(); @@ -109,12 +100,10 @@ public class BazelJavaCompiler { public static File getLangtoolsJar() { return JavaLangtools.file(); } - - /** - * Returns the default javacopts, including the blessed bootclasspath. - */ + + /** Returns the default javacopts, including the blessed bootclasspath. */ public static List getDefaultJavacopts() { - return new ArrayList<>(Arrays.asList(DEFAULT_JAVACOPTS)); + return DEFAULT_JAVACOPTS; } /** @@ -186,70 +175,62 @@ public class BazelJavaCompiler { private static JavaCompiler newInstance(final JavaCompiler delegate) { // We forward most operations to the JavaCompiler implementation in langtools.jar. return new JavaCompiler() { - @Override - public CompilationTask getTask( - Writer out, - JavaFileManager fileManager, - DiagnosticListener diagnosticListener, - Iterable options, - Iterable classes, - Iterable compilationUnits) { - // We prepend bazel's default javacopts to user javacopts, - // so that the user can override them. javac supports this - // "last option wins" style of option override. - List fullOptions = getDefaultJavacopts(); - if (options != null) { - for (String option : options) { - fullOptions.add(option); - } - } - return delegate.getTask(out, - fileManager, - diagnosticListener, - fullOptions, - classes, - compilationUnits); + @Override + public CompilationTask getTask( + Writer out, + JavaFileManager fileManager, + DiagnosticListener diagnosticListener, + Iterable options, + Iterable classes, + Iterable compilationUnits) { + // We prepend bazel's default javacopts to user javacopts, + // so that the user can override them. javac supports this + // "last option wins" style of option override. + ImmutableList.Builder fullOptions = ImmutableList.builder(); + fullOptions.addAll(getDefaultJavacopts()); + if (options != null) { + fullOptions.addAll(options); } + return delegate.getTask( + out, fileManager, diagnosticListener, fullOptions.build(), classes, compilationUnits); + } - @Override - public StandardJavaFileManager getStandardFileManager( - DiagnosticListener diagnosticListener, - Locale locale, - Charset charset) { - StandardJavaFileManager fileManager = delegate.getStandardFileManager( - diagnosticListener, - locale, - charset); - - try { - fileManager.setLocation( - StandardLocation.PLATFORM_CLASS_PATH, // bootclasspath - JavacBootclasspath.asFiles()); - } catch (IOException e) { - // Should never happen, according to javadocs for setLocation - throw new RuntimeException(e); - } - return fileManager; + @Override + public StandardJavaFileManager getStandardFileManager( + DiagnosticListener diagnosticListener, + Locale locale, + Charset charset) { + StandardJavaFileManager fileManager = + delegate.getStandardFileManager(diagnosticListener, locale, charset); + + try { + fileManager.setLocation( + StandardLocation.PLATFORM_CLASS_PATH, // bootclasspath + JavacBootclasspath.asFiles()); + } catch (IOException e) { + // Should never happen, according to javadocs for setLocation + throw new RuntimeException(e); } + return fileManager; + } - @Override - public int run(InputStream in, OutputStream out, OutputStream err, - String... arguments) { - // prepend bazel's default javacopts to user arguments - List args = getDefaultJavacopts(); - args.addAll(Arrays.asList(arguments)); - return delegate.run(in, out, err, args.toArray(new String[0])); - } + @Override + public int run(InputStream in, OutputStream out, OutputStream err, String... arguments) { + // prepend bazel's default javacopts to user arguments + List args = getDefaultJavacopts(); + Collections.addAll(args, arguments); + return delegate.run(in, out, err, args.toArray(new String[0])); + } - @Override - public Set getSourceVersions() { - return delegate.getSourceVersions(); - } + @Override + public Set getSourceVersions() { + return delegate.getSourceVersions(); + } - @Override - public int isSupportedOption(String option) { - return delegate.isSupportedOption(option); - } - }; + @Override + public int isSupportedOption(String option) { + return delegate.isSupportedOption(option); + } + }; } } diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/JavaBuilderConfig.java b/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/JavaBuilderConfig.java index 511dfa584f..c86291dd0b 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/JavaBuilderConfig.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/JavaBuilderConfig.java @@ -14,18 +14,18 @@ package com.google.devtools.build.java.bazel; -import java.util.ArrayList; -import java.util.Collections; +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.buildjar.javac.JavacOptions; import java.util.List; -/** - * Utility class to provide java-level access to the blessed javabuilder javacopts. - */ +/** Utility class to provide Java-level access to the blessed JavaBuilder javacopts. */ public class JavaBuilderConfig { - + + private static final ImmutableList JAVACOPTS = + ImmutableList.copyOf( + JavacOptions.removeBazelSpecificFlags(JavaBuilderJavacOpts.DEFAULT_JAVACOPTS)); + public static List defaultJavacOpts() { - List result = new ArrayList<>(); - Collections.addAll(result, JavaBuilderJavacOpts.DEFAULT_JAVACOPTS); - return result; + return JAVACOPTS; } } diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/BUILD b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/BUILD index 9e3e6cbeea..9ab49b9ca7 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/BUILD +++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/BUILD @@ -14,6 +14,7 @@ java_library( ":javac_turbine_compiler", ":zip_output_file_manager", ":zip_util", + "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar:javac_options", "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins:dependency", "//src/java_tools/buildjar/java/com/google/devtools/build/java/turbine:turbine_options", "//src/java_tools/buildjar/java/com/google/devtools/build/java/turbine:turbine_options_parser", diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java index b294bd2a3c..8959d3613b 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java @@ -15,10 +15,10 @@ package com.google.devtools.build.java.turbine.javac; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.CharMatcher; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.buildjar.javac.JavacOptions; import com.google.devtools.build.buildjar.javac.plugins.dependency.DependencyModule; import com.google.devtools.build.buildjar.javac.plugins.dependency.DependencyModule.StrictJavaDeps; import com.google.devtools.build.buildjar.javac.plugins.dependency.StrictJavaDepsPlugin; @@ -119,7 +119,7 @@ public class JavacTurbine implements AutoCloseable { ImmutableList.Builder argbuilder = ImmutableList.builder(); - filterJavacopts(argbuilder, turbineOptions.javacOpts()); + argbuilder.addAll(JavacOptions.removeBazelSpecificFlags(turbineOptions.javacOpts())); // Disable compilation of implicit source files. // This is insurance: the sourcepath is empty, so we don't expect implicit sources. @@ -321,31 +321,6 @@ public class JavacTurbine implements AutoCloseable { return result.build(); } - @VisibleForTesting - static void filterJavacopts( - ImmutableList.Builder javacArgs, Iterable defaultJavacopts) { - for (String opt : defaultJavacopts) { - - // TODO(cushon): temporary hack until 4149f08bcc8bd1318d4021cf372ec89240ee3dbb is released - opt = CharMatcher.is('\'').trimFrom(opt); - - if (isErrorProneFlag(opt)) { - // drop Error Prone's fake javacopts - continue; - } - javacArgs.add(opt); - } - } - - /** - * Returns true for flags that are specific to Error Prone. - * - *

WARNING: keep in sync with ErrorProneOptions#isSupportedOption - */ - static boolean isErrorProneFlag(String opt) { - return opt.startsWith("-Xep"); - } - /** Extra sources in srcjars to disk. */ private static List extractSourceJars(TurbineOptions turbineOptions, Path tmpdir) throws IOException { diff --git a/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java b/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java index 0f20f16112..fb5f6249de 100644 --- a/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java +++ b/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java @@ -349,29 +349,6 @@ public class JavacTurbineTest { } }; - @Test - public void filterJavacopts() { - ImmutableList.Builder output = ImmutableList.builder(); - JavacTurbine.filterJavacopts( - output, - Arrays.asList( - "-g", - "-source", - "6", - "-target", - "6", - "-Xlint:all", - "-source", - "7", - "-target", - "7", - "-Xep:GuardedBy:ERROR")); - assertThat(output.build()) - .containsExactly( - "-g", "-source", "6", "-target", "6", "-Xlint:all", "-source", "7", "-target", "7") - .inOrder(); - } - @Test public void jdeps() throws Exception { -- cgit v1.2.3