Skip to content
This repository was archived by the owner on Jan 14, 2026. It is now read-only.

feat: introduce toolchain helper #194

Merged
comius merged 4 commits into from
Nov 29, 2023
Merged

feat: introduce toolchain helper #194

comius merged 4 commits into from
Nov 29, 2023
+103 −0

Conversation

@thesayyn
Copy link
Collaborator

@thesayyn thesayyn commented Nov 22, 2023

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 a team and comius as code owners November 22, 2023 23:34
comius
comius suggested 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)

@thesayyn
Copy link
Collaborator Author

thesayyn commented Nov 23, 2023

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 thesayyn force-pushed the introduce_toolchains_helper branch from 66cc0df to 9befa0f Compare November 23, 2023 20:48
@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 force-pushed the introduce_toolchains_helper branch from 2832da5 to 251af60 Compare November 23, 2023 20:53
@thesayyn thesayyn requested a review from comius November 23, 2023 20:59
@thesayyn
Copy link
Collaborator Author

thesayyn commented Nov 23, 2023

@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
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@comius comius comius approved these changes

@bellspice bellspice bellspice approved these changes

Reviewers whose approvals may not affect merge requirements

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.

4 participants

@thesayyn @comius @bellspice