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.

Sign up for GitHub

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

Jump to bottom

base: logging enhancements: multiple debug output levels #243

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

base: logging enhancements: multiple debug output levels #243

wants to merge 2 commits into from

Conversation

mascguy
Copy link
Member

@mascguy mascguy commented May 30, 2021 •
edited

Per recent discussion on the dev mailing list, implement support for multiple levels of debug logging support.

Link to discussion threads:

Corresponding work for macports-ports: PR 11184 - base support: logging enhancements: multiple debug output levels

mascguy
mascguy commented May 30, 2021
View reviewed changes
src/port1.0/portutil.tcl Outdated Show resolved Hide resolved
mascguy
mascguy commented May 30, 2021
View reviewed changes
src/port1.0/port.tcl Outdated Show resolved Hide resolved
@mascguy mascguy mentioned this pull request May 30, 2021
Closed
@mascguy mascguy force-pushed the mascguy-debug3 branch 2 times, most recently from 3726e3d to 36ac989 Compare May 30, 2021 17:08
mascguy
mascguy commented May 30, 2021
View reviewed changes
src/port/port.tcl Outdated Show resolved Hide resolved
@mascguy mascguy changed the title base: add lower-level debug output, via new levels debug2/debug3 base: add lower-level debug output, via new levels debug[1-3] May 30, 2021
@mascguy mascguy force-pushed the mascguy-debug3 branch 4 times, most recently from bc4ca8a to 36a28c1 Compare May 30, 2021 18:06
mascguy
mascguy commented May 30, 2021
View reviewed changes
src/macports1.0/macports.tcl Outdated
debug {
if {[ui_isset ports_debug]} {
return stderr
} else {
return {}
}
}
debug[0-9] {
Copy link
Member Author

@mascguy mascguy May 30, 2021 •
edited

Choose a reason for hiding this comment

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

Internally, levels 0-9 supported. But range 0-3 enforced via argument checks, etc.

@mascguy mascguy force-pushed the mascguy-debug3 branch 2 times, most recently from e78f220 to 99ca671 Compare May 30, 2021 18:55
mascguy
mascguy commented May 30, 2021
View reviewed changes
src/port/port.tcl Outdated Show resolved Hide resolved
@mascguy mascguy force-pushed the mascguy-debug3 branch 3 times, most recently from f06618a to b78b2cd Compare May 30, 2021 20:44
@mascguy mascguy changed the title base: add lower-level debug output, via new levels debug[1-3] base: add lower-level debug output, via new levels debug1-3 May 31, 2021
@mascguy mascguy force-pushed the mascguy-debug3 branch 7 times, most recently from e97ada2 to ac8e0e5 Compare May 31, 2021 13:38
raimue
raimue reviewed Jun 1, 2021
View reviewed changes
src/port/port.tcl Outdated Show resolved Hide resolved
src/pextlib1.0/Pextlib.c Outdated Show resolved Hide resolved
@mascguy mascguy force-pushed the mascguy-debug3 branch 12 times, most recently from 50ac08a to 8af5ebd Compare June 4, 2021 22:15
@cjones051073
Copy link
Member

Having 4 levels of debug seems a bit excessive to me. I would have thought that say 3 would easily be enough granularity? I also find having level ‘0’ called debug, whilst 1-3 called debugN a little confusing. I would personally suggest making debug and debug1 the same level (so just make debug an alias to debug1 for compatibility) and then have debug2 and debug3 for two increasingly more detailed levels.

@mascguy
Copy link
Member Author

mascguy commented Jun 5, 2021 •
edited

Having 4 levels of debug seems a bit excessive to me. I would have thought that say 3 would easily be enough granularity? I also find having level ‘0’ called debug, whilst 1-3 called debugN a little confusing. I would personally suggest making debug and debug1 the same level (so just make debug an alias to debug1 for compatibility) and then have debug2 and debug3 for two increasingly more detailed levels.

Sure, and three levels isn't set in stone; that was merely an arbitrary choice to start with.

Overall, this effort is still evolving, and the goal is to support logging based on namespaces/packages - similar to Python and Java - to allow more control over output and levels. (Per recommendations by folks on the mailing list.)

The latter hasn't been implemented yet, but watch this space.

Further thoughts/comments/suggestions welcome!

@mascguy
Copy link
Member Author

mascguy commented Jun 5, 2021 •
edited

Relative to namespace/package logging, I'm currently evaluating the TCL logger package, to avoid reinventing the wheel. Based on a cursory look, it may provide a nice foundation for what we're trying to accomplish.

If anyone has any experience with that package - or if there's a better option - please let me know!

@mascguy mascguy marked this pull request as draft June 5, 2021 19:06
@mascguy mascguy changed the title base: add lower-level debug output, via new levels debug1-3 base: logging enhancements: package/namespace loggers; multiple debug output levels Jun 8, 2021
@mascguy mascguy force-pushed the mascguy-debug3 branch 2 times, most recently from 297ed4c to 4b64623 Compare June 18, 2021 20:59
@mascguy mascguy removed the request for review from kencu June 22, 2021 14:17
cjones051073
cjones051073 approved these changes Jul 15, 2021
View reviewed changes
Copy link
Member

@cjones051073 cjones051073 left a comment

Choose a reason for hiding this comment

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

Seems OK to me, thanks for this.

src/pextlib1.0/tests/curl.tcl Outdated
grepper $tempfile {gz_yes}
# CNielsen: Temporarily disable compressed fetch, as failing after 30-second timeout
#curl fetch --enable-compression https://www.whatsmyip.org/http-compression-test/ $tempfile
#grepper $tempfile {gz_yes}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn’t seem directly related to the debug level changes, or am I missing something ?

Copy link
Member Author

@mascguy mascguy Jul 15, 2021 •
edited

Choose a reason for hiding this comment

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

This was necessary back a month or so ago, as the test consistently failed due to timeout. (It wasn't intended to be kept long-term though.)

Ultimately it will be reverted in this PR, or changed to use a different testing endpoint if necessary. (The latter would be based on guidance from Josh and/or Ryan.)

@cooljeanius
Copy link

Having 4 levels of debug seems a bit excessive to me. I would have thought that say 3 would easily be enough granularity?

4 levels is how many Fink has, IIRC...

@mascguy mascguy marked this pull request as ready for review August 29, 2023 03:39
@mascguy
Copy link
Member Author

mascguy commented Aug 30, 2023

This PR has been changed to ready-for-review, with the caveat that merge conflicts need to be resolved. (Will try to get to that over the next few days.)

Relative to namespace-scoped logging - something not currently included - I'm wondering if we should shelve that at this point?

Reason being, we'd need to support a way to selectively set logging levels per-namespace. (Presumably via some type of config file, such as macports.conf.) And that adds a whole new level of complexity. Not to mention the question of whether that would ever be taken advantage of.

@mascguy mascguy changed the title base: logging enhancements: package/namespace loggers; multiple debug output levels base: logging enhancements: multiple debug output levels Aug 30, 2023
@cooljeanius
Copy link

Relative to namespace-scoped logging - something not currently included - I'm wondering if we should shelve that at this point?

Yeah, probably worth saving that for a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raimue raimue raimue left review comments

@cjones051073 cjones051073 cjones051073 approved these changes

@jmroot jmroot Awaiting requested review from jmroot

Assignees
No one assigned
Labels
enhancement
Milestone
No milestone
4 participants
@mascguy @cjones051073 @cooljeanius @raimue