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

Starlarkify sh_binary and sh_test #23732

Closed
wants to merge 4 commits into from
Closed

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Sep 24, 2024

The native sh_{binary,test} incorrectly used the execution platform instead of the target platform to determine whether to use a launcher. The starlarkified rule obtains the launcher from the sh_toolchain, which now has one instance registered for each common Bazel platform. Users can add additional toolchains as needed.

Fixes #23658

RELNOTES: sh_binary and sh_test can now be built for Windows on non-Windows execution platforms (and vice versa).

@fmeum fmeum force-pushed the sh-binary branch 7 times, most recently from f958b01 to a68d2b1 Compare September 24, 2024 14:10
@comius comius self-requested a review September 24, 2024 14:12
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 24, 2024

@comius I will ping you when this is ready. It is currently stacked on #23731 and #23660 and some tests are still failing.

@fmeum fmeum force-pushed the sh-binary branch 5 times, most recently from f636a77 to 629fc59 Compare September 24, 2024 18:12
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 24, 2024

@comius I had to disable doc generation on this PR for CI to get anywhere (see the last commit). Is there anything I need to do to get references such as ${link sh_test} to work for starlarkified rules?

@fmeum fmeum force-pushed the sh-binary branch 15 times, most recently from dd24d0c to 0341a0f Compare September 26, 2024 11:28
@@ -90,7 +90,7 @@ working directory. If run directly (using the <code>standalone</code> strategy),
results, including caching policies and environment variables. Tests generally need to be run
after the build is complete and on the target architecture, whereas genrules are executed during
the build and on the exec architecture (the two may be different). If you need a general purpose
testing rule, use <a href="${link sh_test}"><code>sh_test</code></a>.
testing rule, use <a href="shell#sh_test"><code>sh_test</code></a>.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't get ${link sh_test} to work and dropped it since it only occurs a single time in the docs.

@fmeum fmeum marked this pull request as ready for review October 1, 2024 10:35
@fmeum fmeum requested review from a team as code owners October 1, 2024 10:35
@fmeum fmeum requested review from gregestren and meteorcloudy and removed request for a team and gregestren October 1, 2024 10:35
@github-actions github-actions bot added team-Configurability platforms, toolchains, cquery, select(), config transitions team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Oct 1, 2024
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

LGTM for sh_toolchain and the repo rule! Thanks for the effort!

tools/sh/sh_configure.bzl Show resolved Hide resolved
@comius comius requested review from c-mita and removed request for comius October 1, 2024 12:26
@comius comius added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Oct 4, 2024
@Wyverald
Copy link
Member

Wyverald commented Oct 4, 2024

Could you please rebase this?

@fmeum
Copy link
Collaborator Author

fmeum commented Oct 4, 2024

@Wyverald I will switch this to draft and wait for #23873 to be merged, which will make rules_shell work.

@comius Could you advise me on the next steps for sh_* rules? I guess you will set up autoloading for them? Should I keep the docgen bits of this PR or should I drop it completely?

@fmeum fmeum marked this pull request as draft October 4, 2024 18:01
@fmeum
Copy link
Collaborator Author

fmeum commented Oct 16, 2024

@fmeum fmeum closed this Oct 16, 2024
@fmeum fmeum deleted the sh-binary branch October 16, 2024 12:01
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sh_binary to return RunEnvironmentInfo provider
4 participants