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

Update txt logger mode to 'a' #584

Merged
merged 4 commits into from
Jun 9, 2023

Conversation

antklein
Copy link
Contributor

@antklein antklein commented Jun 6, 2023

Python logging to a file with mode='w' can cause log loss when logging happens after the logger has already been cleaned up.
See issue: python/cpython#86544

The fix for Python was to have special cases for files opened in mode='w'. Currently we use mode='w+', so the checked in fix does not solve our problem.

I've implemented moving the file_logger to mode='a' and an explicit deletion of the logging file before opening the FileHandler.

@antklein
Copy link
Contributor Author

antklein commented Jun 7, 2023

@makubacki looks like the Ubuntu Python 3.10 and 3.11 builds are complaining because of the below error. Who could help look into that?

Test Stuart PR for Policy 5 library inf changed                       | FAIL |
'' does not contain 'MdeModulePkg'

@Javagedes
Copy link
Contributor

Javagedes commented Jun 7, 2023

@makubacki looks like the Ubuntu Python 3.10 and 3.11 builds are complaining because of the below error. Who could help look into that?

Test Stuart PR for Policy 5 library inf changed                       | FAIL |
'' does not contain 'MdeModulePkg'

@antklein Do not worry about this error. I added a new policy check that still need to be integrated into EDK2. This will continue to fail while I work through the mailing list and EDK2 PR process.

@Javagedes
Copy link
Contributor

@antklein This change looks good, but could you please specify the situation where this is an issue? The only way I see this being an issue is if you setup the same txt logger twice, which does not happen in an invocable.

Thanks!
Joey

@antklein
Copy link
Contributor Author

antklein commented Jun 7, 2023

@antklein This change looks good, but could you please specify the situation where this is an issue? The only way I see this being an issue is if you setup the same txt logger twice, which does not happen in an invocable.

Thanks! Joey

Hi Joey,
For us this exact issue was happening. python/cpython#86544 (comment)

That's because in our platform level PlatformPostBuild function we spin off sub-threads which make use of the RunPythonScript from edk2toollib.utility_functions. If the main thread closes, closing the logging FileHandle, the sub-threads then return causing the RunPtyhonScript to log additional information clearing the original BUILDLOG output.

@Javagedes
Copy link
Contributor

@antklein This change looks good, but could you please specify the situation where this is an issue? The only way I see this being an issue is if you setup the same txt logger twice, which does not happen in an invocable.
Thanks! Joey

Hi Joey, For us this exact issue was happening. python/cpython#86544 (comment)

That's because in our platform level PlatformPostBuild function we spin off sub-threads which make use of the RunPythonScript from edk2toollib.utility_functions. If the main thread closes, closing the logging FileHandle, the sub-threads then return causing the RunPtyhonScript to log additional information clearing the original BUILDLOG output.

Understood. That scenario makes sense. Thanks!

@Javagedes Javagedes added the bug Something isn't working label Jun 7, 2023
@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Merging #584 (3f4dce4) into master (e0625a4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #584   +/-   ##
=======================================
  Coverage   78.99%   78.99%           
=======================================
  Files          47       47           
  Lines        4774     4776    +2     
=======================================
+ Hits         3771     3773    +2     
  Misses       1003     1003           
Impacted Files Coverage Δ
edk2toolext/edk2_logging.py 88.20% <100.00%> (+0.13%) ⬆️

@Javagedes Javagedes merged commit 813ed73 into tianocore:master Jun 9, 2023
@Javagedes Javagedes added this to the v0.23.6 milestone Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants