Fix parsing the fragments with Ninja declarations to start from offset
The was a problem that we created NinjaLexer without paying attention to the inner offset in ByteFragmentAtOffset.
There was additionally a problem that we recorded build statement to start at the wrong offset.
Add test for the Ninja file with newlines.
Closes #10722.
PiperOrigin-RevId: 293781612
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 38894e7..6517bf4 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,7 +49,10 @@
ByteBufferFragment fragment = byteFragmentAtOffset.getFragment();
int offset = byteFragmentAtOffset.getRealStartOffset();
- NinjaLexer lexer = new NinjaLexer(fragment);
+ // Lexer should start at the beginning of byteFragmentAtOffset -> apply offset.
+ ByteBufferFragment subFragment =
+ fragment.subFragment(byteFragmentAtOffset.getOffset(), fragment.length());
+ NinjaLexer lexer = new NinjaLexer(subFragment);
if (!lexer.hasNextToken()) {
throw new IllegalStateException("Empty fragment passed as declaration.");
@@ -99,7 +102,8 @@
ByteFragmentAtOffset targetFragment =
declarationStart == offset
? byteFragmentAtOffset
- : new ByteFragmentAtOffset(declarationStart, fragment);
+ // We pass an offset *inside the fragment* into ByteFragmentAtOffset constructor
+ : new ByteFragmentAtOffset(declarationStart - fragment.getStartIncl(), fragment);
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 48b84e5..98f8469 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
@@ -22,6 +22,7 @@
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;
@@ -112,12 +113,18 @@
NinjaScope currentScope = queue.removeFirst();
List<ByteFragmentAtOffset> targetFragments = rawTargets.get(currentScope);
Preconditions.checkNotNull(targetFragments);
- for (ByteFragmentAtOffset fragment : targetFragments) {
+ for (ByteFragmentAtOffset byteFragmentAtOffset : targetFragments) {
future.add(
service.submit(
- () ->
- new NinjaParserStep(new NinjaLexer(fragment.getFragment()))
- .parseNinjaTarget(currentScope, fragment.getRealStartOffset())));
+ () -> {
+ // 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());
+ }));
}
queue.addAll(currentScope.getIncludedScopes());
queue.addAll(currentScope.getSubNinjaScopes());
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 d35b7b2..9198369 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
@@ -117,6 +117,33 @@
}
@Test
+ public void testOneFilePipelineWithNewlines() throws Exception {
+ Path vfsPath =
+ tester.writeTmpFile(
+ "test.ninja",
+ "",
+ "",
+ "",
+ "",
+ "rule r1",
+ " command = c $in $out",
+ "",
+ "",
+ "",
+ "build t1: r1 in1 in2",
+ "",
+ "",
+ "",
+ "build t2: r1 in3",
+ "");
+ NinjaPipeline pipeline =
+ new NinjaPipeline(
+ vfsPath.getParentDirectory(), tester.getService(), ImmutableList.of(), "ninja_target");
+ List<NinjaTarget> targets = pipeline.pipeline(vfsPath);
+ checkTargets(targets);
+ }
+
+ @Test
public void testWithIncluded() throws Exception {
Path vfsPath =
tester.writeTmpFile(
