Page MenuHomePhabricator

Eliminate differences in lldbinline-generated Makefiles and ensure they're regenerated every time
ClosedPublic

Authored by spyffe on Jun 6 2016, 10:50 AM.

Details

Reviewers
tfiala
Summary

To eliminate problems where 'lldbinline.py'-generated Makefiles are re-used, I've standardized the generation of them. The testsuite now asserts if there is a discrepancy between what is there and what 'lldbinline.py' would generate.

I've also eliminated all cases where what is there is legitimately different from what 'lldbinline.py' would generate.

Diff Detail

Repository
rL LLVM

Event Timeline

spyffe updated this revision to Diff 59750.Jun 6 2016, 10:50 AM
spyffe retitled this revision from to Eliminate differences in lldbinline-generated Makefiles and ensure they're regenerated every time.
spyffe updated this object.
spyffe added a reviewer: tfiala.
spyffe set the repository for this revision to rL LLVM.
spyffe added a subscriber: lldb-commits.
tfiala accepted this revision.Jun 6 2016, 11:27 AM
tfiala edited edge metadata.

Looks good, Sean!

This revision is now accepted and ready to land.Jun 6 2016, 11:27 AM

Hi, I have reverted this commit, as it makes a number of assumptions, which are not true on windows. Please see comments for details.

If you need help testing out a revised version on windows, let me know. I think Zachary will be able to help with that as well (:P).

packages/Python/lldbsuite/test/lldbinline.py
135

This files will not end up being identical, due to different path separators and newlines when this is being run on windows.

I like the idea of diffing, but it needs to be done in a way that it will work on windows.

136

This will not cause the error to be reported in the test runner. See http://lab.llvm.org:8011/builders/lldb-windows7-android/builds/6268/steps/test1/logs/stdio where, this code is triggered, but the test is still not marked as failed in the summary at the end. I think throwing an exception here would do the expected thing.

138

Windows does not support in-place renames, so this will just fail.

For the diffing issue seems like we should be able to generate makefiles
with stable separators. I haven't seen the generation code, but it seems
like we could write a function lldbsuite.support.fs.unixpath() that works
like normpath but always uses /, never \. Would that fix it?

When you say it doesn't support in place renames, the only issue I'm aware
of is that you can't use os.rename if the destination already exists. As
such, you also can't implement atomic rename. As long as that isn't
required, you should be able to unlink first, then rename. I thought we had
a function in lldbsuite.support.fs that does this, but I can't remember

Did this one go in? If so, can we close the review?

Thanks!

Appear to be reverted in r272062.

tfiala closed this revision.Sep 23 2016, 9:37 AM

Closing this out as it appears to be resolved one way or another.