Fix wrong manipulation with ByteFragmentAtOffset
- the offset inside ByteFragmentAtOffset is the offset of the ByteBuffer from the beginning of the file, it is needed mostly for NinjaScope manipulations
- the offset inside ByteBufferFragment is the offset in the ByteBuffer, it points already at the place where lexing and parsing should begin
- add test for a big Ninja file and small ByteBuffer sizes used to read that file (the test is failing with the previous implementation)
Closes #10769.
PiperOrigin-RevId: 294685980
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/parser/NinjaParser.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/parser/NinjaParser.java
index 6517bf4..562f728 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/parser/NinjaParser.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/parser/NinjaParser.java
@@ -49,11 +49,7 @@
ByteBufferFragment fragment = byteFragmentAtOffset.getFragment();
int offset = byteFragmentAtOffset.getRealStartOffset();
- // Lexer should start at the beginning of byteFragmentAtOffset -> apply offset.
- ByteBufferFragment subFragment =
- fragment.subFragment(byteFragmentAtOffset.getOffset(), fragment.length());
- NinjaLexer lexer = new NinjaLexer(subFragment);
-
+ NinjaLexer lexer = new NinjaLexer(fragment);
if (!lexer.hasNextToken()) {
throw new IllegalStateException("Empty fragment passed as declaration.");
}
@@ -99,11 +95,21 @@
parseResult.addSubNinjaScope(declarationStart, subNinjaFuture);
break;
case BUILD:
- ByteFragmentAtOffset targetFragment =
- declarationStart == offset
- ? byteFragmentAtOffset
- // We pass an offset *inside the fragment* into ByteFragmentAtOffset constructor
- : new ByteFragmentAtOffset(declarationStart - fragment.getStartIncl(), fragment);
+ ByteFragmentAtOffset targetFragment;
+ if (declarationStart == offset) {
+ targetFragment = byteFragmentAtOffset;
+ } else {
+ // Method subFragment accepts only the offset *inside fragment*.
+ // So we should subtract the offset of fragment's buffer in file
+ // (byteFragmentAtOffset.getOffset()),
+ // and start of fragment inside buffer (fragment.getStartIncl()).
+ int fragmentStart =
+ declarationStart - byteFragmentAtOffset.getOffset() - fragment.getStartIncl();
+ targetFragment =
+ new ByteFragmentAtOffset(
+ byteFragmentAtOffset.getOffset(),
+ fragment.subFragment(fragmentStart, fragment.length()));
+ }
parseResult.addTarget(targetFragment);
break;
case DEFAULT:
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/pipeline/NinjaPipeline.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/pipeline/NinjaPipeline.java
index 98f8469..6463ea2 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/pipeline/NinjaPipeline.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/pipeline/NinjaPipeline.java
@@ -16,13 +16,13 @@
import static com.google.devtools.build.lib.concurrent.MoreFutures.waitForFutureAndGetWithCheckedException;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
-import com.google.devtools.build.lib.bazel.rules.ninja.file.ByteBufferFragment;
import com.google.devtools.build.lib.bazel.rules.ninja.file.ByteFragmentAtOffset;
import com.google.devtools.build.lib.bazel.rules.ninja.file.CollectingListFuture;
import com.google.devtools.build.lib.bazel.rules.ninja.file.GenericParsingException;
@@ -59,6 +59,7 @@
private final Collection<Path> includedOrSubninjaFiles;
private final String ownerTargetName;
private final Set<Path> childPaths;
+ private Integer readBlockSize;
/**
* @param basePath base path for resolving include and subninja paths.
@@ -116,15 +117,10 @@
for (ByteFragmentAtOffset byteFragmentAtOffset : targetFragments) {
future.add(
service.submit(
- () -> {
- // Lexer should start at the beginning of fragment -> apply offset.
- ByteBufferFragment bufferFragment = byteFragmentAtOffset.getFragment();
- ByteBufferFragment subFragment =
- bufferFragment.subFragment(
- byteFragmentAtOffset.getOffset(), bufferFragment.length());
- return new NinjaParserStep(new NinjaLexer(subFragment))
- .parseNinjaTarget(currentScope, byteFragmentAtOffset.getRealStartOffset());
- }));
+ () ->
+ new NinjaParserStep(new NinjaLexer(byteFragmentAtOffset.getFragment()))
+ .parseNinjaTarget(
+ currentScope, byteFragmentAtOffset.getRealStartOffset())));
}
queue.addAll(currentScope.getIncludedScopes());
queue.addAll(currentScope.getSubNinjaScopes());
@@ -171,6 +167,15 @@
}
}
+ /**
+ * Set the size of the block read by {@link ParallelFileProcessing}. Method is mainly intended to
+ * be used in tests.
+ */
+ @VisibleForTesting
+ public void setReadBlockSize(Integer readBlockSize) {
+ this.readBlockSize = readBlockSize;
+ }
+
private Path getChildNinjaPath(String rawText, String parentNinjaFileName)
throws GenericParsingException {
Path childPath = basePath.getRelative(rawText);
@@ -196,6 +201,9 @@
path.getBaseName()));
}
BlockParameters parameters = new BlockParameters(path.getFileSize());
+ if (readBlockSize != null) {
+ parameters.setReadBlockSize(readBlockSize);
+ }
return service.submit(
() -> {
try (ReadableByteChannel channel = path.createReadableByteChannel()) {
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/ninja/NinjaPipelineTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/ninja/NinjaPipelineTest.java
index 9198369..1ab3ed3 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/rules/ninja/NinjaPipelineTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/ninja/NinjaPipelineTest.java
@@ -23,9 +23,13 @@
import com.google.common.util.concurrent.MoreExecutors;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
import com.google.devtools.build.lib.bazel.rules.ninja.file.GenericParsingException;
+import com.google.devtools.build.lib.bazel.rules.ninja.parser.NinjaRule;
+import com.google.devtools.build.lib.bazel.rules.ninja.parser.NinjaRuleVariable;
import com.google.devtools.build.lib.bazel.rules.ninja.parser.NinjaTarget;
+import com.google.devtools.build.lib.bazel.rules.ninja.parser.NinjaVariableValue;
import com.google.devtools.build.lib.bazel.rules.ninja.pipeline.NinjaPipeline;
import com.google.devtools.build.lib.concurrent.ExecutorUtil;
+import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.DigestHashFunction;
import com.google.devtools.build.lib.vfs.DigestHashFunction.DefaultHashFunctionNotSetException;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -36,6 +40,7 @@
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.util.List;
+import java.util.Map;
import java.util.concurrent.Executors;
import org.junit.After;
import org.junit.Before;
@@ -257,6 +262,38 @@
+ "including 'one.ninja'.");
}
+ @Test
+ public void testBigFile() throws Exception {
+ String[] lines = new String[1000];
+ for (int i = 0; i < lines.length - 1; i++) {
+ lines[i] = "rule rule" + i + "\n command = echo 'Hello' > ${out}";
+ }
+ lines[999] = "build out: rule1";
+ Path path = tester.writeTmpFile("big_file.ninja", lines);
+ NinjaPipeline pipeline =
+ new NinjaPipeline(
+ path.getParentDirectory(), tester.getService(), ImmutableList.of(), "ninja_target");
+ // Test specifically that all manipulations with connecting buffers are working fine:
+ // for that, have relatively long file and small buffer size.
+ pipeline.setReadBlockSize(100);
+ List<NinjaTarget> targets = pipeline.pipeline(path);
+ assertThat(targets).hasSize(1);
+ Map<String, List<Pair<Integer, NinjaRule>>> rules = targets.get(0).getScope().getRules();
+ assertThat(rules).hasSize(999);
+ assertThat(rules.get("rule1")).hasSize(1);
+ NinjaVariableValue expectedValue =
+ NinjaVariableValue.builder().addText("echo 'Hello' > ").addVariable("out").build();
+ assertThat(
+ rules
+ .get("rule1")
+ .get(0)
+ .getSecond()
+ .getVariables()
+ .get(NinjaRuleVariable.COMMAND)
+ .getRawText())
+ .isEqualTo(expectedValue.getRawText());
+ }
+
private static void checkTargets(List<NinjaTarget> targets) {
assertThat(targets).hasSize(2);
for (NinjaTarget target : targets) {
