Repository: uber/prototool
Pull request #459 (2019-06-04 02:28:59)

Update to protoc 3.8.0



codecov-io: # [Codecov](https://codecov.io/gh/uber/prototool/pull/459?src=pr&el=h1) Report
> Merging [#459](https://codecov.io/gh/uber/prototool/pull/459?src=pr&el=desc) into [dev](https://codecov.io/gh/uber/prototool/commit/6a473a4f1d86e7c8ff6a844d7dc4f7c3f6207a3f?src=pr&el=desc) will **decrease** coverage by `0.4%`.
> The diff coverage is `42.1%`.

[![Impacted file tree graph](https://codecov.io/gh/uber/prototool/pull/459/graphs/tree.svg?width=650&token=A9V0DApqZg&height=150&src=pr)](https://codecov.io/gh/uber/prototool/pull/459?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##              dev     #459      +/-   ##
==========================================
- Coverage   74.99%   74.58%   -0.41%     
==========================================
  Files         133      133              
  Lines        6435     6473      +38     
==========================================
+ Hits         4826     4828       +2     
- Misses       1107     1141      +34     
- Partials      502      504       +2
```


| [Impacted Files](https://codecov.io/gh/uber/prototool/pull/459?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/protoc/compiler.go](https://codecov.io/gh/uber/prototool/pull/459/diff?src=pr&el=tree#diff-aW50ZXJuYWwvcHJvdG9jL2NvbXBpbGVyLmdv) | `50.82% <42.1%> (-3.9%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/uber/prototool/pull/459?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/uber/prototool/pull/459?src=pr&el=footer). Last update [6a473a4...b4b9095](https://codecov.io/gh/uber/prototool/pull/459?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/docs/README.md b/docs/README.md
index 9ee9327..2ea71f7 100644
--- a/docs/README.md
+++ b/docs/README.md
@@ -122,7 +122,7 @@ Recommended base config file:
 
 ```yaml
 protoc:
-  version: 3.7.1
+  version: 3.8.0
 lint:
   group: uber2
 ```
diff --git a/docs/faq.md b/docs/faq.md
index d9e631a..4a07c6a 100644
--- a/docs/faq.md
+++ b/docs/faq.md
@@ -17,7 +17,7 @@ prototool cache update
 # Cache to a specific directory path/to/cache
 prototool cache update --cache-path path/to/cache
 # Cache using custom configuration data instead of finding a prototool.yaml file using the file discovery mechanism
-prototool cache update --config-data '{"protoc":{"version":"3.7.1"}}'
+prototool cache update --config-data '{"protoc":{"version":"3.8.0"}}'
 ```
 
 There is also a command `prototool cache delete` which will delete all cached assets of
diff --git a/docs/protoc.md b/docs/protoc.md
index 94da75c..63b7ecb 100644
--- a/docs/protoc.md
+++ b/docs/protoc.md
@@ -21,12 +21,12 @@ for more details.
 - Otherwise, if on Linux, `$HOME/.cache/prototool` will be used, or on Darwin,
   `$HOME/Library/Caches/prototool` will be used.
 
-By default, `protoc` version `3.7.1` is downloaded, however this is configurable in your
+By default, `protoc` version `3.8.0` is downloaded, however this is configurable in your
 `prototool.yaml` file.
 
 ```yaml
 protoc:
-  version: 3.7.1
+  version: 3.8.0
 ```
 
 Downloads are safe to run concurrently across processes, for example if using from Bazel, as
diff --git a/etc/config/example/prototool.yaml b/etc/config/example/prototool.yaml
index 2047fe3..dea2360 100644
--- a/etc/config/example/prototool.yaml
+++ b/etc/config/example/prototool.yaml
@@ -8,9 +8,9 @@ excludes:
 # Protoc directives.
 protoc:
   # The Protobuf version to use from https://github.com/protocolbuffers/protobuf/releases.
-  # By default use 3.7.1.
+  # By default use 3.8.0.
   # You probably want to set this to make your builds completely reproducible.
-  version: 3.7.1
+  version: 3.8.0
 
   # Additional paths to include with -I to protoc.
   # By default, the directory of the config file is included,
diff --git a/example/proto/prototool.yaml b/example/proto/prototool.yaml
index 964fa21..da9dbe5 100644
--- a/example/proto/prototool.yaml
+++ b/example/proto/prototool.yaml
@@ -1,5 +1,5 @@
 protoc:
-  version: 3.7.1
+  version: 3.8.0
 lint:
   group: uber2
   rules:
diff --git a/internal/protoc/compiler.go b/internal/protoc/compiler.go
index 7cb49ff..4b5be21 100644
--- a/internal/protoc/compiler.go
+++ b/internal/protoc/compiler.go
@@ -52,14 +52,20 @@ var (
 	// backup that does not require this to be at the beginning of the line
 	fullPluginFailedRegexp = regexp.MustCompile("(.*)--.*_out: protoc-gen-(.*): Plugin failed with status code (.*).$")
 
-	extraImportRegexp           = regexp.MustCompile("^(.*): warning: Import (.*) but not used.$")
+	// protoc started printing line and column information in 3.8.0
+	extraImport380Regexp = regexp.MustCompile(`^(.*):(\d+):(\d+): warning: Import (.*) but not used.$`)
+	extraImportRegexp    = regexp.MustCompile("^(.*): warning: Import (.*) but not used.$")
+	// protoc started printing line and column information in 3.8.0
+	recursiveImport380Regexp    = regexp.MustCompile(`^(.*):(\d+):(\d+): File recursively imports itself: (.*)$`)
 	recursiveImportRegexp       = regexp.MustCompile("^(.*): File recursively imports itself: (.*)$")
 	directoryDoesNotExistRegexp = regexp.MustCompile("^(.*): warning: directory does not exist.$")
 	fileNotFoundRegexp          = regexp.MustCompile("^(.*): File not found.$")
 	// protoc outputs both this line and fileNotFound, so we end up ignoring this one
 	// TODO figure out what the error is for errors in the import
-	importNotFoundRegexp              = regexp.MustCompile("^(.*): Import (.*) was not found or had errors.$")
-	noSyntaxSpecifiedRegexp           = regexp.MustCompile(`No syntax specified for the proto file: (.*)\. Please use`)
+	importNotFoundRegexp    = regexp.MustCompile("^(.*): Import (.*) was not found or had errors.$")
+	noSyntaxSpecifiedRegexp = regexp.MustCompile(`No syntax specified for the proto file: (.*)\. Please use`)
+	// protoc started printing line and column information in 3.8.0
+	jsonCamelCase380Regexp            = regexp.MustCompile(`^(.*):(\d+):(\d+): (The JSON camel-case name of field.*)$`)
 	jsonCamelCaseRegexp               = regexp.MustCompile("^(.*): (The JSON camel-case name of field.*)$")
 	isNotDefinedRegexp                = regexp.MustCompile("^(.*): (.*) is not defined.$")
 	seemsToBeDefinedRegexp            = regexp.MustCompile(`^(.*): (".*" seems to be defined in ".*", which is not imported by ".*". To use it here, please add the necessary import.)$`)
@@ -649,6 +655,25 @@ func (c *compiler) parseProtocLine(cmdMeta *cmdMeta, protocLine string) *text.Fa
 				Message:  `No syntax specified. Please use 'syntax = "proto2";' or 'syntax = "proto3";' to specify a syntax version.`,
 			}
 		}
+		if matches := extraImport380Regexp.FindStringSubmatch(protocLine); len(matches) > 4 {
+			if cmdMeta.protoSet.Config.Compile.AllowUnusedImports {
+				return nil
+			}
+			line, err := strconv.Atoi(matches[2])
+			if err != nil {
+				return c.handleUninterpretedProtocLine(protocLine)
+			}
+			column, err := strconv.Atoi(matches[3])
+			if err != nil {
+				return c.handleUninterpretedProtocLine(protocLine)
+			}
+			return &text.Failure{
+				Filename: bestFilePath(cmdMeta, matches[1]),
+				Line:     line,
+				Column:   column,
+				Message:  fmt.Sprintf(`Import "%s" was not used.`, matches[4]),
+			}
+		}
 		if matches := extraImportRegexp.FindStringSubmatch(protocLine); len(matches) > 2 {
 			if cmdMeta.protoSet.Config.Compile.AllowUnusedImports {
 				return nil
@@ -658,6 +683,22 @@ func (c *compiler) parseProtocLine(cmdMeta *cmdMeta, protocLine string) *text.Fa
 				Message:  fmt.Sprintf(`Import "%s" was not used.`, matches[2]),
 			}
 		}
+		if matches := recursiveImport380Regexp.FindStringSubmatch(protocLine); len(matches) > 4 {
+			line, err := strconv.Atoi(matches[2])
+			if err != nil {
+				return c.handleUninterpretedProtocLine(protocLine)
+			}
+			column, err := strconv.Atoi(matches[3])
+			if err != nil {
+				return c.handleUninterpretedProtocLine(protocLine)
+			}
+			return &text.Failure{
+				Filename: bestFilePath(cmdMeta, matches[1]),
+				Line:     line,
+				Column:   column,
+				Message:  fmt.Sprintf(`File recursively imports itself %s.`, matches[4]),
+			}
+		}
 		if matches := recursiveImportRegexp.FindStringSubmatch(protocLine); len(matches) > 2 {
 			return &text.Failure{
 				Filename: bestFilePath(cmdMeta, matches[1]),
@@ -687,6 +728,22 @@ func (c *compiler) parseProtocLine(cmdMeta *cmdMeta, protocLine string) *text.Fa
 			// see comments at top
 			return nil
 		}
+		if matches := jsonCamelCase380Regexp.FindStringSubmatch(protocLine); len(matches) > 4 {
+			line, err := strconv.Atoi(matches[2])
+			if err != nil {
+				return c.handleUninterpretedProtocLine(protocLine)
+			}
+			column, err := strconv.Atoi(matches[3])
+			if err != nil {
+				return c.handleUninterpretedProtocLine(protocLine)
+			}
+			return &text.Failure{
+				Filename: bestFilePath(cmdMeta, matches[1]),
+				Line:     line,
+				Column:   column,
+				Message:  matches[4],
+			}
+		}
 		if matches := jsonCamelCaseRegexp.FindStringSubmatch(protocLine); len(matches) > 2 {
 			return &text.Failure{
 				Filename: bestFilePath(cmdMeta, matches[1]),
diff --git a/internal/vars/vars.go b/internal/vars/vars.go
index 08e2052..0cc4ae5 100644
--- a/internal/vars/vars.go
+++ b/internal/vars/vars.go
@@ -29,5 +29,5 @@ const (
 	// github.com/protocolbuffers/protobuf to use.
 	//
 	// See https://github.com/protocolbuffers/protobuf/releases for the latest release.
-	DefaultProtocVersion = "3.7.1"
+	DefaultProtocVersion = "3.8.0"
 )


Test patch:
diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go
index ca3e2ce..21de585 100644
--- a/internal/cmd/cmd_test.go
+++ b/internal/cmd/cmd_test.go
@@ -81,14 +81,14 @@ func TestCompile(t *testing.T) {
 		t,
 		false,
 		false,
-		`testdata/compile/extra_import/extra_import.proto:1:1:Import "dep.proto" was not used.`,
+		`testdata/compile/extra_import/extra_import.proto:6:1:Import "dep.proto" was not used.`,
 		"testdata/compile/extra_import/extra_import.proto",
 	)
 	assertDoCompileFiles(
 		t,
 		false,
 		false,
-		`testdata/compile/json/json_camel_case_conflict.proto:1:1:The JSON camel-case name of field "helloworld" conflicts with field "helloWorld". This is not allowed in proto3.`,
+		`testdata/compile/json/json_camel_case_conflict.proto:7:9:The JSON camel-case name of field "helloworld" conflicts with field "helloWorld". This is not allowed in proto3.`,
 		"testdata/compile/json/json_camel_case_conflict.proto",
 	)
 	assertDoCompileFiles(
@@ -110,7 +110,8 @@ func TestCompile(t *testing.T) {
 		t,
 		false,
 		false,
-		`testdata/compile/recursive/one.proto:1:1:File recursively imports itself one.proto -> two.proto -> one.proto.`,
+		`testdata/compile/recursive/one.proto:5:1:File recursively imports itself one.proto -> two.proto -> one.proto.
+		testdata/compile/recursive/one.proto:5:1:Import "two.proto" was not found or had errors.`,
 		"testdata/compile/recursive/one.proto",
 	)
 	assertDoCompileFiles(