Uh oh!
There was an error while loading. Please reload this page.
Improve SuperPMI script robustness - #65586
Conversation
ghost
commented
Feb 18, 2022
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
|
BruceForstall
commented
Feb 18, 2022
@dotnet/jit-contrib Improve SPMI collection robustness |
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.
26bf29d to
a6fc7d1CompareBruceForstall
commented
Feb 18, 2022
@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) forcins) | |
| s="".join(safe_char(c) forcins)[-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.