satpy icon indicating copy to clipboard operation
satpy copied to clipboard

Fix bz2 unzip function errors

Open simonrp84 opened this issue 2 years ago • 3 comments

The bz2 unzip function in readers/utils.py doesn't close the BZ2 file handler, which on some platforms leads to permission errors when os.remove tries to delete the unclosed files. This PR wraps the BZ2 handler in a with closing() statement to prevent this error.

simonrp84 avatar Aug 25 '22 09:08 simonrp84

Maybe related to this https://github.com/pytroll/satpy/issues/2183 ?

mraspaud avatar Aug 25 '22 09:08 mraspaud

Codecov Report

Merging #2193 (4f01ae7) into main (0c2b965) will decrease coverage by 0.00%. The diff coverage is 85.96%.

@@            Coverage Diff             @@
##             main    #2193      +/-   ##
==========================================
- Coverage   94.19%   94.19%   -0.01%     
==========================================
  Files         295      295              
  Lines       45393    45396       +3     
==========================================
+ Hits        42760    42761       +1     
- Misses       2633     2635       +2     
Flag Coverage Δ
behaviourtests 4.66% <0.00%> (-0.01%) :arrow_down:
unittests 94.84% <85.96%> (-0.01%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
satpy/readers/utils.py 91.95% <45.45%> (ø)
satpy/tests/reader_tests/test_utils.py 99.25% <95.65%> (-0.75%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Aug 25 '22 09:08 codecov[bot]

Coverage Status

Coverage decreased (-0.004%) to 94.792% when pulling 4f01ae7cfdf8f3c5b1d3ecda5df8a00a1f1e8dee on simonrp84:fix_bz2_unzip into 0c2b9650e48867fbfb6292102400e3d5007bf74a on pytroll:main.

coveralls avatar Aug 25 '22 10:08 coveralls

Looks good, do you think the block above could also drop the closing or would the raising in the except block be a problem?

mraspaud avatar Oct 03 '22 16:10 mraspaud

@mraspaud I think it should be OK to remove that closing but that's not my code originally so I'm not 100% sure what the original purpose was. I'll remove it as the tests pass OK for this.

simonrp84 avatar Oct 04 '22 08:10 simonrp84

I think there has been quite some work merged for this already, so I'm closing this PR for now. Feel free to reopen if it turns out that problem isn't solved.

mraspaud avatar Feb 09 '23 09:02 mraspaud