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

opt: break memo's dependence on builtins #80745

Merged
merged 1 commit into from
Apr 29, 2022

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Apr 28, 2022

This commit breaks the memo packages dependence on the builtins
package so that they can be compiled concurrently. Optimizer packages
are no longer on the critical path, for now.

Informs #79357

Release note: None

@mgartner mgartner requested a review from a team as a code owner April 28, 2022 21:07
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

:lgtm:

nit: add "Informs #79357"

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @yuzefovich)


-- commits line 4 at r1:
nit: packages -> package's

Copy link
Collaborator Author

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

Done.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @yuzefovich)


-- commits line 4 at r1:

Previously, rytaft (Rebecca Taft) wrote…

nit: packages -> package's

Done.

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @yuzefovich)

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

:lgtm:

Looks like you need to run ./dev gen bazel so that the disallowed imports test is included in the bazel targets.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner)

This commit breaks the `memo` package's dependence on the `builtins`
package so that they can be compiled concurrently. Optimizer packages
are no longer on the critical path, for now.

Informs cockroachdb#79357

Release note: None
@mgartner
Copy link
Collaborator Author

TFTRs!

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 29, 2022

Build succeeded:

@craig craig bot merged commit 5871a77 into cockroachdb:master Apr 29, 2022
@mgartner mgartner deleted the break-memo-dep-on-builtins branch May 3, 2022 17:45
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.

4 participants