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.

Sign up for GitHub

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

Jump to bottom

feat: introduce toolchain helper #194

Merged
merged 4 commits into from
Nov 29, 2023
Merged

feat: introduce toolchain helper #194

merged 4 commits into from
Nov 29, 2023

Conversation

thesayyn
Copy link
Collaborator

This is not final yet, I just want to test out with rules_python first. Welcome to review though.

Also due to bazelbuild/bazel#20297 i can't write unit tests for the incompatible flag. I'll look for a workaround.

@thesayyn thesayyn requested review from comius and a team as code owners November 22, 2023 23:34
comius
comius requested changes Nov 23, 2023
View reviewed changes
Copy link
Collaborator

@comius comius left a comment

Choose a reason for hiding this comment

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

i can't write unit tests for the incompatible flag. I'll look for a workaround.

I don't mind if some tests are omitted at this level of granularity. Also the code is copied from builtin proto_common.bzl, and already tested there on a slightly bigger granularity (i.e there are tests for proto_library/proto_lang_toolchain/java_proto_library)

proto/proto_common.bzl Outdated Show resolved Hide resolved
@thesayyn
Copy link
Collaborator Author

I don't mind if some tests are omitted at this level of granularity. Also the code is copied from builtin proto_common.bzl, and already tested there on a slightly bigger granularity (i.e there are tests for proto_library/proto_lang_toolchain/java_proto_library)

hmm, I agree but I still believe we should test this repo with the incompatible flag to be safe even if it's not this granular.

@thesayyn
Copy link
Collaborator Author

thesayyn commented Nov 23, 2023 •
edited
Loading

@comius I propose adding a new test target to bazelci here that tests the incompatible flag against a prerelease of bazel. i can make that a follow-up

@thesayyn thesayyn requested a review from comius November 23, 2023 20:59
@thesayyn
Copy link
Collaborator Author

@comius PTAL. this is ready.

@thesayyn thesayyn mentioned this pull request Nov 23, 2023
Merged
@comius comius added the ready to pull Required label for automatic import to Google label Nov 29, 2023
@comius comius merged commit f9b0b88 into main Nov 29, 2023
9 checks passed
comius added a commit that referenced this pull request Feb 12, 2024
@comius
Merge pull request #194 from bazelbuild:introduce_toolchains_helper
d046e49 
PiperOrigin-RevId: 586331999
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bellspice bellspice bellspice approved these changes

@comius comius comius approved these changes

Assignees
No one assigned
Labels
ready to pull Required label for automatic import to Google
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thesayyn @comius @bellspice