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

Sort, reorder and color build summary output #6649

Merged
merged 34 commits into from
Feb 2, 2024
Merged

Conversation

neha170
Copy link
Contributor

@neha170 neha170 commented Nov 1, 2023

Merge Checklist

All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)

  • The toolchain has been rebuilt successfully (or no changes were made to it)
  • The toolchain/worker package manifests are up-to-date
  • Any updated packages successfully build (or no packages were changed)
  • Packages depending on static components modified in this PR (Golang, *-static subpackages, etc.) have had their Release tag incremented.
  • Package tests (%check section) have been verified with RUN_CHECK=y for existing SPEC files, or added to new SPEC files
  • All package sources are available
  • cgmanifest files are up-to-date and sorted (./cgmanifest.json, ./toolkit/scripts/toolchain/cgmanifest.json, .github/workflows/cgmanifest.json)
  • LICENSE-MAP files are up-to-date (./SPECS/LICENSES-AND-NOTICES/data/licenses.json, ./SPECS/LICENSES-AND-NOTICES/LICENSES-MAP.md, ./SPECS/LICENSES-AND-NOTICES/LICENSE-EXCEPTIONS.PHOTON)
  • All source files have up-to-date hashes in the *.signatures.json files
  • sudo make go-tidy-all and sudo make go-test-coverage pass
  • Documentation has been updated to match any changes to the build system
  • Ready to merge

Summary

Sort summary, reorder summary, add option to color output on terminal

Change Log
  • Add new make option LOG_COLOR with options auto (default), always, never
  • LOG_COLOR = always: Color both terminal output and logs
  • LOG_COLOR = auto: Color only terminal output but not the logs
  • LOG_COLOR = never: Disable color in both log and terminal output
  • Remove color codes from logs when LOG_COLOR is not always
  • Print SRPMs in build summary in alphabetical order
  • Reorder summary (print failed, built and prebuilt ones at the end)
  • Add new color package import for golang
Does this affect the toolchain?

NO

Associated issues
  • Some initial discussion can be found here #46098944
Test Methodology
  • Locally built and confirmed the changes in build summary

Summary( before without sort/color):
image

Sorted and colored summary:
image

Logs are non-colored when option is not LOG_COLOR=always:
image

On dark background
image

@neha170 neha170 requested a review from a team as a code owner November 1, 2023 21:51
@microsoft-github-policy-service microsoft-github-policy-service bot added the main PR Destined for main label Nov 1, 2023
@neha170 neha170 changed the title Point to logs on build/test failure, sort summary Point to logs on build/test failure, sort and color summary Nov 10, 2023
@corvus-callidus
Copy link
Contributor

Is there a way to make the color optional for accessibility purposes?

@neha170
Copy link
Contributor Author

neha170 commented Nov 11, 2023

Is there a way to make the color optional for accessibility purposes?

@https://github.com/dmcilvaney Are we ok adding another make option for choosing to keep color on or off?
I am thinking something like LOG_COLOR=on/off like we have LOG_LEVEL...

Copy link
Contributor

@PawelWMS PawelWMS left a comment

Choose a reason for hiding this comment

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

Let's update the SetToSliceAll function's name to make it clear we're allowing duplicates in this case (see: comments near that function).

And let's try to strip the colouring from file logs to keep the easy to read, parse, and search through them. You can try modifying newWriterHook inside toolkit/tools/internal/logger/writerhook.go so that it cuts out the colouring when useColors is false.

Ideally, let's also try to find and use existing libraries to help with coloring the output.

Minor comments aside from that.

Thank you for cleaning this up!

@dmcilvaney
Copy link
Contributor

Is there a way to make the color optional for accessibility purposes?

@https://github.com/dmcilvaney Are we ok adding another make option for choosing to keep color on or off? I am thinking something like LOG_COLOR=on/off like we have LOG_LEVEL...

I think always, never, auto is the tuple I've always seen with most linux tools, but the flag name is fine with me.

@neha170
Copy link
Contributor Author

neha170 commented Nov 15, 2023

Keeping this PR focused on summary (color, sort, reorder) and making a separate PR for changes requested in https://microsoft.visualstudio.com/OS/_workitems/edit/46098944
PR: #6768

@neha170 neha170 changed the title Point to logs on build/test failure, sort and color summary Sort, reorder and color build summary output Nov 15, 2023
@neha170 neha170 merged commit 07ec048 into main Feb 2, 2024
11 checks passed
@neha170 neha170 deleted the nehaagarwal/toolkit_log branch February 2, 2024 17:52
neha170 added a commit that referenced this pull request Feb 2, 2024
Add new make option LOG_COLOR with options auto (default), always, never. 'always' colors both terminal output and logs; 'auto' colors only terminal output; 'never' disables color in both.
neha170 added a commit that referenced this pull request Feb 2, 2024
Add new make option LOG_COLOR with options auto (default), always, never. 'always' colors both terminal output and logs; 'auto' colors only terminal output but not the logs; 'never' disables color in both
jslobodzian pushed a commit that referenced this pull request Feb 7, 2024
Add new make option LOG_COLOR with options auto (default), always, never. 'always' colors both terminal output and logs; 'auto' colors only terminal output but not the logs; 'never' disables color in both
Adub17030MS pushed a commit that referenced this pull request Feb 9, 2024
Add new make option LOG_COLOR with options auto (default), always, never. 'always' colors both terminal output and logs; 'auto' colors only terminal output but not the logs; 'never' disables color in both
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
main PR Destined for main
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants