From 771590728e06b15a4dbf37f29c2460c5b99cdf5d Mon Sep 17 00:00:00 2001 From: Jesse Wilson Date: Mon, 25 Mar 2024 23:20:53 -0400 Subject: [PATCH 1/2] Fix a bug where outputs weren't deterministic This was only problematic when inputs weren't deterministic. We started seeing this with the changes to our Gradle setup. --- .../com/squareup/wire/schema/LinkerTest.kt | 51 ++++++++++++++++++- .../kotlin/com/squareup/wire/schema/Linker.kt | 4 ++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/wire-compiler/src/test/java/com/squareup/wire/schema/LinkerTest.kt b/wire-compiler/src/test/java/com/squareup/wire/schema/LinkerTest.kt index df836478f6..7cd2512970 100644 --- a/wire-compiler/src/test/java/com/squareup/wire/schema/LinkerTest.kt +++ b/wire-compiler/src/test/java/com/squareup/wire/schema/LinkerTest.kt @@ -19,6 +19,7 @@ package com.squareup.wire.schema import com.squareup.wire.testing.add import kotlin.test.assertFailsWith +import okio.ForwardingFileSystem import okio.Path import okio.Path.Companion.toPath import okio.fakefilesystem.FakeFileSystem @@ -480,10 +481,58 @@ class LinkerTest { assertThat(enumValueDeprecated!!.encodeMode).isNotNull() } + @Test + fun schemaIsDeterministicEvenIfProtoPathOrderIsNot() { + fs.add( + "source-path/a.proto", + """ + |message A { + |} + """.trimMargin(), + ) + fs.add( + "source-path/b.proto", + """ + |import "a.proto"; + |extend A { + | optional string b = 1; + |} + """.trimMargin(), + ) + fs.add( + "source-path/c.proto", + """ + |import "a.proto"; + |extend A { + | optional string c = 2; + |} + """.trimMargin(), + ) + + val schemaSorted = loadAndLinkSchema() + assertThat((schemaSorted.getType("A") as MessageType).extensionFields.map { it.name }) + .containsExactly("b", "c") + + val schemaReversed = loadAndLinkSchema(reverseSort = true) + assertThat((schemaReversed.getType("A") as MessageType).extensionFields.map { it.name }) + .containsExactly("b", "c") + } + private fun loadAndLinkSchema( opaqueTypes: List = listOf(), + reverseSort: Boolean = false, ): Schema { - val loader = SchemaLoader(fs) + val forwardingFileSystem = when { + reverseSort -> { + object : ForwardingFileSystem(fs) { + override fun listRecursively(dir: Path, followSymlinks: Boolean): Sequence = + super.listRecursively(dir, followSymlinks).toList().reversed().asSequence() + } + } + else -> fs + } + + val loader = SchemaLoader(forwardingFileSystem) loader.opaqueTypes = opaqueTypes loader.initRoots( sourcePath = listOf(Location.get("source-path")), diff --git a/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/Linker.kt b/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/Linker.kt index e5f587c5f0..56aad8c881 100644 --- a/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/Linker.kt +++ b/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/Linker.kt @@ -136,6 +136,10 @@ class Linker { } } + // The order of the input files shows up in the order of extension fields in the output files. + // Sort the inputs to get consistent output even when the order of input files is inconsistent. + sourceFiles.sortBy { it.protoFile.location.path } + for (fileLinker in sourceFiles) { fileLinker.requireTypesRegistered() } From 24a10e8c04ff677a7adf2c5c926593245a42bcf5 Mon Sep 17 00:00:00 2001 From: Jesse Wilson Date: Mon, 25 Mar 2024 23:23:53 -0400 Subject: [PATCH 2/2] Rename a local --- .../src/test/java/com/squareup/wire/schema/LinkerTest.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/wire-compiler/src/test/java/com/squareup/wire/schema/LinkerTest.kt b/wire-compiler/src/test/java/com/squareup/wire/schema/LinkerTest.kt index 7cd2512970..aba242a899 100644 --- a/wire-compiler/src/test/java/com/squareup/wire/schema/LinkerTest.kt +++ b/wire-compiler/src/test/java/com/squareup/wire/schema/LinkerTest.kt @@ -522,7 +522,7 @@ class LinkerTest { opaqueTypes: List = listOf(), reverseSort: Boolean = false, ): Schema { - val forwardingFileSystem = when { + val schemaLoaderFileSystem = when { reverseSort -> { object : ForwardingFileSystem(fs) { override fun listRecursively(dir: Path, followSymlinks: Boolean): Sequence = @@ -532,7 +532,7 @@ class LinkerTest { else -> fs } - val loader = SchemaLoader(forwardingFileSystem) + val loader = SchemaLoader(schemaLoaderFileSystem) loader.opaqueTypes = opaqueTypes loader.initRoots( sourcePath = listOf(Location.get("source-path")),