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

Pin bazel version to 5.2.0 to work around drake#17763 #235

Merged
merged 1 commit into from
Aug 23, 2022
Merged

Conversation

ggould-tri
Copy link
Contributor

@ggould-tri ggould-tri commented Aug 23, 2022

This change is Reviewable

Copy link
Contributor Author

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

+@svenevs for review.
I don't normally commit to this repo, so let me know if there are procedures I need to be following.

Reviewable status: all discussions resolved, LGTM missing from assignee svenevs, platform LGTM missing (waiting on @svenevs)

Copy link
Contributor

@svenevs svenevs left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all discussions resolved, platform LGTM missing (waiting on @ggould-tri)

Copy link
Contributor

@svenevs svenevs 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 normally commit to this repo, so let me know if there are procedures I need to be following.

Same, it seems it is the same as drake. drake-ci only needs one approval. +@rpoyner-tri for platform review per schedule please.

Reviewable status: all discussions resolved, LGTM missing from assignee rpoyner-tri, platform LGTM missing (waiting on @rpoyner-tri)

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Hm. Do we want to track RobotLocomotion/drake#17764 and pin 5.1 instead?

Reviewable status: all discussions resolved, LGTM missing from assignee rpoyner-tri, platform LGTM missing (waiting on @rpoyner-tri)

Copy link
Contributor Author

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

It would have been using 5.2 prior to today, so this is restoring status quo ante.
IMO, if the ultimate objective is for this to use head bazel as in the past, and we have a brief exception here, then 5.2 is the logical choice as it's both what we did before and also closest to head.
If on the other hand the objective is to get d-e-e to consume a bazel version from drake and we just temporarily don't have the plumbing yet, 5.1.0 makes more sense.
But I'm not sure what the original objective of d-e-e using bazel head was.

Reviewable status: all discussions resolved, LGTM missing from assignee rpoyner-tri, platform LGTM missing (waiting on @rpoyner-tri)

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all discussions resolved, platform LGTM missing (waiting on @ggould-tri)

Copy link
Contributor Author

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

Somehow I seem to be marked as a reviewer here? -@ggould-tri ?

Reviewable status: all discussions resolved, platform LGTM missing (waiting on @ggould-tri)

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: all discussions resolved, platform LGTM missing (waiting on @ggould-tri)

@ggould-tri ggould-tri self-assigned this Aug 23, 2022
Copy link
Contributor Author

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

Hm, that didn't work. +@ggould-tri :lgtm:

Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [ggould-tri] (waiting on @ggould-tri)

@ggould-tri ggould-tri merged commit a4c9bda into main Aug 23, 2022
@ggould-tri ggould-tri deleted the pin-bazel branch August 23, 2022 18:59
Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [ggould-tri] (waiting on @ggould-tri)

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

4 participants