Bug 160585 – Some EWS console logs doesn't go to log file

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

After the fix of https://bugs.webkit.org/show_bug.cgi?id=159539, some of the logs started going to log files. However all the console logs doesn't go to the log file. Only logs from the log statements in webkitpy/tool/commands/queues.py are going to log file. This is because the logger from only that module is configured to log to file. We should ensure that all the console logs go to logfile.

[Aakash Jain]

Created attachment 285393 [details]
Proposed patch

[Daniel Bates]

Comment on attachment 285393 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285393&action=review

Please add tests to Tools/Scripts/webkitpy/common/system/filesystem_unittest.py for this change.

> Tools/Scripts/webkitpy/common/system/filesystem.py:238
> +    def open_text_file_for_writing(self, path, mode='w'):

Expose the mode argument of codes.open() is error prone because it allows a person to violate the purpose of this function - to open a file for writing. For example, suppose a person makes the illogical call open_text_file_for_writing("a.txt", "r") then the file a.txt will be opened in read-only mode. In general when designing an interface we want to make it easy to use correctly and hard to use incorrectly. Clearly, the proposed change makes it easy to use this function incorrectly. I suggest that we add new boolean parameter to this function, called shouldAppend or append, that determines whether we open the specified file in write-only mode or in append-mode. This parameter should default to False - open the file in write-only mode.

[Aakash Jain]

Created attachment 285469 [details]
Updated patch

[Daniel Bates]

Comment on attachment 285469 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285469&action=review

> Tools/Scripts/webkitpy/tool/commands/queues.py:126
> +        logutils.configure_logger_to_log_to_file(logging.getLogger("webkitpy"), self.queue_log_path(), self.host.filesystem)

I suggest that we add a comment in this function to explain that we explicitly pass 'logging.getLogger("webkitpy")' instead of _log because we want to capture all messages logged from webkitpy modules.

[Aakash Jain]

Created attachment 285598 [details]
Added the comment, also moved logging initialization to queueengine.py from queues.py

[Daniel Bates]

Comment on attachment 285598 [details]
Added the comment, also moved logging initialization to queueengine.py from queues.py

View in context: https://bugs.webkit.org/attachment.cgi?id=285598&action=review

> Tools/ChangeLog:11
> +        (FileSystemHandler._open): Ensure that we open logfile in append mode in order to avoid

Nit: logfile => log file

> Tools/ChangeLog:21
> +        (QueueEngine._begin_logging): Configure the python logger to log to file.

Nit: python => Python

> Tools/Scripts/webkitpy/common/system/logutils.py:226
>      logger.addHandler(handler)
> +    return handler

Instead of modifying this function so as to make its behavior even more similar to logutils.configure_logging() I suggest we take this opportunity to remove this function entirely and have the caller call logutils.configure_logging() directly.

> Tools/Scripts/webkitpy/common/system/logutils.py:230
> +def remove_handler_from_logger(logger, handler):
> +    logger.removeHandler(handler)

I do not see the need for this convenience function. The caller should just call logger.removeHandler(handler) directly.

[Aakash Jain]

This was landed manually in https://trac.webkit.org/changeset/204275. 
Discussed with Dan and split it into two patches. The other portion is in https://bugs.webkit.org/show_bug.cgi?id=160698

[Brent Fulgham]

Comment on attachment 285598 [details]
Added the comment, also moved logging initialization to queueengine.py from queues.py

Clearing flags since this patch has been landed.