Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

log: Avoid side effects by using own logger instance - fixes #8907#8908

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
srebhan wants to merge1 commit intorclone:master
base:master
Choose a base branch
Loading
fromsrebhan:fix_issue_8907

Conversation

@srebhan
Copy link

What is the purpose of this change?

Commitdfa4d94 replaced the old logging with the more modernslog package. However, this change is intrusive for applications usingslog as their default logging facility, causing side effects and a fragile construct when usingslog logging with custom output functions.

This PR sets up a custom logger, avoiding side effects from overriding theslog default logger.

Was the change discussed in an issue or in the forum before?

resolves#8907

Checklist

  • I have read thecontribution guidelines.
  • I have added tests for all changes in this PR if appropriate.
  • I have added documentation for the changes if appropriate.
  • All commit messages are inhouse style.
  • I'm done, this Pull Request is ready for review :-)

Copy link
Member

@ncwncw left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this. Using rclone as a library is probably the least well supported way of using rclone but we need to play nice if people do :-)

I put some queries inline.

h:=NewOutputHandler(os.Stderr,opts,logFormatDate|logFormatTime)

// Set the slog default handler
slog.SetDefault(slog.New(h))
Copy link
Member

Choose a reason for hiding this comment

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

If we don't call this then rclone or any of its dependencies won't get properly logged if they are usinglog.Print orlog.Fatal.

From the docs

Setting a logger as the default with

slog.SetDefault(logger)

will cause the top-level functions like Info to use it. SetDefault also updates
the default logger used by the log package, so that existing applications that
use log.Printf and related functions will send log records to the logger's
handler without needing to be rewritten.

Copy link
Author

Choose a reason for hiding this comment

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

I do understand, however, we are using rclone as a library and overriding the default slog logger will mess with all our logging and causes the issues mentioned in#8907.
How about we are setting this handler the default from the rclone application? I can add a function tofs...

Just for protocol, using adefault logging facility in a library is bad style and should be avoided. Add a logger interface and allow overriding the logger in the library and then set that logger from the application is a much better pattern as it allows the application to choose how to log stuff.

slog.SetDefault(slog.New(h))

// Make log.Printf logs at level Notice
slog.SetLogLoggerLevel(fs.SlogLevelNotice)
Copy link
Member

Choose a reason for hiding this comment

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

I don't see an equivalent for this line in your patch? Unless the Default logger is Notice level which I don't see in the docs?

Copy link
Author

@srebhansrebhanOct 24, 2025
edited
Loading

Choose a reason for hiding this comment

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

Well this line overrides the log-bridge of slog! This should not be done within the library but rather in the application so others can use the library without their logging being changed...

@ncw
Copy link
Member

ncw commentedOct 24, 2025

Any thoughts on the review notes@srebhan ? I think this is an important fix but I want to make sure we get it right.

@srebhan
Copy link
Author

I responded to your comments@ncw. I see your point of this PR changing behavior but the current approach is somewhat limited due to the structure. I could put up a PR reworking logging such that the logger can be customized from the application and is not defined in the library itself. What do you think?

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

Reviewers

@ncwncwncw left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Log package interferes with application logging

2 participants

@srebhan@ncw

[8]ページ先頭

©2009-2025 Movatter.jp