-
Notifications
You must be signed in to change notification settings - Fork 37
[ATfL] Make sure internal shell is not used by lit #602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ATfL] Make sure internal shell is not used by lit #602
Conversation
This is to mitigate the effects of this commit: llvm/llvm-project@ee0f86d
|
The error fixed by this commit: |
|
It would be better if someone could fix the test. I don't think anyone else so far has relied on the Support for the external shell is going to be removed eventually and there is a very real risk that new tests do not work with the external shell and there will be very little upstream coverage of that configuration. |
What worries me more is how low is the coverage of the 'using just-built libc++ for building LLVM' scenario. |
DavidTruby
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I mainly use the internal shell for lit, but I still regularly see errors I just know to ignore. I don't think it's ready for a product. I do strongly think we should document this somewhere and reconsider in future. I'm also sure issue reports for breaking tests upstream would be much appreciated!
I've also been concerned about this for a long time. There's even supposedly "built-in" bootstrapping builds you can turn on that I've never been able to get to work. Is there any hope we could ask eg. Linaro to look at a build bot for these? |
|
I'm trying to figure out what issue are we facing here. I've triggered a manual build, and, as expected, it fails on the And, having all those files still there, I could confirm that it doesn't happen in the command line (did the positive test case first, that clang actually needs Now the questions are:
|
|
I think llvm/llvm-project#165140 might fix this.
Not really bash like. It runs the binaries directly inside Python using
Yes. It's a builtin function. It by doesn't do anything special to |
I've tried this locally, and it seems it does really help. Thing is, the way this PR has been opened does not make it easy to just cherry-pick it, I had to rework this patch manually in order to try it out. |
|
Closing it as the upstream fix has proven to work. |
This is to mitigate the effects of this commit:
llvm/llvm-project@ee0f86d