Skip to content

Conversation

guw
Copy link

@guw guw commented Aug 19, 2025

Fix for #3854

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

Issue Number: #3854

What is the new behavior?

The execution toolchain will not be selected for incompatible target platform.

Does this PR introduce a breaking change?

  • No

Copy link

aspect-workflows bot commented Aug 19, 2025

Test

2 test targets passed

Targets
//docs:check_core [k8-fastbuild]        24ms
//packages/runfiles:test [k8-fastbuild] 111ms

@guw
Copy link
Author

guw commented Aug 20, 2025

It sounds like #3800 will take sometime to bake. Can we merge this meanwhile to mitigate #3854?

@gregmagolan @alexeagle @mattem @jbedard

@@ -107,9 +107,27 @@ resolved_toolchain(name = "resolved_toolchain", visibility = ["//visibility:publ

for [platform, meta] in PLATFORMS.items():
build_content += """
# Toolchain resolution is quite complex when it has to choose between both target and exec platform.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this comment be in the generated code like this or outside? Should it be repeated for each platform like it is atm?

Copy link
Author

@guw guw Aug 21, 2025

Choose a reason for hiding this comment

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

Where would you like to put it? I can move it outside.

I think it's what @jbedard was asking for
Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

@fmeum is my expert here. I'm pretty sure we had this code years ago and removed it.

@alexeagle alexeagle changed the title Set target_compatible_with for host execution toolchain fix: set target_compatible_with for host execution toolchain Aug 22, 2025
@alexeagle alexeagle enabled auto-merge (squash) August 22, 2025 17:53
@fmeum
Copy link
Member

fmeum commented Aug 22, 2025

This fix would make the situation more incorrect until we land a fix similar to #3800: we don't need two different kinds of constraints on any of the two toolchains (ignoring the compilation edge case for now).

@guw Could you share more details on how Node ends up in the image? That ultimate consumer of the Node toolchain should be updated to consume the existing target toolchain type rather than the exec toolchain type.

@guw
Copy link
Author

guw commented Aug 23, 2025

This is only a mitigation. We got confirmation on the issue that it helps mitigating the problem for others as well.

@fmeum #3859 could be merged instead. I need your feedback there.

The node binary will be include based on js_image_layer. js_image_layer does seem to rely on js_binary. I do plan on submitting PRs to this repo for updating this here:
https://github.com/aspect-build/rules_js/blob/c51b75e86c1e41dbbd4db6e54d43e5ccd25e6c6a/js/private/js_binary.bzl#L508

There aren't that many references, btw (search).

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