-
Notifications
You must be signed in to change notification settings - Fork 522
Split :toolchain_type to correctly cover all use cases
#3800
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
Split :toolchain_type to correctly cover all use cases
#3800
Conversation
alexeagle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine this is approximately what needs to happen, thanks for working on it!
Is there an issue (or maybe several) which could be linked from here?
I'm not sure who's best suited to do the review, among folks in the bazel-contrib org there are certainly a few experts on toolchain types and resolution.
|
Instead of an incompatible attribute on repo rules and module extensions, it may be possible to temporarily make the new toolchain types |
|
@Silic0nS0ldier any chance this might get the attention it needs to land? For context, we've been running with this for a few months now, and it solved a lot of the issues we had cross-compiling from Mac. |
Can someone help me understand why this needs a flag? Can't the "old" toolchain_type just become the compile/host toolchain and new ones are introduced for runtime? Meanwhile, rules_js and others can start updating their rules to use the runtime toolchain Can the 3rd type be postponed to a second PR? I don't see an immediate need there. If yes I can work on a simpler version which keeps the existing |
According to discussions in #3854 having two toolchains of the same type for different things is troublesome. It's better to have separate runtime as well as compile toolchains. This commit creates a new runtime_toolchain_type and registers toolchains without execution constraints for this type. Once merged, rules_ts can start consuming the new toolchain type in its js_binary rule to ensure the correct Node for the correct target environment is selected. Fixed [Bug]: Execution toolchain defined without `target_compatible_with` makes it a candidate to selection Fixes #3854 Work towards #3795 ## PR Checklist Please check if your PR fulfills the following requirements: - [x] Tests for the changes have been added (for bug fixes / features) - [x] Docs have been added / updated (for bug fixes / features) ## PR Type What kind of change does this PR introduce? <!-- Please check the one that applies to this PR using "x". --> - [x] Bugfix - [x] Feature (please, look at the "Scope of the project" section in the README.md file) - [x] Documentation content changes ## Does this PR introduce a breaking change? - [ ] Yes - [x] No It tries to remain compatible and support existing consumers. ## Other information This is an alternative to #3800.
|
I think this was finally resolved in #3859 |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #3795
See issue for details.
What is the new behavior?
@rules_nodejs//nodejs:toolchain_typeinto@rules_nodejs//nodejs:runtime_toolchain_typeand@rules_nodejs//nodejs:exec_runtime_toolchain_type.@rules_nodejs//nodejs:runtime_toolchain_typeis suited towards usage in the target environment (e.g. runtime for*_binaryrule outputs).@rules_nodejs//nodejs:exec_runtime_toolchain_typeis suited towards usage within rule actions (e.g. running NodeJS directly).incompatible_split_toolchainson;node.toolchainmodule extension tag.nodejs_register_toolchainsmacro.nodejs_toolchains_reporepository rule (private API).:resolved_toolchaintarget moved into@rules_nodejs//nodejs. This makes it possible to access a NodeJS toolchain implementation without needing to know the name of a user controlled generated repository (e.g. for use inside agenruledefined in another ruleset).@rules_nodejs//nodejs:compilation_toolchain_typesuited to usage of NodeJS as a compiler (e.g. for snapshot generation) when the exec and target platforms need to be aligned. The exact platforms can still be different, but they should at least be compatible across OS and CPU.Does this PR introduce a breaking change?
Other information
Extra TODO