-
Notifications
You must be signed in to change notification settings - Fork 766
[tooling] Add support for the Task (go-task) task runner #3863
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
Conversation
b0bcd19
to
e281954
Compare
e281954
to
6775db8
Compare
This repo has a lot of tooling but previously had no easy entrypoint other than its github action workflows. This complicated discoverability and CI reproduceability. This change adds support for invoking every command intended to be run in CI via the [Task](https://taskfile.dev) task runner. Since `Task` is written in golang, it is easily installable without additional dependency. Alternatives considered: - [GNU Make](https://www.gnu.org/software/make/) is a build system, and has a lot more complexity and features than we need for a task runner. - [Just](https://github.com/casey/just) has a similar featureset to Task but since it is written in Rust it isn't possible to `go run` on a system without it.
6775db8
to
a8f1281
Compare
@@ -20,10 +20,6 @@ fi | |||
# the instructions to build non-portable BLST. | |||
source ./scripts/constants.sh | |||
|
|||
# Enable subnet testing by building xsvm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was always a bit weird to have this script build xsvm but not avalanchego. But which version to build - with race detection or without? With the introduction of a task runner, these scripts can become more focused and composable. The test-e2e task can run the build and build-xsvm tasks before this script, and the test-e2e-ci task can reuse the build-race and build-xsvm tasks.
@@ -33,6 +33,9 @@ | |||
default = pkgs.mkShell { | |||
# The Nix packages provided in the environment | |||
packages = with pkgs; [ | |||
# Task runner | |||
go-task |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we have nix install go-task and go-task install nix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More like:
./scripts/run_task.sh install-nix
(usesgo run
to executetask
to install nix)nix develop
(starts nix shell that includes go-task)./scripts/run_task.sh [task]
(invokestask
from the nix shell) andtask
(invoked directly) are now equivalent
Using the wrapper script simplifies usage in CI since task
can always be invoked whether from a nix shell or a regular shell. For a user manually installing task
or using a nix shell, they're free to use task
directly.
fef9233
to
c030dcf
Compare
c030dcf
to
cb79efc
Compare
[[ -f "$file" ]] || continue | ||
|
||
# Search for scripts/* except for scripts/run_task.sh | ||
MATCHES=$(grep -H -n -P "scripts/(?!run_task\.sh)" "$file" || true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this essentially ensures we only use scripts/run_task.sh
and nothing else in scripts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, the intention is for tasks to be the common interface for both CI and local execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good. One thing that I'd rather seeing is having the task
tool itself living within our repo ( or a clone that we maintain, which can be used across multiple repos )
if command -v task > /dev/null 2>&1; then | ||
exec task "${@}" | ||
else | ||
go run github.com/go-task/task/v3/cmd/[email protected] "${@}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using the above version is good, but specifying the hash instead of the tag is even better:
go run github.com/go-task/task/v3/cmd/task@58ab26c "${@}"
since hashes cannot be retroactivly modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tags are as good as hashes because go by default uses a proxy that effectively renders tags immutable.
Why would we want to maintain the tool in our repo? We depend on many other tools that live outside our repo, I'm not sure what makes |
I think if we're worried about the task repository compromised, we can always fork it to our organization and then carefully review the upstream commits and upgrade the forked repository. |
flake.nix
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
# To use: | |||
# - install nix: https://github.com/DeterminateSystems/nix-installer?tab=readme-ov-file#install-nix | |||
# - install nix: `./scripts/run_task.sh install_nix` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
task: Task "install_nix" does not exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty cool
Why this should be merged
This repo has a lot of tooling but previously had no easy entrypoint other than its github action workflows. This complicated discoverability and CI reproduceability.
This change adds support for invoking every command intended to be run in CI via the Task task runner. Since
Task
is written in golang, it is easily installable without additional dependency.The intention is to ensure that every development task can be invoked with
scripts/run_task.sh [task]
and that the logic of task execution - even for tasks primarily intended for usage in CI - be defined in a given task rather than in a github action workflow.Alternatives considered:
go run
on a system without it.task
also supports parallel execution which may prove useful in the future.How this was tested
CI, locally for tasks that aren't tested by CI (e.g. build-image)
Need to be documented in RELEASES.md?
N/A