extract `bzlmod` modules for core rules and toolchains #193

fricklerhandwerk · 2022-03-07T12:59:24Z

closes #182

This is a fairly large change in terms of LOC, and also has some noise due to a generated README.md being checked into version control. It's best to follow it commit by commit, as they are fairly self-contained.

The main line of thought is to literally separate rules by concern, as described in #182, and keep the tests working (which accounts for most of the non-obvious changes).

this is in preparation to support `bzlmod`

otherwise the entire file will be consumed as the next line, skipping everything after the last match.

tl;dr: end users of `rules_nixpkgs` should not be affected by this change at all. the additional code is temporary and purely for migration reasons. migrating the go toolchain to `bzlmod` necessitates accessing `rules_nixpkgs_core` as an external repository (later module). therefore we have to incorporate that new repository into the `WORKSPACE` setup. to maintain compatibility with existing installations, `rules_nixpkgs_dependencies()` automatically fetches the new sub-repository from a remote archive. but local testing requires taking it from the working directory! therefore we temporary introduce a `local=` parameter which lets the caller point optionally to a directory for local sub-repositories. the option will be removed when migration to `bzlmod` is complete. it must be configurable, because the call to `local_repository()` takes its path starting from the workspace directory where `rules_nixpkgs_dependencies()` is called: - `bazel test //...` needs `local="."` - `cd examples/toolchains/{go, java, ...}; bazel run :hello` needs `local="../../.."`

no alias for `default_java_toolchain`, since that seems to be used only internally, and a copy of what is usually distributed with Bazel.

leave aliases to existing files to ensure backwards compatibility. put CC toolchain in separate workspace, which is required because `nixpkgs_cc_configure()` references a file `cc.nix` from its own repository, which will be external at the call site.

otherwise internal references will break

necessary to facilitate sandboxed integration testing, where the original source directory of the main workspace is not accessible without more jumping through hoops.

requires exposing `:srcs` for all the toolchain modules, so they can be added as `runfiles` to the sandbox.

aherrmann

Great work! Thank you!
A few comments, though this already looks very good.

To be sure that the backwards compatibility aspect works we should also test this on some users of rules_nixpkgs. rules_haskell and daml come to mind.

docs/BUILD.bazel

docs/update-readme.sh

nixpkgs/toolchains/rust.bzl

nixpkgs/repositories.bzl

toolchains/rust/MODULE.bazel

docs/BUILD.bazel

nixpkgs/BUILD.bazel

keep aliases for backwards compatibility

it belongs there and is used only there

the boilerplate for accessing runfiles turned out to be not needed. from now on one can just add targets or filenames for generating documentation and checking that the generated files are up to date under version control.

while incomplete, this should cover most existing use cases seamlessly, such that existing users would not notice the refactoring.

this requires some more cleanup and refactoring before it is readable again, see TODOs.

aherrmann

Thank you! Good working teasing those apart!

A few last minor comments, and avdv/daml@04dff55#r68737946. Feel free to merge once they're resolved.

This is one of those PRs, where git's colorMoved = default and colorMovedWS = allow-indentation-change is really useful.

aherrmann · 2022-03-15T16:18:28Z

docs/BUILD.bazel

+        # sorry for this mess. Bazel `str.format()` does not allow nested `{}`
+        # pairs and `awk` is sensitive to line breaks.
+        # TODO: consider alternative: leave reference for the end and just
+        # append the generated string with `cat`.


I think one can escape { as {{ and then it can be nested.

🤔 Does that make anything substantially better? I'll try...

It does not work, not even in Python 3.8 (ValueError: unexpected '{' in field name). This is a pathological case. Just look at the amount of additional braces! It is probably better to have a simpler setup in the first place, as noted in the comment.

examples/toolchains/rust/WORKSPACE

aherrmann · 2022-03-15T16:50:19Z

Uhm, that was a bit faster than intended, sorry about that! It looks like we lost the merge-queue label requirement in the Mergify config. I've opened #199 to fix that.
@fricklerhandwerk In that case, could you address the last minor comments in a follow-up?

avdv · 2022-03-16T08:30:58Z

To be sure that the backwards compatibility aspect works we should also test this on some users of rules_nixpkgs. rules_haskell and daml come to mind.

FTR, here are the changes that were necessary in the daml project to use rules_nixpkgs after extracting the modules:

digital-asset/daml@main...avdv:rules_nixpkgs-pr-193

(due to not using rules_nixpkgs_dependencies() the logic from nixpkgs/repositories.bzl had to be duplicated and also the patch did not apply cleanly anymore)

fricklerhandwerk · 2022-03-16T09:08:21Z

@avdv Which patch do you mean?

avdv · 2022-03-16T09:56:05Z

@avdv Which patch do you mean?

In daml they are applying a patch to rules_nixpkgs to avoid "occasional segmentation faults of nix":
https://github.com/digital-asset/daml/blob/e0965709fe62b5c86f980ef25472a5eeb59f3adb/deps.bzl#L51-L59

this one:
https://github.com/digital-asset/daml/blob/main/bazel_tools/nixpkgs-disable-http2.patch

I did not care too much about it, as I did not see a segfault and the patch could be adapted if needed.

aherrmann · 2022-03-16T10:53:58Z

@avdv Thanks for testing and sharing the diff!

due to not using rules_nixpkgs_dependencies() the logic from nixpkgs/repositories.bzl had to be duplicated

Raised a question here:

Was it necessary to duplicate this here, or could this have been done by calling rules_nixpkgs_dependencies in WORKSPACE as well?

fricklerhandwerk force-pushed the issue-182 branch 8 times, most recently from dd5bb89 to f5861e3 Compare March 9, 2022 11:41

fricklerhandwerk added 13 commits March 9, 2022 12:52

move core nixpkgs rules to separate module

0abb1a5

this is in preparation to support `bzlmod`

docs generation: reset record separator after match

389f8c3

otherwise the entire file will be consumed as the next line, skipping everything after the last match.

README: fix typos

7268ea3

generate separate README for core module

d19f882

move utilities to core module

7f91de8

extract go toolchain into separate module

b143b59

extract python toolchain into module

2f6b91f

extract java toolchain into module

8891efa

no alias for `default_java_toolchain`, since that seems to be used only internally, and a copy of what is usually distributed with Bazel.

extract CC toolchain into module

b08b196

leave aliases to existing files to ensure backwards compatibility. put CC toolchain in separate workspace, which is required because `nixpkgs_cc_configure()` references a file `cc.nix` from its own repository, which will be external at the call site.

split long sentence

f525a13

add notes on aliases to aid migration

972299e

extract Rust toolchain into module

8297cec

fricklerhandwerk force-pushed the issue-182 branch 3 times, most recently from 90ccdc5 to 8dad4fd Compare March 9, 2022 12:21

fricklerhandwerk added 3 commits March 10, 2022 10:36

wrap Java toolchain in workspace

6aa5eb6

otherwise internal references will break

wrap Python toolchain in workspace

95e8986

necessary to facilitate sandboxed integration testing, where the original source directory of the main workspace is not accessible without more jumping through hoops.

fix sandboxed integration test

22dbd3c

requires exposing `:srcs` for all the toolchain modules, so they can be added as `runfiles` to the sandbox.

fricklerhandwerk force-pushed the issue-182 branch from 8dad4fd to 22dbd3c Compare March 10, 2022 09:39

fricklerhandwerk marked this pull request as ready for review March 10, 2022 10:27

fricklerhandwerk requested a review from aherrmann March 10, 2022 10:27

fricklerhandwerk force-pushed the issue-182 branch 2 times, most recently from 7665ea2 to a19f1e1 Compare March 10, 2022 12:08

fricklerhandwerk mentioned this pull request Mar 10, 2022

Move tests into respective modules #194

Closed

fricklerhandwerk force-pushed the issue-182 branch from a19f1e1 to b680096 Compare March 10, 2022 12:23

aherrmann reviewed Mar 11, 2022

View reviewed changes

fricklerhandwerk added 5 commits March 15, 2022 08:31

move constraints definitions into core

add02d1

keep aliases for backwards compatibility

add module dependencies where possible

5819e1d

extract POSIX toolchain into module

a792c43

move BUILD.pkg template to core

b1d493e

it belongs there and is used only there

fix comment typo

d06eafc

fricklerhandwerk force-pushed the issue-182 branch from 35eab0e to d90d092 Compare March 15, 2022 07:31

fricklerhandwerk added 2 commits March 15, 2022 10:12

generalize scripts to generate documentation

885708a

the boilerplate for accessing runfiles turned out to be not needed. from now on one can just add targets or filenames for generating documentation and checking that the generated files are up to date under version control.

import sub-repositories based on original import type

29bae9a

while incomplete, this should cover most existing use cases seamlessly, such that existing users would not notice the refactoring.

fricklerhandwerk force-pushed the issue-182 branch from d90d092 to 29bae9a Compare March 15, 2022 09:18

fricklerhandwerk added 2 commits March 15, 2022 12:11

prepare reference docs generation for each submodule

610d6eb

this requires some more cleanup and refactoring before it is readable again, see TODOs.

add rendered documentation for Java

b4967da

fricklerhandwerk changed the title ~~extract bzlmod module for core rules~~ extract bzlmod modules for core rules and toolchains Mar 15, 2022

fricklerhandwerk mentioned this pull request Mar 15, 2022

Generate reference documentation for each toolchain module #197

Closed

fricklerhandwerk requested a review from aherrmann March 15, 2022 11:23

aherrmann approved these changes Mar 15, 2022

View reviewed changes

mergify bot merged commit b6b6e42 into master Mar 15, 2022

mergify bot deleted the issue-182 branch March 15, 2022 16:39

avdv mentioned this pull request Mar 16, 2022

Additional changes after bzlmod separation #200

Merged

avdv mentioned this pull request May 31, 2022

Run all tests and accumulate failures on CI #233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extract `bzlmod` modules for core rules and toolchains #193

extract `bzlmod` modules for core rules and toolchains #193

fricklerhandwerk commented Mar 7, 2022 •

edited

Loading

aherrmann left a comment

aherrmann left a comment

aherrmann Mar 15, 2022

fricklerhandwerk Mar 16, 2022

fricklerhandwerk Mar 16, 2022

aherrmann commented Mar 15, 2022

avdv commented Mar 16, 2022 •

edited

Loading

fricklerhandwerk commented Mar 16, 2022

avdv commented Mar 16, 2022

aherrmann commented Mar 16, 2022

extract bzlmod modules for core rules and toolchains #193

extract bzlmod modules for core rules and toolchains #193

Conversation

fricklerhandwerk commented Mar 7, 2022 • edited Loading

aherrmann left a comment

Choose a reason for hiding this comment

aherrmann left a comment

Choose a reason for hiding this comment

aherrmann Mar 15, 2022

Choose a reason for hiding this comment

fricklerhandwerk Mar 16, 2022

Choose a reason for hiding this comment

fricklerhandwerk Mar 16, 2022

Choose a reason for hiding this comment

aherrmann commented Mar 15, 2022

avdv commented Mar 16, 2022 • edited Loading

fricklerhandwerk commented Mar 16, 2022

avdv commented Mar 16, 2022

aherrmann commented Mar 16, 2022

extract `bzlmod` modules for core rules and toolchains #193

extract `bzlmod` modules for core rules and toolchains #193

fricklerhandwerk commented Mar 7, 2022 •

edited

Loading

avdv commented Mar 16, 2022 •

edited

Loading