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

Add rule to define hermetic POSIX toolchain #34

Merged
merged 13 commits into from
Dec 13, 2022
Merged

Add rule to define hermetic POSIX toolchain #34

merged 13 commits into from
Dec 13, 2022

Conversation

aherrmann
Copy link
Member

@aherrmann aherrmann commented Sep 2, 2022

depends on #35

This implements a POSIX toolchain based on sh_binaries as part of #19.

The toolchain uses the same toolchain type as the less hermetic version and is intended as an eventual replacement. For the moment it is marked as experimental, because there are more tasks to #19 that need to be closed before it can become a full replacement.

Backwards compatibility plan:
In the genrule use-case the hermetic POSIX toolchain can be used in the same way as the less hermetic version. In the custom rule use-case the toolchain provider exposes the same attributes as the less hermetic version for backwards compatibility. However, additional tools inputs must be declared to map the files into the build actions sandbox. If the tools have runfiles requirements, then the same workaround to bazelbuild/bazel#15486 as with sh_binaries is required.

@aherrmann aherrmann requested a review from avdv September 2, 2022 16:11
@aherrmann aherrmann force-pushed the hermetic-posix branch 6 times, most recently from 1da2d28 to 6e83db3 Compare September 6, 2022 09:35
It is a helper target also required in other parts of rules_sh.
sh_binary for a Shell script creates a wrapper binary that relies on the
runfiles mechanism to discover the corresponding shell script at
runtime. This introduces an implicit dependency on the runfiles
mechanism.

The purpose of the true/false toolchain is to test sh_binaries that
don't depend on runfiles, therefore the shell script approach is not
appropriate.

This uses C++ binaries instead.
@aherrmann aherrmann mentioned this pull request Sep 6, 2022
@aherrmann aherrmann changed the base branch from master to update-bazel September 6, 2022 09:52
@aherrmann
Copy link
Member Author

Windows put up a bit of a fight, but it's resolved now (see #35).
This is ready for review.

Base automatically changed from update-bazel to master September 6, 2022 11:43
Copy link
Member

@avdv avdv left a comment

Choose a reason for hiding this comment

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

LGTM 💯 , only found a few nits.

sh/experimental/posix_hermetic.bzl Outdated Show resolved Hide resolved
tests/posix_hermetic/custom_rule_output_test.sh Outdated Show resolved Hide resolved
tests/posix_hermetic/custom_rule_output_test.sh Outdated Show resolved Hide resolved
tests/posix_hermetic/genrule_explicit_output_test.sh Outdated Show resolved Hide resolved
tests/posix_hermetic/genrule_path_output_test.sh Outdated Show resolved Hide resolved
tests/posix_hermetic/custom_rule_output_test.sh Outdated Show resolved Hide resolved
tests/posix_hermetic/custom_rule_output_test.sh Outdated Show resolved Hide resolved
tests/posix_hermetic/genrule_explicit_output_test.sh Outdated Show resolved Hide resolved
tests/posix_hermetic/genrule_path_output_test.sh Outdated Show resolved Hide resolved
@aherrmann aherrmann self-assigned this Dec 12, 2022
@aherrmann aherrmann added the merge-queue merge on green CI label Dec 13, 2022
@mergify mergify bot merged commit 40a8451 into master Dec 13, 2022
@mergify mergify bot deleted the hermetic-posix branch December 13, 2022 10:24
@mergify mergify bot removed the merge-queue merge on green CI label Dec 13, 2022
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.

2 participants