Page MenuHomePhabricator

[lldb][CMake] Fix windows-x86-debug compilation with python enabled using multi-target generator
ClosedPublic

Authored by leonid.mashinskiy on Aug 30 2019, 2:43 AM.

Details

Summary

Visual Studio CMake generator is multi-target and does not define CMAKE_BUILD_TYPE, so Debug build on VS was failing due selection of release python library.
This patch reverts back some of latest changes and fixes building by raw VS using CMake expression generators.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

+windows folks, then can speak to the substance of this change.

As for the implementation, I get the impression that there is a lot of duplicated code in here. The code for finding the release and debug pythons is nearly identical. Can this be extracted into an utility function?

I'll look at this in detail soon, but a few caveats:

  • lldb tests don't work with the debug Python interpreter, which is why I've been complaining about test failures approximately forever when the bots seem fine.
  • Some of these changes you're undoing caused a lot of pain when they were first put in, so let's be really sure if we need to undo this.
  • I use the VS project only for code browsing and debugging. For building and tests, I use the ninja project. So I'm not very familiar with the problem you're trying to fix.

Is there a way to reduce some of the code duplication? The more duplication there is, the harder it is to maintain.

cmake/modules/LLDBConfig.cmake
267

Nit: multi-configuration

amccarth requested changes to this revision.Aug 30 2019, 3:03 PM
amccarth added inline comments.
cmake/modules/LLDBConfig.cmake
206–213

This first changed line seems useful.

The rest seems of dubious benefit, even if the duplication can be reduced. Are folks really using the VS project to build both Debug and Release? I'd be surprised, since the debug version won't pass half of the tests.

This revision now requires changes to proceed.Aug 30 2019, 3:03 PM

Since this patch appeared to be arguable from your point of view I will write some comments before making any further changes.

  • I use the VS project only for code browsing and debugging. For building and tests, I use the ninja project. So I'm not very familiar with the problem you're trying to fix.

Since there are lot of tablegenned code just opening project in VS without building it makes lot's of code unresolved for producivity tools providing smart code navigation, highlighting and etc (like Intellisence or Resharper) which helps me understand code faster.
Also I find it useful to be able to compile single cpp file from VS.
And also building from VS was working on 9 branch so I just thought it was broken unintentionally and can be fixed.

  • Some of these changes you're undoing caused a lot of pain when they were first put in, so let's be really sure if we need to undo this.

I am not just undoing latest changes, that's why there are so many copy-pasted code. If CMAKE_BUILD_TYPE is set (which is set when building using ninja or other single-target generator) then logic should flow the same way as without my changes. I'm just supporting third way when CMAKE_BUILD_TYPE is not selected and configuration is deduced on built time and paths to selected libraries are selected using CMake expression generators as it was in 9 branch.

  • lldb tests don't work with the debug Python interpreter, which is why I've been complaining about test failures approximately forever when the bots seem fine.

I found it easier to debug some tests in full debug environment, though I didn't try to run all test suite in debug.
Also we found some tests to be flaky even in release configurations but bot had never showed that.

If you are ok with my explanations I will try to extract some code to reduce copy-pasting, otherwise this patch can be abandoned.

Also I have a question about x86 windows configuration - is it officially supported or it will be abandoned soon? Because now it is completely broken.

I'm open to this if we can reduce the code duplication.

One of my concerns is that changes in July and August completely broke the build for me for many days. I had to remove all but one version of Python to ensure that it always found the right one.

I, too, would appreciate being able to run tests under the debugger, but I find that's impossible with python_d because it detects and halts virtually every test (like using the Python allocator without holding the GIL and leaked resource warnings).

Extracted python lookup logic into function

labath added a comment.Sep 5 2019, 3:55 AM

Thanks. The new version seems fine to me. I'll leave it for the windows folks to have the final say on this..

amccarth accepted this revision.Sep 5 2019, 8:10 AM

Thanks for factoring out the duplication.

Fingers crossed.

This revision is now accepted and ready to land.Sep 5 2019, 8:10 AM

Thanks for the review!

Can somebody commit this please because I don't have commit access?

Yes, I can commit it for you soon.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2019, 10:23 AM