Dogfood buf, propagate stderr for protoc plugins, and re-add SourceCo…

…deInfo comparison testing (#102)

internal/buf/bufbuild/builder_test.go

@@ -33,7 +33,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/zap"
-	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/types/descriptorpb"
 )
 
@@ -45,8 +44,7 @@ var buftestingDirPath = filepath.Join(
 
 func TestBuildGoogleapis(t *testing.T) {
 	t.Parallel()
-	//for _, includeSourceInfo := range []bool{true, false} {
-	for _, includeSourceInfo := range []bool{false} {
+	for _, includeSourceInfo := range []bool{true, false} {
 		t.Run(
 			fmt.Sprintf("includeSourceInfo:%v", includeSourceInfo),
 			func(t *testing.T) {
@@ -59,8 +57,7 @@ func TestBuildGoogleapis(t *testing.T) {
 
 func TestBufProtocGoogleapis(t *testing.T) {
 	t.Parallel()
-	//for _, includeSourceInfo := range []bool{true, false} {
-	for _, includeSourceInfo := range []bool{false} {
+	for _, includeSourceInfo := range []bool{true, false} {
 		t.Run(
 			fmt.Sprintf("includeSourceInfo:%v", includeSourceInfo),
 			func(t *testing.T) {
@@ -87,36 +84,30 @@ func TestActualProtocGoogleapis(t *testing.T) {
 
 func TestCompareGoogleapis(t *testing.T) {
 	t.Parallel()
-	//for _, includeSourceInfo := range []bool{true, false} {
-	for _, includeSourceInfo := range []bool{false} {
+	for _, includeSourceInfo := range []bool{true, false} {
 		t.Run(
 			fmt.Sprintf("includeSourceInfo:%v", includeSourceInfo),
 			func(t *testing.T) {
 				t.Parallel()
 				image := testBuildGoogleapis(t, includeSourceInfo)
 				fileDescriptorSet := bufcore.ImageToFileDescriptorSet(image)
 				actualProtocFileDescriptorSet := testBuildActualProtocGoogleapis(t, includeSourceInfo)
-				assertFileDescriptorSetsEqualJSON(t, fileDescriptorSet, actualProtocFileDescriptorSet)
-				// Cannot compare due to unknown field issue
-				//assertFileDescriptorSetsEqualProto(t, fileDescriptorSet, protocFileDescriptorSet)
+				prototesting.AssertFileDescriptorSetsEqual(t, fileDescriptorSet, actualProtocFileDescriptorSet)
 			},
 		)
 	}
 }
 
 func TestCompareBufProtocGoogleapis(t *testing.T) {
 	t.Parallel()
-	//for _, includeSourceInfo := range []bool{true, false} {
-	for _, includeSourceInfo := range []bool{false} {
+	for _, includeSourceInfo := range []bool{true, false} {
 		t.Run(
 			fmt.Sprintf("includeSourceInfo:%v", includeSourceInfo),
 			func(t *testing.T) {
 				t.Parallel()
 				bufProtocFileDescriptorSet := testBuildBufProtocGoogleapis(t, includeSourceInfo)
 				actualProtocFileDescriptorSet := testBuildActualProtocGoogleapis(t, includeSourceInfo)
-				assertFileDescriptorSetsEqualJSON(t, bufProtocFileDescriptorSet, actualProtocFileDescriptorSet)
-				// Cannot compare due to unknown field issue
-				//assertFileDescriptorSetsEqualProto(t, fileDescriptorSet, protocFileDescriptorSet)
+				prototesting.AssertFileDescriptorSetsEqual(t, bufProtocFileDescriptorSet, actualProtocFileDescriptorSet)
 			},
 		)
 	}
@@ -150,9 +141,7 @@ func testCompare(t *testing.T, relDirPath string) {
 	fileDescriptorSet := bufcore.ImageToFileDescriptorSet(image)
 	filePaths := buftesting.GetProtocFilePaths(t, dirPath, 0)
 	actualProtocFileDescriptorSet := buftesting.GetActualProtocFileDescriptorSet(t, false, false, dirPath, filePaths)
-	assertFileDescriptorSetsEqualWire(t, fileDescriptorSet, actualProtocFileDescriptorSet)
-	assertFileDescriptorSetsEqualJSON(t, fileDescriptorSet, actualProtocFileDescriptorSet)
-	assertFileDescriptorSetsEqualProto(t, fileDescriptorSet, actualProtocFileDescriptorSet)
+	prototesting.AssertFileDescriptorSetsEqual(t, fileDescriptorSet, actualProtocFileDescriptorSet)
 }
 
 func testBuildGoogleapis(t *testing.T, includeSourceInfo bool) bufcore.Image {
@@ -331,22 +320,3 @@ func testGetImageImportPaths(image bufcore.Image) []string {
 	sort.Strings(importNames)
 	return importNames
 }
-
-func assertFileDescriptorSetsEqualWire(t *testing.T, one *descriptorpb.FileDescriptorSet, two *descriptorpb.FileDescriptorSet) {
-	diffTwo, err := prototesting.DiffFileDescriptorSetsWire(one, two, "buf", "protoc")
-	assert.NoError(t, err)
-	assert.Equal(t, "", diffTwo, "Wire diff:\n%s", diffTwo)
-}
-
-func assertFileDescriptorSetsEqualJSON(t *testing.T, one *descriptorpb.FileDescriptorSet, two *descriptorpb.FileDescriptorSet) {
-	// TODO: test with resolver?
-	// This also has the effect of verifying output order
-	diffOne, err := prototesting.DiffFileDescriptorSetsJSON(one, two, "buf", "protoc")
-	assert.NoError(t, err)
-	assert.Equal(t, "", diffOne, "JSON diff:\n%s", diffOne)
-}
-
-func assertFileDescriptorSetsEqualProto(t *testing.T, one *descriptorpb.FileDescriptorSet, two *descriptorpb.FileDescriptorSet) {
-	equal := proto.Equal(one, two)
-	assert.True(t, equal, "proto.Equal returned false")
-}

internal/buf/cmd/buf/buf.go

@@ -22,7 +22,7 @@ import (
 )
 
 // Version is the version of buf.
-const Version = "0.19.0"
+const Version = "0.20.0-dev"
 
 // Main is the main.
 func Main(use string, options ...RootCommandOption) {

internal/buf/cmd/buf/internal/protoc/protoc_test.go

@@ -82,7 +82,7 @@ func TestComparePrintFreeFieldNumbersGoogleapis(t *testing.T) {
 	)
 }
 
-func TestCompareOutputJSONGoogleapis(t *testing.T) {
+func TestCompareOutputGoogleapis(t *testing.T) {
 	t.Parallel()
 	googleapisDirPath := buftesting.GetGoogleapisDirPath(t, buftestingDirPath)
 	filePaths := buftesting.GetProtocFilePaths(t, googleapisDirPath, 1000)
@@ -94,9 +94,7 @@ func TestCompareOutputJSONGoogleapis(t *testing.T) {
 		filePaths,
 	)
 	bufProtocFileDescriptorSet := testGetBufProtocFileDescriptorSet(t, googleapisDirPath)
-	diffOne, err := prototesting.DiffFileDescriptorSetsJSON(bufProtocFileDescriptorSet, actualProtocFileDescriptorSet, "buf", "protoc")
-	assert.NoError(t, err)
-	assert.Equal(t, "", diffOne, "JSON diff:\n%s", diffOne)
+	prototesting.AssertFileDescriptorSetsEqual(t, bufProtocFileDescriptorSet, actualProtocFileDescriptorSet)
 }
 
 func TestCompareGeneratedStubsGoogleapisGo(t *testing.T) {

internal/pkg/app/appproto/appprotoexec/binary_handler.go

@@ -17,7 +17,6 @@ package appprotoexec
 import (
 	"bytes"
 	"context"
-	"fmt"
 	"os/exec"
 
 	"github.com/bufbuild/buf/internal/pkg/app"
@@ -69,16 +68,15 @@ func (h *binaryHandler) Handle(
 		return err
 	}
 	h.logger.Debug("binary", zap.String("path", h.pluginPath))
-	stderrBuffer := bytes.NewBuffer(nil)
 	responseBuffer := bytes.NewBuffer(nil)
 	cmd := exec.CommandContext(ctx, h.pluginPath)
 	cmd.Env = app.Environ(container)
 	cmd.Stdin = bytes.NewReader(requestData)
 	cmd.Stdout = responseBuffer
-	cmd.Stderr = stderrBuffer
+	cmd.Stderr = container.Stderr()
 	if err := cmd.Run(); err != nil {
 		// TODO: strip binary path as well?
-		return fmt.Errorf("%v\n%v", err, stderrBuffer.String())
+		return err
 	}
 	response := &pluginpb.CodeGeneratorResponse{}
 	if err := protoencoding.NewWireUnmarshaler(nil).Unmarshal(responseBuffer.Bytes(), response); err != nil {

internal/pkg/app/appproto/appprotoexec/protoc_proxy_handler.go

@@ -98,17 +98,14 @@ func (h *protocProxyHandler) Handle(
 		request.FileToGenerate...,
 	)
 	h.logger.Debug("protoc_proxy", zap.Strings("args", args))
-	stderrBuffer := bytes.NewBuffer(nil)
 	cmd := exec.CommandContext(ctx, h.protocPath, args...)
 	cmd.Env = app.Environ(container)
 	cmd.Stdin = bytes.NewReader(fileDescriptorSetData)
-	// do we want to do this?
-	cmd.Stdout = stderrBuffer
-	cmd.Stderr = stderrBuffer
+	cmd.Stdout = ioutil.Discard
+	cmd.Stderr = container.Stderr()
 	if err := cmd.Run(); err != nil {
-		// Suppress printing of temp path
 		// TODO: strip binary path as well?
-		return fmt.Errorf("%v\n%v", err, strings.Replace(stderrBuffer.String(), tmpDir.AbsPath(), "", -1))
+		return err
 	}
 	if featureProto3Optional {
 		responseWriter.SetFeatureProto3Optional()

internal/pkg/diff/diff.go

@@ -54,7 +54,7 @@ func Diff(
 	if len(data) > 0 {
 		// diff exits with a non-zero status when the files don't match.
 		// Ignore that failure as long as we get output.
-		return modifyHeader(data, filename1, filename2, keepTimestamps)
+		return tryModifyHeader(data, filename1, filename2, keepTimestamps), nil
 	}
 	return nil, err
 }
@@ -75,15 +75,15 @@ func writeTempFile(dir, prefix string, data []byte) (string, error) {
 	return file.Name(), nil
 }
 
-func modifyHeader(
+func tryModifyHeader(
 	diff []byte,
 	filename1 string,
 	filename2 string,
 	keepTimestamps bool,
-) ([]byte, error) {
+) []byte {
 	bs := bytes.SplitN(diff, []byte{'\n'}, 3)
 	if len(bs) < 3 {
-		return nil, fmt.Errorf("got unexpected diff for %s-%s", filename1, filename2)
+		return diff
 	}
 	// Preserve timestamps.
 	var t0, t1 []byte
@@ -103,5 +103,5 @@ func modifyHeader(
 	}
 	bs[0] = []byte(fmt.Sprintf("--- %s%s", filename1, t0))
 	bs[1] = []byte(fmt.Sprintf("+++ %s%s", filename2, t1))
-	return bytes.Join(bs, []byte{'\n'}), nil
+	return bytes.Join(bs, []byte{'\n'})
 }

internal/pkg/prototesting/prototesting.go

@@ -23,9 +23,13 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"testing"
 
 	"github.com/bufbuild/buf/internal/pkg/diff"
 	"github.com/bufbuild/buf/internal/pkg/protoencoding"
+	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/assert"
+	"google.golang.org/protobuf/testing/protocmp"
 	"google.golang.org/protobuf/types/descriptorpb"
 )
 
@@ -70,10 +74,18 @@ func GetProtocFileDescriptorSet(
 	if err != nil {
 		return nil, err
 	}
+	firstFileDescriptorSet := &descriptorpb.FileDescriptorSet{}
+	if err := protoencoding.NewWireUnmarshaler(nil).Unmarshal(data, firstFileDescriptorSet); err != nil {
+		return nil, err
+	}
+	resolver, err := protoencoding.NewResolver(
+		firstFileDescriptorSet.File...,
+	)
+	if err != nil {
+		return nil, err
+	}
 	fileDescriptorSet := &descriptorpb.FileDescriptorSet{}
-	// we do not know the FileDescriptorSet ahead of time so we cannot use it for extensions
-	// TODO: change to image read logic
-	if err := protoencoding.NewWireUnmarshaler(nil).Unmarshal(data, fileDescriptorSet); err != nil {
+	if err := protoencoding.NewWireUnmarshaler(resolver).Unmarshal(data, fileDescriptorSet); err != nil {
 		return nil, err
 	}
 	return fileDescriptorSet, nil
@@ -198,3 +210,21 @@ func DiffFileDescriptorSetsJSON(
 	}
 	return string(output), nil
 }
+
+// DiffFileDescriptorSetsCompare diffs the two FileDescriptorSets using the cmp package.
+func DiffFileDescriptorSetsCompare(
+	one *descriptorpb.FileDescriptorSet,
+	two *descriptorpb.FileDescriptorSet,
+) string {
+	return cmp.Diff(one, two, protocmp.Transform())
+}
+
+// AssertFileDescriptorSetsEqual asserts that the FileDescriptorSet are equal for
+// JSON and compare.
+func AssertFileDescriptorSetsEqual(t *testing.T, one *descriptorpb.FileDescriptorSet, two *descriptorpb.FileDescriptorSet) {
+	diff, err := DiffFileDescriptorSetsJSON(one, two, "buf", "protoc")
+	assert.NoError(t, err)
+	assert.Empty(t, diff)
+	diff = DiffFileDescriptorSetsCompare(one, two)
+	assert.Empty(t, diff)
+}

make/buf/base.mk

@@ -17,6 +17,7 @@ FILE_IGNORES := $(FILE_IGNORES) \
 	internal/buf/internal/buftesting/cache/ \
 	internal/pkg/storage/storageos/tmp/
 
+PROTOC_USE_BUF := true
 
 include make/go/bootstrap.mk
 include make/go/go.mk
@@ -26,6 +27,8 @@ include make/go/docker.mk
 include make/go/protoc_gen_go.mk
 include make/go/dep_go_fuzz.mk
 
+protocpreinstall:: installbuf
+
 .PHONY: wkt
 wkt: installstorage-go-binary-data $(PROTOC)
 	rm -rf internal/gen/data

make/go/base.mk

@@ -99,6 +99,9 @@ dockerdeps::
 .PHONY: deps
 deps:: dockerdeps
 
+.PHONY: preinstallgenerate
+preinstallgenerate::
+
 .PHONY: pregenerate
 pregenerate::
 
@@ -110,6 +113,7 @@ licensegenerate::
 
 .PHONY: generate
 generate:
+	@$(MAKE) preinstallgenerate
 	@$(MAKE) pregenerate
 	@$(MAKE) postgenerate
 	@$(MAKE) licensegenerate

make/go/dep_protoc.mk

@@ -38,5 +38,17 @@ $(PROTOC):
 
 dockerdeps:: $(PROTOC)
 
-.PHONY: protocpre
-protocpre::
+.PHONY: protocpreinstall
+protocpreinstall::
+
+.PHONY: protocgenerate
+protocgenerate::
+
+preinstallgenerate:: protocpreinstall
+
+pregenerate:: protocgenerate
+
+.PHONY: protoc
+protoc:
+	$(MAKE) protocpreinstall
+	$(MAKE) protocgenerate

make/go/go.mk

@@ -42,6 +42,11 @@ all:
 	@$(MAKE) lint
 	@$(MAKE) test
 
+.PHONY: shortall
+shortall:
+	@$(MAKE) lint
+	@$(MAKE) shorttest
+
 .PHONY: ci
 ci:
 	@$(MAKE) deps
@@ -111,6 +116,10 @@ pretest::
 test: pretest
 	go test $(GO_TEST_FLAGS) $(GOPKGS)
 
+.PHONY: shorttest
+shorttest: pretest
+	go test -test.short $(GO_TEST_FLAGS) $(GOPKGS)
+
 .PHONY: deppkgs
 deppkgs:
 	@go list -f '{{join .Deps "\n"}}' $(GOPKGS) | xargs go list -f '{{if not .Standard}}{{.ImportPath}}{{end}}'

make/go/protoc_gen_go.mk

@@ -18,18 +18,23 @@ PROTOC_GEN_GO_OPT ?=
 
 EXTRA_MAKEGO_FILES := $(EXTRA_MAKEGO_FILES) scripts/protoc_gen_plugin.bash
 
+PROTOC_GEN_GO_EXTRA_FLAGS :=
+ifdef PROTOC_USE_BUF
+PROTOC_GEN_GO_EXTRA_FLAGS := --use-buf
+endif
+
 .PHONY: protocgengoclean
 protocgengoclean:
 	rm -rf "$(PROTOC_GEN_GO_OUT)"
 
 .PHONY: protocgengo
-protocgengo: protocpre protocgengoclean $(PROTOC) $(PROTOC_GEN_GO)
-	bash $(MAKEGO)/scripts/protoc_gen_plugin.bash \
+protocgengo: protocgengoclean $(PROTOC) $(PROTOC_GEN_GO)
+	bash $(MAKEGO)/scripts/protoc_gen_plugin.bash $(PROTOC_GEN_GO_EXTRA_FLAGS) \
 		"--proto_path=$(PROTO_PATH)" \
 		"--proto_include_path=$(CACHE_INCLUDE)" \
 		$(patsubst %,--proto_include_path=%,$(PROTO_INCLUDE_PATHS)) \
 		"--plugin_name=go" \
 		"--plugin_out=$(PROTOC_GEN_GO_OUT)" \
 		"--plugin_opt=$(PROTOC_GEN_GO_OPT)"
 
-pregenerate:: protocgengo
+protocgenerate:: protocgengo