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

V2: Update to google protobuf v27.0-rc1 #799

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

timostamm
Copy link
Member

This updates to the release candidate v27.0-rc1 for the well-known types, the plugin API, and conformance tests.

Comment on lines +38 to +40
import { fileDesc_google_protobuf_test_messages_edition2023 } from "./gen/google/protobuf/test_messages_edition2023_pb.js";
import { fileDesc_google_protobuf_test_messages_proto2_editions } from "./gen/google/protobuf/test_messages_proto2_editions_pb.js";
import { fileDesc_google_protobuf_test_messages_proto3_editions } from "./gen/google/protobuf/test_messages_proto3_editions_pb.js";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conformance tests cover editions with two files that are configured to behave the same as the proto2 and proto3 test files, and a separate file specific to editions.

We're passing all tests for JSON and binary serialization, and continue to skip all tests for the text-format, which we do not implement.

@@ -9,8 +9,8 @@
"generate": "protoc --es_out=src/gen --es_opt=ts_nocheck=false,target=ts --proto_path=$(upstream-include conformance) $(upstream-files conformance)",
"postgenerate": "license-header src/gen",
"test": "npm run test:bigint && npm run test:string",
"test:bigint": "BUF_BIGINT_DISABLE=0 conformance_test_runner --enforce_recommended --failure_list failing_tests_with_bigint.txt --text_format_failure_list failing_tests_text_format.txt bin/conformance.js",
"test:string": "BUF_BIGINT_DISABLE=1 conformance_test_runner --enforce_recommended --failure_list failing_tests_without_bigint.txt --text_format_failure_list failing_tests_text_format.txt bin/conformance.js"
"test:bigint": "BUF_BIGINT_DISABLE=0 conformance_test_runner --maximum_edition MAX --enforce_recommended --failure_list failing_tests_with_bigint.txt --text_format_failure_list failing_tests_text_format.txt bin/conformance.js",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refers to the enum value google.protobuf.Edition.EDITION_MAX, and runs tests with unbounded edition support. This makes sure we don't miss a test.

@@ -8,8 +8,8 @@
"build:copy-gen-js": "rsync -a --exclude '*.js' src/gen/js dist/types/gen && rsync -a --exclude '*.d.ts' src/gen/js dist/esm/gen",
"pregenerate": "rm -rf src/gen/*/* descriptorset.*",
"generate": "npm run generate:ts && npm run generate:js",
"generate:ts": "protoc --experimental_editions --es_out=src/gen/ts --es_opt=ts_nocheck=false,target=ts --proto_path=. $(buf ls-files extra) --proto_path=$(upstream-include test) $(upstream-files test) google/protobuf/type.proto",
"generate:js": "protoc --experimental_editions --es_out=src/gen/js --es_opt=ts_nocheck=false,target=js+dts --proto_path=. $(buf ls-files extra) --proto_path=$(upstream-include test) $(upstream-files test) google/protobuf/type.proto",
"generate:ts": "protoc --es_out=src/gen/ts --es_opt=ts_nocheck=false,target=ts --proto_path=. $(buf ls-files extra) --proto_path=$(upstream-include test) $(upstream-files test) google/protobuf/type.proto",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Editions are no longer experimental, and we can remove the flag --experimental_editions here (and at several other places).

Comment on lines +108 to +110
if (f.options) {
clearField(FieldOptionsDesc, f.options, "featureSupport");
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We embed the file descriptor for google/protobuf/descriptor.proto, and cannot use binary serialization during bootstrap. Since the new field google.protobuf.FieldOptions.feature_support is not relevant for serialization, we drop it.

Comment on lines +161 to +166
const r = reflect(
FeatureSetDesc,
featureSetHasAllSet(def.overridableFeatures)
? def.overridableFeatures
: def.fixedFeatures,
);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message google.protobuf.FeatureSetDefaults.FeatureSetEditionDefault has changed in v27.0-rc1: The field features is replaced by the fields overridable_features and fixed_features, so we have to update our script
that embeds edition defaults.

This was caught by our assertions, so the script is doing its job.

Comment on lines +24 to +40
/*
import {create, toBinary} from "@bufbuild/protobuf";
import {CodeGeneratorResponse_Feature, CodeGeneratorResponseDesc, Edition} from "@bufbuild/protobuf/wkt";
const res = create(CodeGeneratorResponseDesc, {
supportedFeatures: BigInt(
CodeGeneratorResponse_Feature.SUPPORTS_EDITIONS | CodeGeneratorResponse_Feature.PROTO3_OPTIONAL
),
minimumEdition: Edition.EDITION_LEGACY,
maximumEdition: Edition.EDITION_MAX,
});
console.log(
toBinary(CodeGeneratorResponseDesc, res)
);
*/
const minimalResponse = new Uint8Array([
16, 3, 24, 132, 7, 32, 255, 255, 255, 255, 7,
]);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script is used internally to simulate plugin invocations. We simply dump the code generator request.

protoc v27.0-rc1 starts honoring the fields minimum_edition and maximum_edition of google.protobuf.compiler.CodeGeneratorResponse, and errors if a file uses an edition that the plugin does not advertise as supported.

We don't want to use our runtime here to create a response, because we use this code to test our runtime. So we manually create a response that advertises support for all editions, using the range Edition.EDITION_LEGACY to Edition.EDITION_MAX, and use the binary data literally here.

Comment on lines +75 to +77
"!src/google/protobuf/map_unittest.proto",
"!src/google/protobuf/map_lite_unittest.proto",
"!src/google/protobuf/unittest_lite.proto",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these test protos from upstream are useful to us, and we ignore them.

Comment on lines +408 to +412
let archiveVersion = this.#version;
const rcMatch = /^(\d+\.\d+-rc)(\d)$/.exec(archiveVersion);
if (rcMatch != null) {
archiveVersion = rcMatch[1] + "-" + rcMatch[2];
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream release candidates use the two forms "rc1" and "rc-1" in their URL.

Copy link
Member

@smaye81 smaye81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the helpful comments on the changes 👍

@timostamm timostamm merged commit 91ac90f into v2 Apr 22, 2024
5 checks passed
@timostamm timostamm deleted the tstamm/update-google-protobuf-27.0-rc1 branch April 22, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants