Re: [Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

Thank you! It seems that your commit have fixed the bug - it haven't been reproduced on `lldb-x64-windows-ninja` since your commit. On Thu, Dec 6, 2018 at 9:45 PM Zachary Turner wrote: > Hopefully fixed in r348511. I went with the approach of concatenating the > executable file name, for two re

Re: [Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

Hopefully fixed in r348511. I went with the approach of concatenating the executable file name, for two reasons. First, it's easier, since dealing with the tmpfile module and mkstemp and sorts has its own set of complexities and issues. Secondly, because it means the build artifacts will still b

Re: [Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

I am in favor of unique filenames because it will guarantee we avoid any problems like this in the future, but in most cases, as long as the names are unique *in the test suite*, that should be sufficient. So I don’t have any objections to Zachary’s approach of making the filenames unique enough

Re: [Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

Yea that sounds reasonable On Thu, Dec 6, 2018 at 1:07 AM Pavel Labath wrote: > On 06/12/2018 09:25, Aleksandr Urakov via lldb-commits wrote: > > > > I thought about what Stella have said, and it seems that I have found a > > good solution for this. We can use something like `tempfile` > > (https

Re: [Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

On 06/12/2018 09:25, Aleksandr Urakov via lldb-commits wrote: I thought about what Stella have said, and it seems that I have found a good solution for this. We can use something like `tempfile` (https://docs.python.org/3.7/library/tempfile.html) for every internally generated file of the scr

Re: [Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

Yes, this solution would solve the current problem. But theoretically the uniqueness may be defined by the directory, where the file is located, not by the name. It looks very unlikely, but who knows... I thought about what Stella have said, and it seems that I have found a good solution for this.

Re: [Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

I was thinking that we could just automatically compute the output file names as: os.path.join(out_dir, basename(output_file) + '.' + basename(input_file) + '.obj') Currently it's just os.path.join(out_dir, basename(input_file) + '.obj') which is why I think the problem occurs. On Wed, Dec 5,

Re: [Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

I think with all the tests running in parallel we should err on the side of having unique names for everything automatically generated. From: Aleksandr Urakov Sent: Wednesday, December 5, 2018 12:48 PM To: Zachary Turner Cc: reviews+d54942+public+2603ca548f36d...@reviews.llvm.org; Stella Stamen

Re: [Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

With such solution there would be even no need to change the current commit. But I'm not sure that it's trivial to do - the output file name may contain path with directories. May be we can replace slashes with underscores in the output file path and concatenate it with the object file name? Or eve

Re: [Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

It is not possible to specify object file name in compile and link mode. But perhaps we can just change the default object file name to include something from the output file as well On Wed, Dec 5, 2018 at 12:26 PM Aleksandr Urakov via Phabricator < revi...@reviews.llvm.org> wrote: > aleksandr.ura

[Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

aleksandr.urakov added a subscriber: zturner. aleksandr.urakov added a comment. The similar problem with `typedefs.test` is here: http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/1940/steps/test/logs/stdio I have an assumption about the cause of the problem. Are the tests running

[Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

aleksandr.urakov added a comment. It looks strange. The compiler can't open the object file. And in the next build it is ok - may be it was some server failure (e.g. full disk)? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54942/new/ https://reviews.llvm.org/D54

[Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

stella.stamenova added a comment. enums-layout.test is now failing on Windows: http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/1938/steps/test/logs/stdio Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54942/new/ https://reviews.llvm.org/D54942 _

Re: [Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

Ok, thanks! On Wed, Dec 5, 2018 at 6:06 PM Zachary Turner wrote: > Yes, if the tests pass for you with current builder, please commit > On Wed, Dec 5, 2018 at 6:06 AM Aleksandr Urakov via Phabricator < > revi...@reviews.llvm.org> wrote: > >> aleksandr.urakov added a comment. >> >> It seems that

[Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL348386: [PDB] Make PDB lit tests use the new builder (authored by aleksandr.urakov, committed by ). Herald added a subscri

Re: [Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

Yes, if the tests pass for you with current builder, please commit On Wed, Dec 5, 2018 at 6:06 AM Aleksandr Urakov via Phabricator < revi...@reviews.llvm.org> wrote: > aleksandr.urakov added a comment. > > It seems that all the reviews this one depends on are already in. Can we > proceed with it?

[Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

aleksandr.urakov added a comment. It seems that all the reviews this one depends on are already in. Can we proceed with it? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54942/new/ https://reviews.llvm.org/D54942 ___ lldb-commits mailing l

[Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

aleksandr.urakov updated this revision to Diff 176611. aleksandr.urakov added a comment. Update tests due to D55230 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54942/new/ https://reviews.llvm.org/D54942 Files: lit/SymbolFile/PDB/ast-restore.test

[Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

aleksandr.urakov added a comment. Thanks all! In D54942#1310224 , @zturner wrote: > Makes sense. Just curious, is the order file strictly necessary for this > test? `/Gy` is the same as `-ffunction-sections`, so there could be an > argument to be made

[Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

zturner added a comment. > Clang called from the builder shows errors on types like char16_t and > char32_t. That's why enums-layout.test and typedefs.test are made to use MSVC > instead; For me enums-layout.test crashes, but I see the same behavior in typedefs.test. This is because lld-link

[Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

stella.stamenova added a comment. This looks fine, but let's see if Zachary's change lands first and how it looks like after it lands :). Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54942/new/ https://reviews.llvm.org/D54942 __

[Lldb-commits] [PATCH] D54942: [PDB] Make PDB lit tests use the new builder

aleksandr.urakov created this revision. aleksandr.urakov added reviewers: zturner, stella.stamenova. aleksandr.urakov added a project: LLDB. Herald added subscribers: lldb-commits, teemperor, abidh. This patch makes old PDB plugin tests to use the new builder (see D54914