Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test case and fix for annotation processor option with space #272

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,10 @@
.idea/
target/
/.apt_generated/
/.apt_generated_tests/

# Eclipse
.classpath
.project
.factorypath
/.settings/
45 changes: 42 additions & 3 deletions java/com/google/turbine/options/TurbineOptionsParser.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.turbine.options;

import static com.google.common.base.Preconditions.checkArgument;
import static java.nio.charset.StandardCharsets.UTF_8;

import com.google.common.base.CharMatcher;
import com.google.common.base.Splitter;
Expand Down Expand Up @@ -47,9 +48,7 @@ public static TurbineOptions parse(Iterable<String> args) throws IOException {
*/
public static void parse(TurbineOptions.Builder builder, Iterable<String> args)
throws IOException {
Deque<String> argumentDeque = new ArrayDeque<>();
expandParamsFiles(argumentDeque, args);
parse(builder, argumentDeque);
parse(builder, expandArguments(args));
}

private static void parse(TurbineOptions.Builder builder, Deque<String> argumentDeque) {
Expand Down Expand Up @@ -175,6 +174,7 @@ private static void expandParamsFiles(Deque<String> argumentDeque, Iterable<Stri
if (arg.charAt(0) == '\'') {
// perform best-effort unescaping as a concession to ninja, see:
// https://android.googlesource.com/platform/external/ninja/+/6f903faaf5488dc052ffc4e3e0b12757b426e088/src/util.cc#274
// FIXME: do we need this? OptionParser in Bazel's javac compiler does not support this at all!
guw marked this conversation as resolved.
Show resolved Hide resolved
checkArgument(arg.charAt(arg.length() - 1) == '\'', arg);
arg = arg.substring(1, arg.length() - 1);
checkArgument(!arg.contains("'"), arg);
Expand All @@ -193,6 +193,45 @@ private static void expandParamsFiles(Deque<String> argumentDeque, Iterable<Stri
}
}

/**
* Pre-processes an argument list, expanding options @filename to read in the content of the file
* and add it to the list of arguments.
*
* @param args the List of arguments to pre-process.
* @return the List of pre-processed arguments.
* @throws java.io.IOException if one of the files containing options cannot be read.
*/
private static Deque<String> expandArguments(Iterable<String> args) throws IOException {
Deque<String> expanded = new ArrayDeque<>();
for (String arg : args) {
expandArgument(expanded, arg);
}
return expanded;
}

/**
* Expands a single argument, expanding options @filename to read in the content of the file and
* add it to the list of processed arguments. The @ itself can be escaped with @@.
*
* @param expanded the list of processed arguments.
* @param arg the argument to pre-process.
* @throws java.io.IOException if one of the files containing options cannot be read.
*/
private static void expandArgument(Deque<String> expanded, String arg) throws IOException {
// duplicated from https://github.com/bazelbuild/bazel/blob/9e7aa139ae33376cf1ccab205dda6608967f56bd/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/OptionsParser.java#L257
if (arg.startsWith("@@")) {
expanded.add(arg.substring(1));
} else if (arg.startsWith("@")) {
for (String line : Files.readAllLines(Paths.get(arg.substring(1)), UTF_8)) {
if (line.length() > 0) {
expandArgument(expanded, line);
}
}
} else {
expanded.add(arg);
}
}

/**
* Returns the value of an option, or throws {@link IllegalArgumentException} if the value is not
* present.
Expand Down
12 changes: 7 additions & 5 deletions javatests/com/google/turbine/options/TurbineOptionsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,21 @@
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.fail;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.turbine.options.TurbineOptions.ReducedClasspathMode;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Arrays;

import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.turbine.options.TurbineOptions.ReducedClasspathMode;

@RunWith(JUnit4.class)
public class TurbineOptionsTest {

Expand Down Expand Up @@ -193,7 +195,7 @@ public void optionalTargetLabel() throws Exception {
public void paramsFile() throws Exception {
Iterable<String> paramsArgs =
Iterables.concat(
BASE_ARGS, Arrays.asList("--javacopts", "-source", "8", "-target", "8", "--"));
BASE_ARGS, Arrays.asList("--javacopts", "-source", "8", "-target", "8","-Aconnector.opt=with,space, here", "--"));
Path params = tmpFolder.newFile("params.txt").toPath();
Files.write(params, paramsArgs, StandardCharsets.UTF_8);

Expand All @@ -206,7 +208,7 @@ public void paramsFile() throws Exception {
TurbineOptions options = TurbineOptionsParser.parse(Arrays.asList(lines));

// assert that options were read from params file
assertThat(options.javacOpts()).containsExactly("-source", "8", "-target", "8").inOrder();
assertThat(options.javacOpts()).containsExactly("-source", "8", "-target", "8", "-Aconnector.opt=with,space, here").inOrder();
// ... and directly from the command line
assertThat(options.targetLabel()).hasValue("//custom/label");
}
Expand Down