Skip to content

Commit 79285d1

Browse files
jupblbjupblb
committed
Fix CI: dotted nested flags, snapshot drift, Docker coords, formatting
1 parent 793ae4f commit 79285d1

9 files changed

Lines changed: 195 additions & 38 deletions

File tree

‎Dockerfile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ RUN git config --global --add safe.directory *
1818
COPY . .
1919

2020
RUN sbt publishLocal dumpScipJavaVersion
21-
RUN mkdir -p /app && coursier bootstrap "com.sourcegraph:scip-java_2.13:$(cat VERSION)" -f -o /app/scip-java -M com.sourcegraph.scip_java.ScipJava
21+
RUN mkdir -p /app && coursier bootstrap "com.sourcegraph:scip-java:$(cat VERSION)" -f -o /app/scip-java -M com.sourcegraph.scip_java.ScipJava
2222

2323
COPY ./bin/scip-java-docker-script.sh /usr/bin/scip-java
2424

‎build.sbt‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -298,9 +298,8 @@ lazy val cli = project
298298
// Kotlin/Java-only (and the generated class is straightforward to consume
299299
// from Kotlin).
300300
lazy val scipJavaCliBuildInfoGenerator = Def.task {
301-
val out =
302-
(Compile / sourceManaged).value / "com" / "sourcegraph" / "scip_java" /
303-
"BuildInfo.java"
301+
val out = (Compile / sourceManaged).value / "com" / "sourcegraph" /
302+
"scip_java" / "BuildInfo.java"
304303
IO.createDirectory(out.getParentFile)
305304
val optionsLiteral = javacModuleOptions
306305
.map(javaStringLiteral)
@@ -326,13 +325,20 @@ lazy val scipJavaCliBuildInfoGenerator = Def.task {
326325

327326
def javaStringLiteral(value: String): String = {
328327
val escaped = value.flatMap {
329-
case '\\' => "\\\\"
330-
case '"' => "\\\""
331-
case '\n' => "\\n"
332-
case '\r' => "\\r"
333-
case '\t' => "\\t"
334-
case c if c.isControl => f"\\u${c.toInt}%04x"
335-
case c => c.toString
328+
case '\\' =>
329+
"\\\\"
330+
case '"' =>
331+
"\\\""
332+
case '\n' =>
333+
"\\n"
334+
case '\r' =>
335+
"\\r"
336+
case '\t' =>
337+
"\\t"
338+
case c if c.isControl =>
339+
f"\\u${c.toInt}%04x"
340+
case c =>
341+
c.toString
336342
}
337343
"\"" + escaped + "\""
338344
}

‎scip-java/src/main/kotlin/com/sourcegraph/scip_java/ScipJavaApp.kt‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ class ScipJavaApp {
7474

7575
fun run(args: List<String>): Int {
7676
reporter.reset()
77-
val processedArgs = applyGlobalCwd(args)
77+
val processedArgs = applyGlobalCwd(rewriteNestedOptions(args))
7878
val root = RootCommand(this)
7979
root.subcommands(IndexCommand(), AggregateCommand(), SnapshotCommand())
8080
return try {
@@ -103,6 +103,30 @@ class ScipJavaApp {
103103
if (exit != 0) kotlin.system.exitProcess(exit)
104104
}
105105

106+
/**
107+
* The previous moped CLI accepted nested subcommand options written as
108+
* `--aggregate.<flag>` on `scip-java index` (e.g. the Bazel aspect
109+
* passes `--aggregate.allow-empty-index`). clikt forbids `.` in
110+
* option names, so we rewrite the dotted prefix to a `-` separator to match
111+
* the options declared on [IndexCommand]. Tokens after a `--` separator are
112+
* left untouched.
113+
*/
114+
private fun rewriteNestedOptions(args: List<String>): List<String> {
115+
var sawDoubleDash = false
116+
return args.map { arg ->
117+
when {
118+
sawDoubleDash -> arg
119+
arg == "--" -> {
120+
sawDoubleDash = true
121+
arg
122+
}
123+
arg.startsWith("--aggregate.") ->
124+
"--aggregate-" + arg.removePrefix("--aggregate.")
125+
else -> arg
126+
}
127+
}
128+
}
129+
106130
/**
107131
* `--cwd` is a global flag that, unlike a regular clikt parent option, may
108132
* appear in any position (including after the subcommand name), mirroring

‎scip-java/src/main/kotlin/com/sourcegraph/scip_java/ScipPrinters.kt‎

Lines changed: 96 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ object ScipPrinters {
3232
doc.occurrencesList.groupBy { it.getRange(0) }
3333
val symtab: Map<String, SymbolInformation> =
3434
doc.symbolsList.associateBy { it.symbol }
35+
val input = SourceInput(text)
3536

3637
val syntheticDefinitions: Map<String, List<SymbolInformation>> =
3738
doc.symbolsList
@@ -68,10 +69,10 @@ object ScipPrinters {
6869
val occurrences =
6970
(occurrencesByLine[i] ?: emptyList()).sortedWith(occurrenceOrdering)
7071
for (occ in occurrences) {
71-
formatOccurrence(out, occ, line, symtab, null)
72+
formatOccurrence(input, out, occ, line, symtab, null)
7273
if ((occ.symbolRoles and SymbolRole.Definition_VALUE) > 0) {
7374
syntheticDefinitions[occ.symbol]?.forEach { syntheticDefinition ->
74-
formatOccurrence(out, occ, line, symtab, syntheticDefinition)
75+
formatOccurrence(input, out, occ, line, symtab, syntheticDefinition)
7576
}
7677
}
7778
}
@@ -120,21 +121,57 @@ object ScipPrinters {
120121
val endColumn: Int,
121122
)
122123

123-
private fun positionOf(occ: Occurrence): OccurrencePos =
124+
/**
125+
* Faithful port of `moped.reporters.Position.range` + `RangePosition`:
126+
* the SCIP `(line, column)` pair is converted to a flat character offset
127+
* and back. The round-trip matters for occurrences whose end column
128+
* overflows its start line (e.g. Kotlin `companion object` definitions),
129+
* where the overflow "carries" the end onto a later line, turning a raw
130+
* single-line range into a rendered multi-line range.
131+
*/
132+
private fun positionOf(input: SourceInput, occ: Occurrence): OccurrencePos {
133+
val rawStartLine: Int
134+
val rawStartColumn: Int
135+
val rawEndLine: Int
136+
val rawEndColumn: Int
124137
when (occ.rangeCount) {
125-
3 -> OccurrencePos(occ.getRange(0), occ.getRange(1), occ.getRange(0), occ.getRange(2))
126-
4 -> OccurrencePos(occ.getRange(0), occ.getRange(1), occ.getRange(2), occ.getRange(3))
138+
3 -> {
139+
rawStartLine = occ.getRange(0)
140+
rawStartColumn = occ.getRange(1)
141+
rawEndLine = occ.getRange(0)
142+
rawEndColumn = occ.getRange(2)
143+
}
144+
4 -> {
145+
rawStartLine = occ.getRange(0)
146+
rawStartColumn = occ.getRange(1)
147+
rawEndLine = occ.getRange(2)
148+
rawEndColumn = occ.getRange(3)
149+
}
127150
else -> throw IllegalArgumentException("Invalid range: $occ")
128151
}
152+
// moped's Position.range returns NoPosition (all -1) for empty input.
153+
if (input.isEmpty) return OccurrencePos(-1, -1, -1, -1)
154+
val start = input.lineToOffset(rawStartLine) + rawStartColumn
155+
val end = input.lineToOffset(rawEndLine) + rawEndColumn
156+
val startLine = input.offsetToLine(start)
157+
val endLine = input.offsetToLine(end)
158+
return OccurrencePos(
159+
startLine = startLine,
160+
startColumn = start - input.lineToOffset(startLine),
161+
endLine = endLine,
162+
endColumn = end - input.lineToOffset(endLine),
163+
)
164+
}
129165

130166
private fun formatOccurrence(
167+
input: SourceInput,
131168
out: StringBuilder,
132169
occ: Occurrence,
133170
line: String,
134171
symtab: Map<String, SymbolInformation>,
135172
syntheticDefinition: SymbolInformation?,
136173
) {
137-
val pos = positionOf(occ)
174+
val pos = positionOf(input, occ)
138175
val isMultiline = pos.startLine != pos.endLine
139176
val width =
140177
if (isMultiline) line.length - pos.startColumn - 1
@@ -239,4 +276,57 @@ object ScipPrinters {
239276
if (start < text.length) result += text.substring(start)
240277
return result
241278
}
279+
280+
/**
281+
* Port of `moped.reporters.Input`'s offset/line bookkeeping. Only the
282+
* pieces needed by [positionOf] are reproduced ([lineToOffset],
283+
* [offsetToLine], [isEmpty]).
284+
*/
285+
private class SourceInput(text: String) {
286+
private val chars: CharArray = text.toCharArray()
287+
288+
val isEmpty: Boolean = text.isEmpty()
289+
290+
// Offset of the first character of each line; a trailing sentinel
291+
// (== chars.size) is appended when the text does not end in '\n'.
292+
private val lineIndices: IntArray = run {
293+
val buf = ArrayList<Int>()
294+
buf.add(0)
295+
var i = 0
296+
while (i < chars.size) {
297+
if (chars[i] == '\n') buf.add(i + 1)
298+
i++
299+
}
300+
if (buf[buf.size - 1] != chars.size) buf.add(chars.size)
301+
buf.toIntArray()
302+
}
303+
304+
fun lineToOffset(line: Int): Int {
305+
require(line in 0..(lineIndices.size - 1)) {
306+
"$line is not a valid line number, allowed [0..${lineIndices.size - 1}]"
307+
}
308+
return lineIndices[line]
309+
}
310+
311+
fun offsetToLine(offset: Int): Int {
312+
require(offset in 0..chars.size) {
313+
"$offset is not a valid offset, allowed [0..${chars.size}]"
314+
}
315+
// File ending in '\n': an offset at EOF is last_line+1:0.
316+
if (offset == chars.size && chars.isNotEmpty() && chars[offset - 1] == '\n') {
317+
return lineIndices.size - 1
318+
}
319+
var lo = 0
320+
var hi = lineIndices.size - 1
321+
while (hi - lo > 1) {
322+
val mid = (hi + lo) / 2
323+
when {
324+
offset < lineIndices[mid] -> hi = mid
325+
lineIndices[mid] == offset -> return mid
326+
else -> lo = mid
327+
}
328+
}
329+
return lo
330+
}
331+
}
242332
}

‎scip-java/src/main/kotlin/com/sourcegraph/scip_java/buildtools/BuildTool.kt‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,10 +55,11 @@ abstract class BuildTool(val name: String, protected val index: IndexCommand) {
5555
output = index.finalOutput,
5656
targetroots = listOf(targetroot),
5757
app = index.app,
58-
parallel = true,
59-
emitInverseRelationships = true,
60-
allowEmptyIndex = false,
61-
allowExportingGlobalSymbolsFromDirectoryEntries = true,
58+
parallel = index.aggregateParallel,
59+
emitInverseRelationships = index.aggregateEmitInverseRelationships,
60+
allowEmptyIndex = index.aggregateAllowEmptyIndex,
61+
allowExportingGlobalSymbolsFromDirectoryEntries =
62+
index.aggregateAllowExportingGlobalSymbolsFromDirectoryEntries,
6263
)
6364
}
6465
}

‎scip-java/src/main/kotlin/com/sourcegraph/scip_java/commands/IndexCommand.kt‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,39 @@ class IndexCommand : CliktCommand(name = "index") {
139139
help = "Optional. The build command to use to compile all sources. Defaults to a build-specific command.",
140140
).multiple()
141141

142+
// Forwarded options for the embedded `aggregate` step. The previous
143+
// moped CLI exposed the nested `aggregate` command's flags on
144+
// `scip-java index` using the `--aggregate.<flag>` naming (e.g. the
145+
// Bazel aspect passes `--aggregate.allow-empty-index`). clikt forbids
146+
// `.` in option names, so these are registered with a `-` separator and the
147+
// dotted form is rewritten to it during argument preprocessing
148+
// (ScipJavaApp.run). Consumed by BuildTool.generateScipFromTargetroot.
149+
val aggregateParallel: Boolean by option(
150+
"--aggregate-parallel",
151+
"--aggregate-no-parallel",
152+
hidden = true,
153+
).flag("--aggregate-no-parallel", default = true)
154+
155+
val aggregateEmitInverseRelationships: Boolean by option(
156+
"--aggregate-emit-inverse-relationships",
157+
"--aggregate-no-emit-inverse-relationships",
158+
hidden = true,
159+
).flag("--aggregate-no-emit-inverse-relationships", default = true)
160+
161+
val aggregateAllowEmptyIndex: Boolean by option(
162+
"--aggregate-allow-empty-index",
163+
hidden = true,
164+
).flag()
165+
166+
val aggregateAllowExportingGlobalSymbolsFromDirectoryEntries: Boolean by option(
167+
"--aggregate-allow-exporting-global-symbols-from-directory-entries",
168+
"--aggregate-no-allow-exporting-global-symbols-from-directory-entries",
169+
hidden = true,
170+
).flag(
171+
"--aggregate-no-allow-exporting-global-symbols-from-directory-entries",
172+
default = true,
173+
)
174+
142175
val workingDirectory: Path
143176
get() = AbsolutePath.of(app.env.workingDirectory)
144177

‎tests/snapshots/src/main/scala/tests/SaveSnapshotHandler.scala‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,12 @@ import java.nio.charset.StandardCharsets
44
import java.nio.file.Files
55
import java.nio.file.Path
66
import java.util.concurrent.ConcurrentLinkedDeque
7+
import java.util.function.Predicate
78

89
import scala.jdk.CollectionConverters._
910

1011
import com.sourcegraph.io.DeleteVisitor
1112

12-
import java.util.function.Predicate
13-
1413
class SaveSnapshotHandler extends SnapshotHandler {
1514
private val writtenTests = new ConcurrentLinkedDeque[Path]()
1615
override def onSnapshotTest(
@@ -28,12 +27,8 @@ class SaveSnapshotHandler extends SnapshotHandler {
2827
return
2928
}
3029
val isWritten = writtenTests.asScala.toSet
31-
val keepWritten: Predicate[Path] =
32-
(file: Path) => !isWritten.contains(file)
33-
Files.walkFileTree(
34-
context.expectDirectory,
35-
new DeleteVisitor(keepWritten)
36-
)
30+
val keepWritten: Predicate[Path] = (file: Path) => !isWritten.contains(file)
31+
Files.walkFileTree(context.expectDirectory, new DeleteVisitor(keepWritten))
3732
}
3833

3934
}

‎tests/unit/src/main/scala/tests/FileLayout.scala‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,10 @@ object FileLayout {
7575
row.stripPrefix("\n").split("\n", 2).toList match {
7676
case path :: contents :: Nil =>
7777
val withEndOfFileLine =
78-
if (contents.endsWith("\n")) contents else contents + "\n"
78+
if (contents.endsWith("\n"))
79+
contents
80+
else
81+
contents + "\n"
7982
path.stripPrefix("/") -> withEndOfFileLine
8083
case other =>
8184
throw new IllegalArgumentException(

‎tests/unit/src/main/scala/tests/ScipJavaSuite.scala‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,9 @@ import munit.internal.console.AnsiColors
2121
* into an in-memory buffer (exposed via [[ApplicationFixture#capturedOutput]])
2222
* and a fresh per-test working/cache directory layout.
2323
*
24-
* Mirrors the surface area of `moped.testkit.MopedSuite` that the test
25-
* suites in this repository actually use, without requiring the
26-
* Scala-only moped runtime.
24+
* Mirrors the surface area of `moped.testkit.MopedSuite` that the test suites
25+
* in this repository actually use, without requiring the Scala-only moped
26+
* runtime.
2727
*/
2828
abstract class ScipJavaSuite(applicationToTest: ScipJavaApp) extends FunSuite {
2929

@@ -54,7 +54,10 @@ abstract class ScipJavaSuite(applicationToTest: ScipJavaApp) extends FunSuite {
5454
extends Fixture[ScipJavaApp]("Application") {
5555
private val out = new ByteArrayOutputStream
5656
private val ps =
57-
new PrintStream(out, /* autoFlush = */ true, StandardCharsets.UTF_8.name())
57+
new PrintStream(
58+
out, /* autoFlush = */ true,
59+
StandardCharsets.UTF_8.name()
60+
)
5861

5962
// NOTE: every invocation gets its own freshly-instrumented [[ScipJavaApp]]
6063
// (all of them writing into the shared `out` buffer of this fixture) so
@@ -79,8 +82,7 @@ abstract class ScipJavaSuite(applicationToTest: ScipJavaApp) extends FunSuite {
7982

8083
def reset(): Unit = out.reset()
8184

82-
def capturedRawOutput: String =
83-
out.toString(StandardCharsets.UTF_8.name())
85+
def capturedRawOutput: String = out.toString(StandardCharsets.UTF_8.name())
8486

8587
def capturedOutput: String = AnsiColors.filterAnsi(capturedRawOutput)
8688

@@ -131,7 +133,10 @@ abstract class ScipJavaSuite(applicationToTest: ScipJavaApp) extends FunSuite {
131133
val sanitized = obtained.replace(temporaryDirectory().toString(), "")
132134
// Workaround for https://github.com/scalameta/munit/issues/179
133135
super.assertNoDiff(
134-
if (sanitized == "\n") "" else sanitized,
136+
if (sanitized == "\n")
137+
""
138+
else
139+
sanitized,
135140
expected,
136141
clue
137142
)

0 commit comments

Comments
 (0)