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

gha: Print all *.log files, in a separate action #1655

Merged

Conversation

real-or-random
Copy link
Contributor

@real-or-random real-or-random commented Mar 10, 2025

Before this commit, we didn't print *_example.log files and
test_suite.log.

Printing is now handled in a separate action, which avoids code
duplication and makes the ci.yml file more readable. This changes the
folding/grouping of the log output in the GitHub Actions CI, but I
think the new variant is as good as the old one.

Furthermore, the condition for printing the logs is changed from
"always()" to "!cancelled()". This ensures that logs will still be
printed if previous steps such as the CI script failed, but that they
won't be printed if the entire run is cancelled (e.g., by clicking a
button in the UI or through a force-push to the PR). This is in line
with a recommendation in the GHA docs:
https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#always

@real-or-random real-or-random force-pushed the 202503-print-more-logs branch 8 times, most recently from ecc34c7 to 8960c5e Compare March 10, 2025 16:48
@real-or-random real-or-random changed the title ci: Print *_example.log and test-suite.log gha: Print all *.log files, in a separate action Mar 10, 2025
@real-or-random real-or-random marked this pull request as ready for review March 10, 2025 16:50
@real-or-random
Copy link
Contributor Author

real-or-random commented Mar 10, 2025

Ready for review (if CI passes :)).

If you prefer, I could split the changes into separate commits, but I don't think it's worth the hassle. It's not entirely clear how to separate these changes, and doing so would increase the sum of the diff because later commits will need to touch lines moved in earlier commits.

@real-or-random
Copy link
Contributor Author

@hebasto Can you take a look at this?

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK.

Comment on lines +23 to +29
shopt -s nullglob
for file in *.log; do
cat_file "$file"
done
Copy link
Member

@hebasto hebasto Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The make.log file is now printed along with other logs.

Does it make sense to separate logs by groups:

  • config.log and make.log
  • tests and benchmarks
  • examples

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this would produce better output, but I also think that *.log is good enough and much easier to maintain.

Comment on lines 7 to 11
run: |
group() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first line of the run command is printed as a group header in the Web interface. It could be a meaningful comment, for example: # Print the log files produced by ci/ci.sh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's annoying that it's not possible to get rid of this line, but making it contain a comment is a great idea. Fixed.

Before this commit, we didn't print *_example.log files and
test_suite.log.

Printing is now handled in a separate action, which avoids code
duplication and makes the ci.yml file more readable. This changes the
folding/grouping of the log output in the GitHub Actions CI, but I
think the new variant is as good as the old one.

Furthermore, the condition for printing the logs is changed from
"always()" to "!cancelled()". This ensures that logs will still be
printed if previous steps such as the CI script failed, but that they
won't be printed if the entire run is cancelled (e.g., by clicking a
button in the UI or through a force-push to the PR). This is in line
with a recommendation in the GHA docs:
https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#always
@real-or-random real-or-random force-pushed the 202503-print-more-logs branch from 8960c5e to 59860bc Compare March 10, 2025 20:09
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 59860bc.

Copy link
Contributor

@sipa sipa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 59860bc

@real-or-random real-or-random merged commit 03bbe8c into bitcoin-core:master Mar 11, 2025
117 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants