rnp icon indicating copy to clipboard operation
rnp copied to clipboard

[#1498] Fix tmpdir cleanup

Open andrey-utkin opened this issue 3 years ago • 4 comments

A lot of temporary directories are always left around after the tests even when all they all pass. There are a few sources of that, and this patchset addresses them all.

Bug: https://github.com/rnpgp/rnp/issues/1498

andrey-utkin avatar Jun 27 '22 13:06 andrey-utkin

Codecov Report

Base: 81.76% // Head: 81.77% // Increases project coverage by +0.01% :tada:

Coverage data is based on head (b0fec63) compared to base (fbfef64). Patch coverage: 95.65% of modified lines in pull request are covered.

:exclamation: Current head b0fec63 differs from pull request most recent head d5836ac. Consider uploading reports for the commit d5836ac to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1866      +/-   ##
==========================================
+ Coverage   81.76%   81.77%   +0.01%     
==========================================
  Files         140      140              
  Lines       28974    28997      +23     
==========================================
+ Hits        23690    23712      +22     
- Misses       5284     5285       +1     
Impacted Files Coverage Δ
src/tests/support.h 100.00% <ø> (ø)
src/tests/support.cpp 82.23% <93.33%> (+0.34%) :arrow_up:
src/tests/ffi.cpp 86.30% <100.00%> (+0.01%) :arrow_up:
src/tests/issues/1030.cpp 90.47% <100.00%> (+0.47%) :arrow_up:
src/tests/rnp_tests.cpp 94.11% <100.00%> (+2.45%) :arrow_up:

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

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Jun 27 '22 13:06 codecov[bot]

@andrey-utkin Could you please add check whether run is made under the GHA, so only local runs would be cleaned up automatically? (this should be helpful: https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables )

Basically, current centos8 failure is something because of what we should not cleanup CI automatically, and upload artifacts once #1430 is implemented.

ni4 avatar Jun 28 '22 13:06 ni4

For now I've added a way to preserve the tempdirs by setting an env var KEEPTEMP.

I'd rather make GHA workflows declare explicitly that they want this, by setting that env var, because this simple concept will likely outlive the distinction between GHA and local runs.

We can add this setting in .github/workflows/*.yml right now, basically reverting to the previous behaviour in GHA. But I imagine if we start writing actual artifact retention logic, we'd handle it from the context of a failed assertion, which should have the knowledge of what the workdir is, and before the workdir is removed, so the setting is not necessary.

andrey-utkin avatar Jun 28 '22 17:06 andrey-utkin

For now I've added a way to preserve the tempdirs by setting an env var KEEPTEMP.

I'd rather make GHA workflows declare explicitly that they want this, by setting that env var, because this simple concept will likely outlive the distinction between GHA and local runs.

I doubt that it would be the heaviest problem once migrating away from GHA :-) But, okay, let's use this approach, but it would be better to rename variable to something starting with RNP_, like. RNP_KEEP_TEMP or so on.

ni4 avatar Jul 04 '22 08:07 ni4

I understand this PR has been approved, however, I can't help but wonder why the windows-2019 msvc tests are consistently timing out:

  • https://github.com/rnpgp/rnp/actions/runs/3513330605/jobs/5907300545
  • https://github.com/rnpgp/rnp/actions/runs/3513330605/jobs/5907300677

I've tried retrying them but same issue with timeout. Prior to this PR, they complete pretty fast:

  • https://github.com/rnpgp/rnp/actions/runs/3514838439

@ni4 @antonsviridenko is this timeout an issue that has already been fixed in another PR and therefore safe to merge this?

ronaldtse avatar Nov 22 '22 14:11 ronaldtse

@ronaldtse Thanks for bringing this up, somehow missed this. Should be related to test file leftovers...

ni4 avatar Nov 22 '22 15:11 ni4

yeah, probably some issue introduced by this PR...

antonsviridenko avatar Nov 22 '22 15:11 antonsviridenko

Seems to hang on SHFileOperationA call, continuing with debugging...

ni4 avatar Nov 22 '22 18:11 ni4

@ronaldtse Now it should be good to go. Issue should be in some hanging GUI, displayed during the SHFileOperationA(), maybe some readonly file or similar.

ni4 avatar Nov 25 '22 16:11 ni4

Thank you @ni4 ! This is perfect, merging.

ronaldtse avatar Nov 25 '22 18:11 ronaldtse