Improve SuperPMI script robustness #65586
Conversation
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsIf we fail to remove a temporary directory, don't crash. Log the failure We have seen this failure in at least one Linux x64 PMI coreclr_tests
|
|
@dotnet/jit-contrib Improve SPMI collection robustness |
src/coreclr/scripts/jitutil.py
Outdated
| except Exception as ex: | ||
| logging.warning("Warning: failed to remove directory \"%s\": %s", self.mydir, ex) | ||
| # Print out all the remaining files and directories. | ||
| for dirpath, dirnames, filenames in os.walk(self.mydir): |
There was a problem hiding this comment.
Is there any point in printing this? What if os.walk fails for that directory as well?
There was a problem hiding this comment.
Well, I wanted to print something in case that provides clues to why the rmtree is failing in the first place; being completely silent to the failure seems wrong. It seemed like walk would be "safer", but I guess I could be paranoid and put try/except around that, too, if you think that would help.
There was a problem hiding this comment.
Yes, that will be good because I have seen things fail in APIs around files/folders. We can't do much about it except to catch those exceptions and move forward.
1. Be more robust to temp directory removal failure If we fail to remove a temporary directory, don't crash. Log the failure and the set of directories and files still remaining, and continue. We have seen this failure in at least one Linux x64 PMI coreclr_tests SuperPMI collection: ``` [Errno 39] Directory not empty: '/datadisks/disk1/work/B18E0979/t/tmpov3b4_qa' ``` 2. Limit the length of file names created by `make_safe_filename`. We were creating file names out of full path names, where those paths contained long temporary directory paths, and thus we were exceeding the maximum allowed file name component.
BruceForstall
force-pushed
the
AddDirectoryRemovalRobustness
branch
from
26bf29d to
a6fc7d1
Compare
BruceForstall
changed the title
|
@kunalspathak I added the extra try/except, and also added code to handle "too long" file names that was causing a failure on OSX. |
| # Typically, a max filename length is 256, but let's limit it far below that, because callers typically add additional | ||
| # strings to this. | ||
| max_allowed_file_name_length = 150 | ||
| s = "".join(safe_char(c) for c in s) |
There was a problem hiding this comment.
| s = "".join(safe_char(c) for c in s) | |
| s = "".join(safe_char(c) for c in s)[-max_allowed_file_name_length:] |
There was a problem hiding this comment.
I'm going to leave this as-is so people (like me) not too knowledgeable about Python precedence rules can understand it more easily...
If we fail to remove a temporary directory, don't crash. Log the failure
and the set of directories and files still remaining, and continue.
We have seen this failure in at least one Linux x64 PMI coreclr_tests
SuperPMI collection:
make_safe_filename. We were creatingfile names out of full path names, where those paths contained long temporary directory
paths, and thus we were exceeding the maximum allowed file name component.