This is an archive of the discontinued LLVM Phabricator instance.

[lit] Use inodes instead of realpaths in the config map
AbandonedPublic

Authored by zturner on Sep 18 2017, 5:41 PM.

Details

Summary

This should better address symlinks and other weird situations instead of relying on path string comparisons.

Diff Detail

Event Timeline

zturner created this revision.Sep 18 2017, 5:41 PM
zturner updated this revision to Diff 115768.Sep 18 2017, 5:48 PM

Added missing file.

dlj edited edge metadata.Sep 18 2017, 8:03 PM

I think the previous logic should have yielded a fairly stable identifier... do you have an example off-hand of a case that fails?

(i.e., I would have expected the call to os.path.realpath() to have done the moral equivalent of looking up a canonical path based on the inode, which normpath() alone won't do.)

llvm/utils/lit/lit/discovery.py
51

.st_ino?

In D38015#874692, @dlj wrote:

I think the previous logic should have yielded a fairly stable identifier... do you have an example off-hand of a case that fails?

(i.e., I would have expected the call to os.path.realpath() to have done the moral equivalent of looking up a canonical path based on the inode, which normpath() alone won't do.)

I don't have an example of where the former normalization breaks, but it just seemed like this is exactly what inodes were designed for. Plus if realpath is going to look up the inode anyway, might as well use it instead of then converting back into some string. So I think the code will work either way, I just felt better about having a map of integers instead of a map of strings since there's fewer things that could go wrong.

zturner abandoned this revision.Sep 20 2017, 12:47 PM

So apparently on Windows st_ino is 0 until Python 3.4. So, that's a deal breaker.