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

Avoid pruning unused dependencies for buf dep update #2966

merged 15 commits into from
May 15, 2024

Conversation

doriable
Copy link
Member

@doriable doriable commented May 10, 2024

When running buf mod update, we want to include in the buf.lock deps
that have been configured in buf.yaml and may not be used yet. This is to
support workflows where a user can configure a dependency and buf dep update
before they use the newly configured dep in their proto definitions.

Prune does the following things right now:

  • builds the workspace
  • remove unused dependencies from the buf.lock
  • check for dependencies not configured in buf.yaml that are present in buf.lock

The first step is necessary for buf dep update for tamper-proofing and validating
the build. However, the latter two steps are unnecessary -- we want to keep
unused dependencies and their transitive dependencies, and since dep update
gets the dependencies from buf.yaml and writes them into buf.lock, there
will not be dependencies present not from the buf.yaml, so that step is a
no-op.

So instead of running Prune, we validate the build after the logic for dep udpate
and we log out the unused dependencies as a warning to the users (but we do not
remove them).

@doriable doriable changed the title Do not prune unused dependencies for buf dep update Avoid pruning unused dependencies for buf dep update May 10, 2024
@doriable
Copy link
Member Author

Found something in testing today: when pulling in a new dependency that is not used yet, we do not pull in its transitive dependencies... I think this is expected/fine since you may not end up needing all transitive dependencies from the new dependency. But I wanted to note this.

Copy link
Contributor

@saquibmian saquibmian left a comment

Choose a reason for hiding this comment

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

I don't think this is right.

private/buf/cmd/buf/command/dep/internal/internal.go Outdated Show resolved Hide resolved
private/buf/cmd/buf/command/dep/internal/internal.go Outdated Show resolved Hide resolved
Copy link
Contributor

@saquibmian saquibmian left a comment

Choose a reason for hiding this comment

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

Minor comments

private/buf/cmd/buf/command/dep/internal/internal.go Outdated Show resolved Hide resolved
private/buf/cmd/buf/command/dep/internal/internal.go Outdated Show resolved Hide resolved
Copy link
Member

@pkwarren pkwarren left a comment

Choose a reason for hiding this comment

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

Changes look good - two minor comments.

private/buf/cmd/buf/command/dep/depprune/depprune.go Outdated Show resolved Hide resolved
private/buf/cmd/buf/command/dep/internal/internal.go Outdated Show resolved Hide resolved
doriable and others added 2 commits May 15, 2024 10:57
Co-authored-by: Philip K. Warren <pkwarren@users.noreply.github.com>
@bufdev bufdev merged commit 4a0f6b4 into dev May 15, 2024
9 checks passed
@bufdev bufdev deleted the bufdev-stuff branch May 15, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants