just icon indicating copy to clipboard operation
just copied to clipboard

Feature request: group logs when running just in GitHub actions

Open GauBen opened this issue 1 year ago • 5 comments

Hey!

GitHub supports slightly interactive action output logs: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#grouping-log-lines

It would be cool if just output had these markers when running in GitHub actions, it can be guessed by reading the env var GITHUB_ACTIONS, which is expected to be true (doc)

Would it be a useful addition to just?

I'd love to contribute this feature but I'm not very familiar with rust, I would need a few hints

GauBen avatar Nov 26 '24 17:11 GauBen

Very cool idea!

I think we should add it first behind a flag, and then later think about turning it on automatically if GITHUB_ACTIONS, depending on whether grouped logs are always useful, or if they have drawbacks or are too noisy.

Log groups have a start and an end, so I guess we would print out:

::group::RECIPE_NAME

Before running a recipe, and:

::endgroup::

When the recipe was finished.

One thing I'm curious about is what happens if you start a group within another group, which might happen if you recursively invoke just:

foo:
  just bar

You'd get something like:

::group::foo
::group::bar
::endgroup::
::endgroup::

Which I'm not sure would display correctly.

Here's how I would start with this feature:

  • Open a draft PR which prints out the group markers unconditionally, without worrying about a flag. You can do this in Recipe::run. I think you'll only need to add eprintln!("::group::{}", self.name) and eprintln!("::endgroup::").
  • We'll also need to see how it looks, so in the same PR, add a new step to .github/workflows/ci.yaml which invokes just on some recipes in order to see what they look like. You can add some dummy recipes to the justfile.
  • Then we can see what it actually looks like, what happens if you invoke just recursively, etc.
  • Assuming it looks good, I can then help you with adding a flag. (Which shiouldn't be too hard, it's in config.rs.)

casey avatar Nov 26 '24 19:11 casey

I was thinking about more granular groups: each recipe line would get its group. Let's try something! Thanks for your quick reply

GauBen avatar Nov 26 '24 20:11 GauBen

Sounds good. We should do whatever is most usable. I don't really know what grouped logs look like, so if giving each recipe line its own group looks good, then that's fine. I suppose it might depend on the commands being run. If a single command generates a bunch of output, then putting it into a group is probably best. However, if there are a bunch of commands run which don't produce much output, then adding each one to a group might not be totally usable. We should probably experiment with both cases to see what works.

casey avatar Nov 26 '24 20:11 casey

Starting #2482 here!

GauBen avatar Nov 26 '24 20:11 GauBen

Just in case this is useful for others:

In https://github.com/influxdata/datafusion-udf-wasm we are using manual grouping in our recipes like this:

# check Rust formatting
check-rust-fmt:
    @echo ::group::check-rust-fmt
    cargo fmt --all -- --check
    @echo ::endgroup::

This mostly works, however sometimes GitHub fails to include a few lines into the respective groups, especially for bash-based recipes like this:

download-deps:
    #!/usr/bin/env bash
    set -euo pipefail

    echo ::group::download-deps
    set -x

    # this line sometimes appears BEFORE the group
    mkdir -p downloads

    # ...

    set +x
    echo ::endgroup::

I think this might be due to stdout and stderr getting slightly out-of-sync for a moment (they are after all separate streams), because set -x triggers writes to stderr but the group control messages are on stdout. Similar things might happen when cargo is called although there it seems to rarely happen, likely due to the slight delay that the sub-process call has.

Work-arounds that I can think of:

  • not using stderr at all (but now you need to redirect stdout for all sub-processes)
  • adding a flush/wait call around the group-control messages

crepererum avatar Sep 03 '25 10:09 crepererum