Skip to content
This repository has been archived by the owner on Mar 4, 2022. It is now read-only.

Config file discovery semantics #10

Closed
bufdev opened this issue Apr 10, 2018 · 3 comments
Closed

Config file discovery semantics #10

bufdev opened this issue Apr 10, 2018 · 3 comments

Comments

@bufdev
Copy link
Contributor

bufdev commented Apr 10, 2018

Right now, config files are named prototool.yaml and searched for starting at the Protobuf file path, and going up a directory until hitting root. All paths in the config files can be relative or absolute, but if relative, the actual path used will start at the config file directory. If using directory mode, this means multiple prototool.yaml files corresponding to multiple found directories with Protobuf files may be used. After a config file is found, Prototool walks starting at that directory searching for Protobuf files, or if no config file is found, Prototool searches starting at the current directory.

A few things:

  • Is prototool.yaml the right name?
  • Should we terminate earlier? Right now for example, if you don't have a prototool.yaml file in your repo, but do in your home directory, prototool will go all the way down to your home directory, and then walk everything in your home directory, which can be annoying.
  • Should we require a prototool.yaml file to use prototool? See the above. It's good practice to always have a prototool.yaml file to at least lock down the version of protoc you use, but this can be annoying for OSS users who are just starting out.
  • The biggest thing: should we not allow multiple prototool.yaml files to be discovered? If multiple prototool.yaml files are discovered, this means that multiple calls to protoc must be done as they denote different builds, and more than one FileDescriptorSet is created. This gets bad when you do e.g. gRPC calls as foo.bar.BazService could be in more than one FileDescriptorSet, so you basically have to stop when you find the first one. All packages in Prototool have to handle multiple FileDescriptorSets, which leads to some other bad outcomes. Most people shouldn't have multiple prototool.yaml files, but it could be useful for compiling vendored prototool.yaml files.
@bufdev bufdev changed the title Config file discovery Config file discovery semantics Apr 10, 2018
@amckinney
Copy link
Contributor

I think that requiring a prototool.yaml file is fine / necessary. It determines the root for how a directory's underlying proto files are treated (e.g. how they're linted / generated), and it clearly expresses a project's protoc plugins, linters, etc.

I see where you're coming from with respect to on-boarding, but adding a configuration file to begin using a new tool seems to be a reasonable requirement. And now that we are only allowing one prototool.yaml, this is even easier to justify.

The naming seems fine to me as well. The tool is called prototool - we may as well name its configuration file the same. An alternative would be to house an arbitrarily-named file in a .prototool/ directory, but that doesn't have much benefit.

What are your thoughts? I'm fine with resolving this issue for now if others agree. cc @peter-edge @AlexAPB

@bufdev
Copy link
Contributor Author

bufdev commented Jul 12, 2018

We don't actually require one right now, there are sensible defaults - we can make it required if need be, but for general use you'll end up setting up a prototool.yaml anyways, it's nice to not make that a strict requirement (and we already have the code for that, heh).

@amckinney
Copy link
Contributor

Cool, I'm fine with that! Like you've described, users will end up setting up a prototool.yaml eventually, but at least they can get started without it.

So in short:

  • Stick to one prototool.yaml for now.
  • Fallback to sensible defaults in its absence.

sevennt pushed a commit to sevennt/prototool that referenced this issue Aug 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants