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

Avoid pruning unused dependencies for buf dep update #2966

Merged
merged 15 commits into from
May 15, 2024
Merged
24 changes: 14 additions & 10 deletions private/buf/bufworkspace/malformed_dep.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,12 @@ const (
type MalformedDepType int

// MalformedDep is a dep that was malformed in some way in the buf.yaml.
// It provides the module ref information and the malformed dep type.
type MalformedDep interface {
// ModuleFullName returns the full name of the malformed dep.
// ModuleRef is the module ref information of the malformed dep.
//
// Always present.
ModuleFullName() bufmodule.ModuleFullName
ModuleRef() bufmodule.ModuleRef
// Type is why this dep was malformed.
//
// Always present.
Expand Down Expand Up @@ -105,7 +106,7 @@ func MalformedDepsForWorkspace(workspace Workspace) ([]MalformedDep, error) {
malformedDeps = append(
malformedDeps,
newMalformedDep(
configuredDepModuleRef.ModuleFullName(),
configuredDepModuleRef,
MalformedDepTypeUnused,
),
)
Expand All @@ -114,8 +115,8 @@ func MalformedDepsForWorkspace(workspace Workspace) ([]MalformedDep, error) {
sort.Slice(
malformedDeps,
func(i int, j int) bool {
return malformedDeps[i].ModuleFullName().String() <
malformedDeps[j].ModuleFullName().String()
return malformedDeps[i].ModuleRef().ModuleFullName().String() <
malformedDeps[j].ModuleRef().ModuleFullName().String()
},
)
return malformedDeps, nil
Expand All @@ -124,19 +125,22 @@ func MalformedDepsForWorkspace(workspace Workspace) ([]MalformedDep, error) {
// *** PRIVATE ***

type malformedDep struct {
moduleFullName bufmodule.ModuleFullName
moduleRef bufmodule.ModuleRef
malformedDepType MalformedDepType
}

func newMalformedDep(moduleFullName bufmodule.ModuleFullName, malformedDepType MalformedDepType) *malformedDep {
func newMalformedDep(
moduleRef bufmodule.ModuleRef,
malformedDepType MalformedDepType,
) *malformedDep {
return &malformedDep{
moduleFullName: moduleFullName,
moduleRef: moduleRef,
malformedDepType: malformedDepType,
}
}

func (m *malformedDep) ModuleFullName() bufmodule.ModuleFullName {
return m.moduleFullName
func (m *malformedDep) ModuleRef() bufmodule.ModuleRef {
return m.moduleRef
}

func (m *malformedDep) Type() MalformedDepType {
Expand Down
4 changes: 2 additions & 2 deletions private/buf/bufworkspace/workspace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -221,9 +221,9 @@ func TestUnusedDep(t *testing.T) {
malformedDeps, err := MalformedDepsForWorkspace(workspace)
require.NoError(t, err)
require.Equal(t, 2, len(malformedDeps))
require.Equal(t, "buf.testing/acme/date", malformedDeps[0].ModuleFullName().String())
require.Equal(t, "buf.testing/acme/date", malformedDeps[0].ModuleRef().ModuleFullName().String())
require.Equal(t, MalformedDepTypeUnused, malformedDeps[0].Type())
require.Equal(t, "buf.testing/acme/extension", malformedDeps[1].ModuleFullName().String())
require.Equal(t, "buf.testing/acme/extension", malformedDeps[1].ModuleRef().ModuleFullName().String())
require.Equal(t, MalformedDepTypeUnused, malformedDeps[1].Type())
}

Expand Down
10 changes: 9 additions & 1 deletion private/buf/cmd/buf/command/dep/depprune/depprune.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,5 +75,13 @@ func run(
if err != nil {
return err
}
return internal.Prune(ctx, container.Logger(), controller, configuredDepModuleKeys, workspaceDepManager, dirPath)
return internal.Prune(
ctx,
container.Logger(),
controller,
container,
doriable marked this conversation as resolved.
Show resolved Hide resolved
configuredDepModuleKeys,
workspaceDepManager,
dirPath,
)
}
16 changes: 8 additions & 8 deletions private/buf/cmd/buf/command/dep/depupdate/depupdate.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"

"github.com/bufbuild/buf/private/buf/bufcli"
"github.com/bufbuild/buf/private/buf/bufctl"
"github.com/bufbuild/buf/private/buf/cmd/buf/command/dep/internal"
"github.com/bufbuild/buf/private/bufpkg/bufmodule"
"github.com/bufbuild/buf/private/pkg/app/appcmd"
Expand Down Expand Up @@ -104,7 +105,6 @@ func run(
if err != nil {
return err
}

configuredDepModuleRefs, err := workspaceDepManager.ConfiguredDepModuleRefs(ctx)
if err != nil {
return err
Expand Down Expand Up @@ -143,11 +143,11 @@ func run(
if err := workspaceDepManager.UpdateBufLockFile(ctx, configuredDepModuleKeys); err != nil {
return err
}
// Prune the buf.lock. This also verifies the workspace builds again.
// Building also has the side effect of doing tamper-proofing.
doriable marked this conversation as resolved.
Show resolved Hide resolved
//
// Note that the check to make sure configuredDepModuleKeys is a superset of the remote deps should be a no-op
// in this case, they should be equivalent based on the updates we just did, but we do the check
// anyways to triple verify.
return internal.Prune(ctx, logger, controller, configuredDepModuleKeys, workspaceDepManager, dirPath)
// Validate that the workspace builds and log unused configured deps.
workspace, err := controller.GetWorkspace(ctx, dirPath, bufctl.WithIgnoreAndDisallowV1BufWorkYAMLs())
if err != nil {
return err
}
// Log warnings for users on unused configured deps.
return internal.LogUnusedConfiguredDepsForWorkspace(workspace, logger)
}
45 changes: 30 additions & 15 deletions private/buf/cmd/buf/command/dep/internal/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,12 @@ func ModuleKeysAndTransitiveDepModuleKeysForModuleRefs(

// Prune prunes the buf.lock.
//
// Used by both mod prune and mod update.
// Used by both dep/mod prune and dep/mod update.
doriable marked this conversation as resolved.
Show resolved Hide resolved
func Prune(
ctx context.Context,
logger *zap.Logger,
controller bufctl.Controller,
container appext.Container,
// Contains all the Modules and their transitive dependencies based on the buf.yaml.
//
// All dependencies must be within this group from RemoteDepsForModuleSet. If a dependency
Expand All @@ -69,6 +70,8 @@ func Prune(
bufYAMLBasedDepModuleKeys []bufmodule.ModuleKey,
workspaceDepManager bufworkspace.WorkspaceDepManager,
dirPath string,
// For some use cases, such as dep/mod update, we want to keep unused declared dependencies
// in buf.lock (and only prune dependencies that are no longer declared) when set to true.
doriable marked this conversation as resolved.
Show resolved Hide resolved
) error {
workspace, err := controller.GetWorkspace(ctx, dirPath, bufctl.WithIgnoreAndDisallowV1BufWorkYAMLs())
if err != nil {
Expand All @@ -84,22 +87,10 @@ func Prune(
}
// Compute those dependencies that are in buf.yaml that are not used at all, and warn
// about them.
malformedDeps, err := bufworkspace.MalformedDepsForWorkspace(workspace)
if err != nil {
if err := LogUnusedConfiguredDepsForWorkspace(workspace, logger); err != nil {
return err
}
for _, malformedDep := range malformedDeps {
switch t := malformedDep.Type(); t {
case bufworkspace.MalformedDepTypeUnused:
logger.Sugar().Warnf(
`Module %[1]s is declared in your buf.yaml deps but is unused. This command only modifies buf.lock files, not buf.yaml files. Please remove %[1]s from your buf.yaml deps if it is not needed.`,
malformedDep.ModuleFullName(),
)
default:
return fmt.Errorf("unknown MalformedDepType: %v", t)
}
}
// Sep that actual computed remote dependencies based on imports. These are all
// Step that actually computes remote dependencies based on imports. These are all
// that is needed for buf.lock.
depModules, err := bufmodule.RemoteDepsForModuleSet(workspace)
if err != nil {
Expand All @@ -120,6 +111,30 @@ func Prune(
return workspaceDepManager.UpdateBufLockFile(ctx, depModuleKeys)
}

// LogUnusedConfiugredDepsForWorkspace takes a workspace and logs the unused configured
// dependneices as warnings to the user.
doriable marked this conversation as resolved.
Show resolved Hide resolved
func LogUnusedConfiguredDepsForWorkspace(
workspace bufworkspace.Workspace,
logger *zap.Logger,
) error {
malformedDeps, err := bufworkspace.MalformedDepsForWorkspace(workspace)
if err != nil {
return err
}
for _, malformedDep := range malformedDeps {
switch t := malformedDep.Type(); t {
case bufworkspace.MalformedDepTypeUnused:
logger.Sugar().Warnf(
`Module %[1]s is declared in your buf.yaml deps but is unused. This command only modifies buf.lock files, not buf.yaml files. Please remove %[1]s from your buf.yaml deps if it is not needed.`,
malformedDep.ModuleRef().ModuleFullName(),
)
default:
return fmt.Errorf("unknown MalformedDepType: %v", t)
}
}
return nil
}

// moduleKeysAndTransitiveDepModuleKeysForModuleKeys returns the ModuleKeys
// and all the transitive dependencies.
func moduleKeysAndTransitiveDepModuleKeysForModuleKeys(
Expand Down