Skip to content

Add dart_package option #298

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add dart_package option #298

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a4c3d98
first attempt
sigurdm Sep 27, 2019
e86f5f8
more tests
sigurdm Oct 2, 2019
3e3d5fb
Avoid using .dart for golden files
sigurdm Oct 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 24 additions & 9 deletions protoc_plugin/lib/file_generator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ class FileGenerator extends ProtobufContainer {
final FileDescriptorProto descriptor;
final GenerationOptions options;

// The relative path used to import the .proto file, as a URI.
// The path used to import the .proto file, as a URI.
// This is a package: URI if a `dart_package` option is given.
// Otherwise this is a relative path.
final Uri protoFileUri;

final enumGenerators = <EnumGenerator>[];
Expand All @@ -130,12 +132,22 @@ class FileGenerator extends ProtobufContainer {
/// True if cross-references have been resolved.
bool _linked = false;

FileGenerator(this.descriptor, this.options)
: protoFileUri = Uri.file(descriptor.name) {
if (protoFileUri.isAbsolute) {
// protoc should never generate an import with an absolute path.
throw "FAILURE: Import with absolute path is not supported";
static Uri calculateUri(FileDescriptorProto descriptor) {
if (descriptor.options.hasExtension(Dart_options.dartPackage)) {
String dartPackage =
descriptor.options.getExtension(Dart_options.dartPackage);
return Uri(scheme: 'package', path: '${dartPackage}/${descriptor.name}');
Copy link

@dave dave Nov 3, 2020

Choose a reason for hiding this comment

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

Hey @sigurdm I've been having a look at this, and I think this should be:

return Uri(scheme: 'package', path: '${dartPackage}/${path.basename(descriptor.name)}');

descriptor.name returns the full path of the proto file.

In my project the proto root is not the same as the package directory, so I get something like:

dartPackage: "foo/bar"
descriptor.name: "bar/baz.proto"

Combining them together gives "foo/bar/bar/baz.proto" when it should be "foo/bar/baz.proto".

I believe it would be better to ignore the path from the descriptor because the dart package name should always point directly to the directory that contains the dart source file. Right?

Copy link
Collaborator Author

@sigurdm sigurdm Nov 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I believe the dart package should not have slashes. It should just be a single name (the name of the package where the proto belongs)

Copy link

@dave dave Nov 3, 2020

Choose a reason for hiding this comment

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

ooh interesting... That would be tricky in my project... I have one directory structure with several dart packages, and they all share a common proto root.

Copy link

@dave dave Nov 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah it seems like an unnecessary restriction that your proto file structure should be the same as your dart package structure. Let me explain my project structure:

+ project root
  + server (go packages)
    + main 
    + repo1
    + repo2
    + ...
    + tests (go tests)
  + lib (main dart package)
  + repositories (other dart packages)
    + repo1
    + repo2
    + ...
  + proto (proto root)
    + main (proto files for main)
    + repo1 (proto files for repo1)
    + repo2 (proto files for repo2)
    + ...
  + test (dart tests)

It's convenient to have the proto files all in the same directory structure, but the file structure of the Dart packages doesn't match this. I'm trying to ensure my proto files stay platform independent, so I don't really want to rearrange my proto files to match my Dart directory structure. Nor do I really want to have a separate proto root for each package.

Copy link
Collaborator Author

@sigurdm sigurdm Nov 3, 2020

Choose a reason for hiding this comment

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

I think it makes sense to have one proto-root per package, if the protos truly belongs in that package. (At least the generated files have to be moved into lib/ of that package.

Copy link

@dave dave Nov 3, 2020

Choose a reason for hiding this comment

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

OK I'll have a go at rearranging my project / protoc commands. I'll let you know how I get on!

} else {
return Uri.file(descriptor.name);
}
}

FileGenerator(this.descriptor, this.options)
: protoFileUri = calculateUri(descriptor) {
// if (protoFileUri.isAbsolute) {
Copy link
Member

@szakarias szakarias Sep 30, 2019

Choose a reason for hiding this comment

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

Remove?

// // protoc should never generate an import with an absolute path.
// throw "FAILURE: Import with absolute path is not supported";
// }

var declaredMixins = _getDeclaredMixins(descriptor);
var defaultMixinName =
Expand Down Expand Up @@ -194,15 +206,18 @@ class FileGenerator extends ProtobufContainer {
FileGenerator get fileGen => this;
List<int> get fieldPath => [];

Uri outputFile(OutputConfiguration config, String extension) {
Uri protoUrl = Uri.file(descriptor.name);
return config.outputPathFor(protoUrl, extension);
}

/// Generates all the Dart files for this .proto file.
List<CodeGeneratorResponse_File> generateFiles(OutputConfiguration config) {
if (!_linked) throw StateError("not linked");

makeFile(String extension, String content) {
Uri protoUrl = Uri.file(descriptor.name);
Uri dartUrl = config.outputPathFor(protoUrl, extension);
return CodeGeneratorResponse_File()
..name = dartUrl.path
..name = outputFile(config, extension).path
..content = content;
}

Expand Down
6 changes: 6 additions & 0 deletions protoc_plugin/lib/src/dart_options.pb.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions protoc_plugin/protos/dart_options.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ message Imports {
}

extend google.protobuf.FileOptions {
// The Dart package that should be used when importing code generated from
// this file.
optional string dart_package = 28205290;

optional Imports imports = 28125061;

Expand Down
117 changes: 117 additions & 0 deletions protoc_plugin/test/file_generator_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
library file_generator_test;

import 'package:protoc_plugin/indenting_writer.dart';
import 'package:protoc_plugin/src/dart_options.pb.dart';
import 'package:protoc_plugin/src/descriptor.pb.dart';
import 'package:protoc_plugin/src/plugin.pb.dart';
import 'package:protoc_plugin/protoc.dart';
Expand Down Expand Up @@ -405,4 +406,120 @@ void main() {
expectMatchesGoldenFile(
fg.generateEnumFile().toString(), 'test/goldens/imports.pbjson');
});

test('FileGenerator generates correct dart package imports', () {
// This defines a.proto, same_package.proto, other_package.proto,
// no_package.proto
//
// a.proto:
// ---------------
// import "dart_options.proto";
// option (dart_options.dart_package) = 'abc';
// message A {
// }
//
// same_package.proto:
// ---------------
// import "a.proto";
// import "dart_options.proto";
// option (dart_options.dart_package) = 'abc';
//
// message B {
// optional A a = 1;
// }
//
// other_package.proto:
// ---------------
// import "a.proto"
// import "dart_options.proto";
// option (dart_options.dart_package) = 'def';
// message C {
// optional A a = 1;
// }
//
// no_package.proto:
// ---------------
// import "a.proto"
// message D {
// optional A a = 1;
// }

DescriptorProto mA = DescriptorProto()..name = 'A';
FileDescriptorProto fda = FileDescriptorProto()
..name = 'a.proto'
..messageType.add(mA)
..options =
(FileOptions()..setExtension(Dart_options.dartPackage, 'abc'));

DescriptorProto mB = DescriptorProto()
..name = 'B'
..field.addAll([
FieldDescriptorProto()
..name = 'a'
..jsonName = 'a'
..number = 1
..label = FieldDescriptorProto_Label.LABEL_OPTIONAL
..type = FieldDescriptorProto_Type.TYPE_MESSAGE
..typeName = ".A",
]);
FileDescriptorProto fdSamePackage = FileDescriptorProto()
..name = 'same_package.proto'
..messageType.add(mB)
..options =
(FileOptions()..setExtension(Dart_options.dartPackage, 'abc'));

DescriptorProto mC = DescriptorProto()
..name = 'C'
..field.addAll([
FieldDescriptorProto()
..name = 'a'
..jsonName = 'a'
..number = 1
..label = FieldDescriptorProto_Label.LABEL_OPTIONAL
..type = FieldDescriptorProto_Type.TYPE_MESSAGE
..typeName = ".A",
]);
FileDescriptorProto fdOtherPackage = FileDescriptorProto()
..name = 'other_package.proto'
..messageType.add(mC)
..options =
(FileOptions()..setExtension(Dart_options.dartPackage, 'def'));

DescriptorProto mD = DescriptorProto()
..name = 'D'
..field.addAll([
FieldDescriptorProto()
..name = 'a'
..jsonName = 'a'
..number = 1
..label = FieldDescriptorProto_Label.LABEL_OPTIONAL
..type = FieldDescriptorProto_Type.TYPE_MESSAGE
..typeName = ".A",
]);
FileDescriptorProto fdNoPackage = FileDescriptorProto()
..name = 'no_package.proto'
..messageType.add(mD);

void testFile(FileDescriptorProto fileDescriptor, Uri expectedFilePath) {
fileDescriptor.dependency.add('a.proto');
final options = parseGenerationOptions(
CodeGeneratorRequest(), CodeGeneratorResponse());
final fga = FileGenerator(fda, options);
final fg = FileGenerator(fileDescriptor, options);
link(options, [fg, fga]);
final filea = fga.outputFile(DefaultOutputConfiguration(), '.pb.dart');
final file = fg.outputFile(DefaultOutputConfiguration(), '.pb.dart');
expectMatchesGoldenFile(fga.generateMainFile().toString(),
'test/goldens/${filea.path}');
expectMatchesGoldenFile(fg.generateMainFile().toString(),
'test/goldens/${file.path}');
expect(
file,
expectedFilePath);
}

testFile(fdSamePackage, Uri.file('same_package.pb.dart'));
testFile(fdOtherPackage, Uri.file('other_package.pb.dart'));
testFile(fdNoPackage, Uri.file('no_package.pb.dart'));
});
}
32 changes: 32 additions & 0 deletions protoc_plugin/test/goldens/a.pb.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

46 changes: 46 additions & 0 deletions protoc_plugin/test/goldens/no_package.pb.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

46 changes: 46 additions & 0 deletions protoc_plugin/test/goldens/other_package.pb.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

46 changes: 46 additions & 0 deletions protoc_plugin/test/goldens/same_package.pb.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.