Skip to content

tests: make toolchain tests run in the same build instead of bazel-in-bazel integration tests #2095

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

Merged

Conversation

rickeylev
Copy link
Collaborator

This changes the //tests/toolchains tests to run in the same Bazel build instance instead of
being bazel-in-bazel integration tests. This is done by using a transition to set
the python version to match the desired toolchain.

This makes running the tests much cheaper -- there were 37 sub-bazel processes being
started (one for each python version), which was very expensive. Debugging is also much easier because they aren't part of a bazel-in-bazel invocation.

@rickeylev rickeylev requested a review from aignas as a code owner July 26, 2024 17:26
@rickeylev
Copy link
Collaborator Author

Ok, CI Should be happy now.

@aignas Do we happen to have existing code to map the TOOL_VERSIONS/PLATFORMS platform info stuff to target_compatible_with values?

Over in tests/toolchains/defs.bzl, I'm generating a target for each version. However, not all versions support all platforms, e.g., 3.8.10 doesn't support windows. So I need to generate e.g.

py_reconfig_test(
  name = "python_3.8.10_test",
  target_compatible_with = select({
    <is x86_64-apple-darwin>: [],
    <is x86_64-unknown-linux-gnu>: [],
    ...etc, one entry for each TOOL_VERSIONS sha256 key...
    "//condiitons:default": ":incompatible",
  }),
  ...
)

That config setting generation looks awfully familiar, as do the platform 2-tuple strings (e.g. "aarch64-apple-darwin", "x86_64-unknown-linux-gnu").

@rickeylev rickeylev requested a review from groodt August 1, 2024 03:33
Copy link
Collaborator

@groodt groodt left a comment

Choose a reason for hiding this comment

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

LGTM

@rickeylev rickeylev added this pull request to the merge queue Aug 1, 2024
Merged via the queue into bazel-contrib:main with commit dbc6c04 Aug 1, 2024
4 checks passed
Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for simplifying!

dev_dependency = True,
)
dev_python.rules_python_private_testing(
register_all_versions = True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something like this could be super useful in registering minor_major versions by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I had a similar thought. I'll post in slack.

aignas added a commit to aignas/rules_python that referenced this pull request Aug 1, 2024
break
if register_all:
arg_structs.extend([
_create_toolchain_attrs_struct(python_version = v)
Copy link
Collaborator

@aignas aignas Aug 1, 2024

Choose a reason for hiding this comment

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

We probably need to add:

Suggested change
_create_toolchain_attrs_struct(python_version = v)
_create_toolchain_attrs_struct(tag=struct(python_version=v, configure_coverage_tool = True, is_default=False))

To fix #2105. I'll include it in #2106.

@rickeylev rickeylev deleted the refactor.same.build.toolchain.tests branch August 2, 2024 01:56
github-merge-queue bot pushed a commit that referenced this pull request Aug 2, 2024
This PR uses the `flag_values` from the platform definitions
for the toolchains so that in the future we can distinguish
between `musl` and `glibc` toolchains in our tests.

For now the change is no-op.

As part of this change we are also registering the coverage
tools so that we can run `bazel coverage` with no errors.

See comment on #2095.
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.

3 participants