Separate core tests and test with bzlmod enabled #319
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The goal is to move the tests in rules_nixpkgs_core into this testing module for BCR compatibility. The tests in rules_nixpkgs_core incur extra dependencies that should not be part of rules_nixpkgs_core. In principle, such dependencies can be marked as dev-dependencies. However, since rules_nixpkgs only consists of repository rules (or in the future module extensions) those are easier to test in a dedicated module.
Requires a workaround for bzlmod module name mangling and remapping. For now this requires manual name mangling.
These require some workarounds for module mapping and equality not working on labels pointing to unknown external workspaces.
The labels as expected by the user are passed in as strings in a mapping to the actual labels which will be exposed with mangled names in the repository rule implementation. The mapping allows to map the user defined location variables to the corresponding paths without exposing mangled labels to the user.
Move the docstring to nixpkgs_package and reference it from the cc site.
Skylib offers diff_test for this exact purpose. It uses `diff` (or `fc.exe` on Windows) under the hood.
That test case had been forgotten in the test-target.
These have been moved to testing/core
aherrmann
force-pushed
the
bzlmod-core
branch
from
bfc16a0
to
e26b6da
Compare
The file is needed by docs and propagated as a dependency to the other toolchains through docs/dependencies_2.bzl.
Setting these flags in `.bazelrc` incurred a required for a Java toolchain. This, in turn, would require a dependency on rules_nixpkgs_java from rules_nixpkgs_core to set up a Java toolchain. I'd like to avoid that kind of circular dependency.
The core module no longer contains any test targets, and unfortunately bazel test fails when there are no test targets defined.
Any Bazel workspace that has tests seems to depend on a JDK toolchain due to ``` ERROR: .../external/remote_coverage_tools/BUILD:10:12: While resolving toolchains for target @remote_coverage_tools//:Main: No matching toolchains found for types @bazel_tools//tools/jdk:runtime_toolchain_type. To debug, rerun with --toolchain_resolution_debug='@bazel_tools//tools/jdk:runtime_toolchain_type' If platforms or toolchains are a new concept for you, we'd encourage reading https://bazel.build/concepts/platforms-intro. ```
Bazel needs a Java runtime to execute tests.
aherrmann
force-pushed
the
bzlmod-core
branch
from
e26b6da
to
bb96d1f
Compare
|
CI is finally green, so this is ready for review. |
This was referenced Feb 9, 2023
benradf
approved these changes
Feb 10, 2023
| "nixpkgs_package", | ||
| ) | ||
|
|
||
| def nixpkgs_repositories(*, bzlmod): |
There was a problem hiding this comment.
I see two invocations of this macro (one with bzlmod = True and one with bzlmod = False), but unless there's some magic going on it doesn't look like the parameter is actually used at the moment. Is it intended to be in the future?
There was a problem hiding this comment.
Good catch, it was needed at one point during development, but is no longer necessary here. I've removed it.
| ) | ||
|
|
||
| ### end dependencies for tests | ||
| rules_nixpkgs_dependencies(toolchains = []) |
There was a problem hiding this comment.
I was going to ask why pass toolchains = [] explicitly instead of relying on the default of None, but then I saw None means load all toolchains.
|
@Mergifyio refresh |
✅ Pull request refreshed |
|
The renaming of the CI steps to capture workspace and bzlmod required an update to the Mergify config as well. Merging manually because of that. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to #183.
This PR builds and tests
rules_nixpkgs_corewithbzlmodenabled.Following the motivation laid out in tweag/rules_sh#41, this separates the tests out into a dedicated testing module,
testing/core. The naming is chosen such that further testing modules can be added for other modules, e.g.testing/cc.The CI workflow is extended to run both, the old tests in WORKSPACE mode, and to test
coreandtesting/corewith bzlmod enabled.The location expansion unit tests remain withincore. The reason is that these are just unit-tests and don't require extra dependencies or module extensions. Andbazel test //...(as run by the CI script) fails if there are no test targets, see bazelbuild/bazel#7291 (comment).The
coremodule contains no more tests after this PR.bazel testneeds to resolve a Java runtime, meaningcorewould either have to depend onrules_nixpkgs_java(problematic due to tweag/rules_sh#41) or pick a local or bindist Java runtime (problematic for hermeticity and on NixOS). Unfortunately,bazel test //...fails when there are no test targets, so the CI script is adapted to runbazel buildincoreinstead.The testing module
testing/coreconfigures a Java runtime usingrules_nixpkgs_java. This uses theWORKSPACE.bzlmodfall-back in when bzlmod is enabled asrules_nixpkgs_javais not bzlmod compatible, yet.This does not yet add a module extension to
@rules_nixpkgs_core. Instead,testing/coreuses an ad-hocnon_module_depsmodule extension to invoke the repository rules provided by@rules_nixpkgs_core. Creating actual module extensions will be done in a separate PR.Most of the functionality worked without change with bzlmod enabled. However, the location expansion feature for the
nixoptsattribute broke due to bzlmod's use of module remapping and workspace name mangling. This is addressed by passing the labels tonix_file_depsin stringly form along their actual labels in a dictionary. This mapping is then used to resolve the stringly labels in the location expansion expressions to the actual file paths. This somewhat related to bazelbuild/bazel#17260. However, the newnative.package_relative_labelwould probably not address this issue, as the labels in the location expressions need to be resolved to paths within thenixpkgs_packagerepository rule implementation, not a macro.