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

docs: improve documentation #109

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jjmaestro
Copy link
Contributor

@jjmaestro jjmaestro commented Oct 29, 2024

  • Revamp README adding an improved version of the Bzlmod and WORKSPACE snippets that are available in the release page. IMHO the release page should now remove those snippets and point to the README.

  • Split the Bzlmod / WORKSPACE docs from deb_index and remove a bunch of old references. Also mark it all as legacy since the present and future is all Bzlmod.

  • Create apt/extensions docs.

  • Add a whole common section to both docs with (hopefully :) easy-to-follow documentation inspired by my answer to a Slack question.

It's probably best and easiest to check the rendered markdown in my branch:

Both are very similar since I've copy-pasted the common part of the doc. I tried refactoring part of the doc to reuse it in both places but I couldn't use a template replacement in the apt.install macro docstring.

NOTE: I had to update aspect_bazel_lib and bazel_skylib plus add cache.bzl as a dependency in apt/private/BUILD.bzl to get bazel run //docs:update to work.

README.md Outdated Show resolved Hide resolved
@jjmaestro
Copy link
Contributor Author

@thesayyn there seems to be some checks awaiting approval :-? Can you approve them so they run? Thanks!!

@jjmaestro
Copy link
Contributor Author

@thesayyn the failures I get in those checks match the weird failure I got while trying to generate the docs and why I added the cache.bzl dependency in apt/private/BUILD.bazel.

I'm not sure what's going on, It seems to only be failing in 6.4.0 though... if you can help me understand this 🙏 Meanwhile, I'll try to repro it locally as well.

@jjmaestro
Copy link
Contributor Author

OK, I've dug a bit into this and I think the issue is that, somehow (still have to check why) I needed to add cache.bzl as a dependency when running with a modern Bazel but that doesn't exist in @bazel_tools for old Bazels, so it breaks in 6.4.0.

Now, here's the thing, cache.bzl didn't exist until three months ago! (bazelbuild/bazel@bd63c61) so I'm not sure why / how is that even being required, I definitely need to check more on that.

But, once I remove it again and re-run the tests for Bazel 6.4.0, I get another error

ERROR: /src/workspace/examples/ubuntu_snapshot/BUILD.bazel:27:6: in tar rule //examples/ubuntu_snapshot:group: 
Traceback (most recent call last):
        File "/root/.cache/bazel/_bazel_root/a08c2e4811c846650b733c6fc815a920/external/aspect_bazel_lib~2.9.3/lib/private/tar.bzl", line 304, column 38, in _tar_impl
                src[DefaultInfo].files_to_run.repo_mapping_manifest
Error: 'FilesToRunProvider' value has no field or method 'repo_mapping_manifest'

And sure enough, when I check, that was added in bazelbuild/bazel@3575211 so again, very modern Bazel. Yet, aspect_bazel_lib v2.9.3 is using it, and declaring a bazel_compatibility = [">=6.0.0"].

And look what I found 😄 bazel-contrib/bazel-lib#975 and bazel-contrib/bazel-lib#976

So, I'm going to try and use 2.9.1 which doesn't have the bug and will see if I can debug the cache.bzl thing which is quite weird (to my inexperienced eye 😅 )

@jjmaestro
Copy link
Contributor Author

OK, so there's something very weird going on when trying to make this work with 6.4.0 and I don't know how to fix it. @thesayyn does it make any sense to support 6.x when, unless I'm mistaken, GoogleContainerTools/rules_distroless has a .bazelversion of 7.2.0?

I'm happy to make a PR bumping the CI to 7.x, using the latest CI config (I've noticed it's hardcoded to a commit that e.g. prevents testing with 8.x.

Can you let me know? I'd really like to unblock and land this so I can rebase the rest of the PRs.

Thanks!!

27242f4 pushed lock version to `12` (my guess is Bazel 8?) plus other
changes that e.g. don't match running with `.bazelversion`'s `7.3.1`.

Regardless, the `MODULE.bazel.lock` file is still not stable and e.g.
just running it in another operating system causes things to change...
so let's just ignore it like I did for the `e2e` tests in 70c14c0
Revamp README adding an improved version of the Bzlmod and `WORKSPACE`
snippets that are available in the release page.

IMHO the release page should now remove those snippets and point to the
README.
@jjmaestro
Copy link
Contributor Author

@thesayyn I've rebased the PR removing the changes to bazel_skylib and aspect_bazel_lib since I've seen #111 removed support for Bazel 6 so everything should work a-OK in Bazel 7+ :) I've also rebased this on top of #112 which I hope you'll accept, because the MODULE.bazel.lock is constantly changing and breaking the rebases, and I really don't see it adding any value.

Also, could you approve the CI run? Thanks!!

docs/deb_index.md Outdated Show resolved Hide resolved
Split the Bzlmod / `WORKSPACE` docs from `apt.install` macro and remove
a bunch of old references. Also mark it all as legacy since the present
and future is all Bzlmod.

Create apt/extensions docs.

Add a whole common section to both docs with (hopefully :)
easy-to-follow documentation inspired by my answer to a Slack question
(https://bazelbuild.slack.com/archives/CA3NW13MH/p1729804678924819).
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