Initial contribution of code from aspect-build/bazel_rules_template #1

alexeagle · 2021-11-29T16:19:21Z

Upstreaming this code to the SIG, since that group is more qualified to recommend how new rulesets are formed.

cgrindel

Looks good. Thanks for putting it together.

cgrindel · 2021-11-29T16:33:31Z

.bazelrc

@@ -0,0 +1,13 @@
+# Bazel settings that apply to this repository.
+# Take care to document any settings that you expect users to apply.
+# Settings that apply only to CI are in .github/workflows/ci.bazelrc


What are your thoughts on leveraging Bazel's --config capability? It allows all of the *.bazelrc files to live in a single spot. They then can be enabled with the --config flag. I use it in rules_spm: ci.bazelrc, enable for CI.

Yes, I've used that --config=ci scheme in the past as well. The main reason to make another file is because in a complex CI setup, you end up calling bazel in lots of different places. You need to query things to discover what to build & test, someone's code requires a different configuration (test fails without -c opt for example).
It's easy to accidentally forget the --config=ci flag in one of the many invocations of bazel, or especially hard to ensure that later maintainers of the CI scripts don't omit it.

So, the pattern I usually use is to copy the ci.bazelrc file into the system or user's location at the beginning of the pipeline. This way the CI rc settings are always applied without needing to remember what flags to pass each bazel call.

To be fair, in this repo, I don't think it makes much difference which we pick. If we want to simplify things, I wouldn't even bother with a separate rc file, just use the ci config in the same .bazelrc with the rest.

My preference is to keep all the CI configuration adjacent, more strongly than to keep all the bazelrc's adjacent. I'm open to discussion on it of course.

It's easy to accidentally forget the --config=ci flag in one of the many invocations of bazel, or especially hard to ensure that later maintainers of the CI scripts don't omit it.

So, the pattern I usually use is to copy the ci.bazelrc file into the system or user's location at the beginning of the pipeline. This way the CI rc settings are always applied without needing to remember what flags to pass each bazel call.

IIUC the same can be achieved by using build:ci etc. in .bazelrc and then writing user.bazelrc or ~/.bazelrc with the following contents at the beginning of the CI pipeline.

build --config=ci query --config=ci ...

cgrindel · 2021-11-29T16:36:42Z

.github/workflows/ci.yaml

+    steps:
+      # Checks-out your repository under $GITHUB_WORKSPACE, so your job can access it
+      - uses: actions/checkout@v2
+      - uses: bazelbuild/setup-bazelisk@v1


Is this necessary? I am able to use Bazelisk without a special setup step.

Right, you showed me that surprisingly GitHub Actions base images already have bazelisk

I'll update the README on this setup-bazelisk thing to indicate that it's optional and drop this line.
bazelbuild/setup-bazelisk@main...alexeagle:patch-1

cgrindel · 2021-11-29T16:50:37Z

.pre-commit-config.yaml

+      - id: buildifier
+      - id: buildifier-lint
+  # Enforce that commit messages allow for later changelog generation
+  - repo: https://github.com/commitizen-tools/commitizen


I have not used commitizen or prettier. However, I wonder if this should be part of the template. For instance, I tend to commit often during development with varying degrees of useful commit messages. I then do a squash commit when the PR is merged to main. The squash commit is where I spend time crafting a useful message. As this config stands, would the developer be forced to create "meaningful" commit messages for every commit on their branches?

I'm pretty sure that commitizen should be part of the setup, so that contributors remember to indicate fix/feature/breaking change. Otherwise it's so hard to determine next release version.

It will remind you about the --no-verify option whenever it fails, so when I'm doing quick hacking like that I just add the option.

cgrindel · 2021-11-29T16:55:17Z

BUILD.bazel

+)
+
+# This declares the release artifact users
+pkg_tar(


I have seen a couple of discussions about whether a release package declaration is necessary. For my repos, I have relied on Github supporting https://github.com/cgrindel/rules_xxx/archive/v0.1.0.tar.gz. This along with the SHA256 value seems sufficient. Are there good arguments against this practice?

Yes, see discussion here
bazel-contrib/SIG-rules-authors#11 (comment)

It's both to get a stable SHA that you can predict ahead-of-time (so the WORKSPACE snippet can be updated in the release process for example) and also to ensure users don't install your tests and examples and docs

Okay got this resolved, now the release automation works without it :)
Thanks @aherrmann

WORKSPACE

mylang/defs.bzl

cgrindel · 2021-11-29T17:06:01Z

mylang/private/toolchains_repo.bzl

@@ -0,0 +1,113 @@
+"""Create a repository to hold the toolchains


I am not sure if toolchains should be included in the default repo structure. Of all the Bazel rule repositories that I have implemented, I only need toolchain stuff for one of them. Perhaps we can leave this as documentation or a separate template?

Yeah I agree, in using this template a lot of the work is correctly removing all the toolchain bits you don't need.

However I think if we just document what to do, it will be hard to follow how to create a toolchain, and we'll end up with a lot of "code in comments" which is harder to keep it working.
If we make a separate template, they will diverge over time and we'll double the maintenance effort.
There are fancy systems for code templating like "cookiecutter" from python-land, where you can interactively ask the user what features to enable, but I don't think the SIG has time to invest in something fancier than just GitHub templates built-in feature.

So I'm torn about what is the better way to do this.

mylang/tests/BUILD.bazel

cgrindel · 2021-11-29T17:12:38Z

mylang/private/versions.bzl

+
+# The integrity hashes can be computed with
+# shasum -b -a 384 [downloaded file] | awk '{ print $1 }' | xxd -r -p | base64
+TOOL_VERSIONS = {


I don't quite understand how this is being used. Can you provide more detail?

it's mirrored from the upstream, providing the bazel downloader with info it needs to place these in the download cache

.github/workflows/release.yml

.gitignore

BUILD.bazel

CONTRIBUTING.md

WORKSPACE

Co-authored-by: Andreas Herrmann <andreash87@gmx.ch>

Yannic

Great start! I fully expect this template to evolve over time, so let's not try to be perfect right from the beginning. Feel free to address any of my nits in follow-ups.

Yannic · 2021-11-29T18:33:49Z

BUILD.bazel

+load("@rules_pkg//:pkg.bzl", "pkg_tar")
+load("@bazel_gazelle//:def.bzl", "gazelle", "gazelle_binary")
+
+gazelle_binary(


I'm not sure I'm in favor of adding gazelle as a dependency to virtually all rule sets. It's super nice to have it, but it also pulls in a ton of stuff that rules definitely don't want to be part of their release artifacts. Maybe it would be better to run this in a lint step on CI and print the produced diff that users would need to pass. You'd likely want the lint step in any case to ensure that it's out of sync (I don't use gazelle myself, so not sure if there's a "gazelle lint" command. if it doesn't exist, we should make it happen IMO)

The gazelle dependency isn't exposed to users. Nothing in this package is loaded by them.

(In my original formulation we wouldn't even ship this file to users, but @aherrmann convinced me that the GH archive of the repo works)

Yannic · 2021-11-29T18:37:53Z

WORKSPACE

@@ -0,0 +1,40 @@
+# Declare the local Bazel workspace.
+# This is *not* included in the published distribution.


IMO, the release tarball should contain a WORKSPACE file

Bazel ignores the WORKSPACE that you include.
But, I've removed the release tarball anyhow.

Yannic · 2021-11-29T18:39:41Z

WORKSPACE

@@ -0,0 +1,40 @@
+# Declare the local Bazel workspace.


Maybe rename to WORKSPACE.bazel for consistency with BUILD files?

there's a bug with WORKSPACE.bazel - I can track it down if you like. We had to give up on it for all clients so far.

Yannic · 2021-11-29T18:41:28Z

docs/BUILD.bazel

+# so that the dependency on stardoc doesn't leak to them.
+load("@aspect_bazel_lib//lib:docs.bzl", "stardoc_with_diff_test", "update_docs")
+
+stardoc_with_diff_test(


Has this rule been fixed on Windows? If not, please add at least a warning.

This rule works on windows, but I think you mean the underlying bazel-skylib diff_test being broken on windows at some recent commit?

mylang/defs.bzl

Yannic · 2021-11-29T18:44:15Z

mylang/private/BUILD.bazel

+
+filegroup(
+    name = "package_content",
+    srcs = glob([


Are you intentionally not including the BUILD file here?

Yannic · 2021-11-29T18:46:15Z

mylang/private/toolchains_repo.bzl

+
+# Add more platforms as needed to mirror all the binaries
+# published by the upstream project.
+PLATFORMS = {


Does this need to be exported from the file?

Maybe not, but I've often needed to access it somewhere else that wants to loop over them

Yannic · 2021-11-29T18:50:20Z

mylang/repositories.bzl

+_DOC = "Fetch external tools needed for mylang toolchain"
+_ATTRS = {
+    "mylang_version": attr.string(mandatory = True, values = TOOL_VERSIONS.keys()),
+    "platform": attr.string(mandatory = True, values = PLATFORMS.keys()),


prefer setting a sane default here so that not every user has to specify it when using the rules?

Also, I think platform here is a bit of an anti-pattern. IMO, we should add+register toolchains for all platforms and let Bazel select the right toolchain.

users don't typically call this rule themselves. this is inside the loop that creates each platform-specific toolchain

Also, I think platform here is a bit of an anti-pattern. IMO, we should add+register toolchains for all platforms and let Bazel select the right toolchain.

As I understand the code that is what it does in the loop below.

Another important bit is also documented here:

Users can avoid this macro and do these steps themselves, if they want more control.

I.e. users should be able to provide their own toolchains with their own platform constraints.

Yannic · 2021-11-29T18:52:55Z

mylang/repositories.bzl

+# Remaining content of the file is only used to support toolchains.
+########
+_DOC = "Fetch external tools needed for mylang toolchain"
+_ATTRS = {


From a style perspective, I prefer to define attrs inline in the rule definition (unless they are common attrs that are used by many rules).

I've enjoyed the order of reading the file that starts with the docstring, then the public API. it's more like reading a function in Java where you have the Javadoc, then the params, then the implementation.

But I don't care that much, can go with yours

mylang/tests/versions_test.bzl

alexeagle · 2021-12-12T00:42:21Z

@aherrmann @cgrindel @Yannic sorry for the delay, I finally got around to trying the pure-GH-archive approach for releases, still adds the WORKSPACE snippet to the release notes automatically just by pushing a tag.

PTAL :)

cgrindel · 2021-12-12T17:36:34Z

@alexeagle Just looked at the pure-GH-archive changes. Very cool. I am going to incorporate it into my repositories, this week.

aherrmann · 2021-12-13T14:45:03Z

@aherrmann @cgrindel @Yannic sorry for the delay, I finally got around to trying the pure-GH-archive approach for releases, still adds the WORKSPACE snippet to the release notes automatically just by pushing a tag.

PTAL :)

Thank you, looks good!

alexeagle and others added 10 commits November 29, 2021 08:12

Initial commit

bac72fa

feat: add more pre-commit checks

bed4214

Add toolchain

f14db11

improve instructions and cleanup

70b3ba0

Update repositories.bzl

42b3792

feat: add gazelle for bzl_library

0bc8706

feat: add docs folder

1850061

feat: add unittest

dfe46e5

feat: document release

8dfa25f

ci: release only require a tag, no commits

7469600

alexeagle requested review from gravypod, aiuto and Yannic November 29, 2021 16:19

alexeagle mentioned this pull request Nov 29, 2021

Provide a GitHub Template repo for new rulesets bazel-contrib/SIG-rules-authors#10

Closed

cgrindel requested changes Nov 29, 2021

View reviewed changes

code review comments

d98e9e2

aherrmann reviewed Nov 30, 2021

View reviewed changes

.github/workflows/release.yml Outdated Show resolved Hide resolved

.gitignore Show resolved Hide resolved

BUILD.bazel Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

WORKSPACE Outdated Show resolved Hide resolved

alexeagle and others added 3 commits November 30, 2021 08:38

Update .gitignore

6734609

Co-authored-by: Andreas Herrmann <andreash87@gmx.ch>

Update CONTRIBUTING.md

d47070f

Co-authored-by: Andreas Herrmann <andreash87@gmx.ch>

Update WORKSPACE

589fea6

Co-authored-by: Andreas Herrmann <andreash87@gmx.ch>

Yannic approved these changes Dec 6, 2021

View reviewed changes

alexeagle force-pushed the initial branch from 2c8f753 to 422f9a0 Compare December 12, 2021 00:37

cleanup: remove pkg_tar

01baa34

alexeagle force-pushed the initial branch from 422f9a0 to 01baa34 Compare December 12, 2021 00:41

alexeagle merged commit 101f370 into main Dec 13, 2021

		@@ -0,0 +1,113 @@
		"""Create a repository to hold the toolchains

		@@ -0,0 +1,40 @@
		# Declare the local Bazel workspace.
		# This is not included in the published distribution.

Initial contribution of code from aspect-build/bazel_rules_template #1

Initial contribution of code from aspect-build/bazel_rules_template #1

Conversation

alexeagle commented Nov 29, 2021

cgrindel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yannic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeagle commented Dec 12, 2021 • edited Loading

cgrindel commented Dec 12, 2021

aherrmann commented Dec 13, 2021

alexeagle commented Dec 12, 2021 •

edited

Loading