Skip to content
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

If CMake supports it, sets the debugger working directory for Visual … #1435

Merged
8 commits merged into from
Sep 11, 2017

Conversation

AnyOldName3
Copy link
Member

…Studio

This should stop a bunch of threads appearing on the forum where a user has attempted to launch Visual Studio's debugger right after using the Windows build script and then suddenly found that it doesn't work properly.

@ghost
Copy link

ghost commented Sep 8, 2017

The duplication hurts me :| Maybe we can add a macro 'openmw_add_executable' around 'add_executable' that also sets the target properties we want?

@AnyOldName3
Copy link
Member Author

It's in an openmw_add_executable macro now. Any better?

@AnyOldName3
Copy link
Member Author

Actually, that broke something. I don't really know what it broke or how (CMake still succeeds, but stuff isn't built properly). I don't see anything particularly wrong with anything when I investigate it.

macro (openmw_add_executable target)
add_executable(${target} ${ARGN})
if (MSVC)
if (CMAKE_VERSION VERSION_GREATER 3.8)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this only be true for CMake versions 3.9 and up? VS_DEBUGGER_WORKING_DIRECTORY was added in version 3.8.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Apparently VERSION_GREATER_EQUAL hasn't been around for that long, either, so I'll have to use an OR.

@PlutonicOverkill
Copy link
Contributor

PlutonicOverkill commented Sep 9, 2017

This may or may not be related, but the two projects that fail to compile, the wizard and the launcher, both pass ${GUI_TYPE} as an argument, which is either set to WIN32 or unset. Before this macro was added, the arguments passed to add_executable started with
openmw-wizard WIN32 componentselectionpage.cpp
and afterwards with
openmw-wizard WIN32;componentselectionpage.cpp
So it seems like WIN32 is not being passed properly.

EDIT: Additionally this generates CMake warnings CMP0003 and CMP0020 which did not occur before. So this is definitely caused by incorrect linker options.

@AnyOldName3
Copy link
Member Author

I've done some more experimentation. If I surround ${ARGN} with quotes, I can get it to behave a bit differently. With that change, it puts the necessary separators in but interprets optional arguments to add_executable which are embedded in ARGN as more source files.

I reckon this means that with correct placement of quotes in every call to openmw_add_executable I can make this work, but then it requires calling in a slightly different way to the original function it wraps.

@AnyOldName3
Copy link
Member Author

I've done some experimenting. It still doesn't work, but it should be easier to see why it might not be working

@AnyOldName3
Copy link
Member Author

I'm printing a bunch of stuff with message and it seems that add_executable's options, WIN32, MASOSX_BUNDLE and EXCLUDE_FROM_ALL, are in fact being set correctly, so the argument passing for those has been resolved.

Unfortunately, we still can't find our WINMAIN function. I've looked into this, and usually, this is because WIN32 has been set for a console application (which isn't the case, as the Wizard and Launcher are both GUI-only) or, alternatively, because something's gone screwy with Qt, as that provides the WINMAIN function when it's used.

@AnyOldName3
Copy link
Member Author

If I make it so WIN32 is never set, everything compiles again, and then runs 'perfectly' except a console window is created instead of just the GUI. This is consistent with what I'd expect if the necessary Qt libraries weren't fed to the linker. I don't think I'm even touching the linker input, though.

@AnyOldName3
Copy link
Member Author

Finally I have something I can work with. If I use the master version of the CMake files, the Wizard and Launcher projects depend on qtmain.lib, but when using my modified version, this isn't set up. This also happens to be where Qt keeps its WinMain function definition.

There's still the open question of why this transitive dependency isn't added by CMake after my changes. Theoretically, it still should be, as the transitive dependency on Qt5Gui.lib is added successfully. There's a slim chance that it's because we use the deprecated qt5_use_modules macro, but I'm not exactly sure what we're supposed to use instead, and if switching to that will break compatibility with older CMake versions.

@AnyOldName3
Copy link
Member Author

I'm under the impression that anything to do with Qt is a symptom rather than a cause. I still think that there's something broken with how I'm wrapping add_executable

@AnyOldName3
Copy link
Member Author

I think I fixed it. It seems policies CMP0003 and CMP0020 were being unset upon entering the macro, so I turned them back on. I'm not sure why this is - I didn't think macros created a new scope.

@psi29a
Copy link
Member

psi29a commented Sep 10, 2017

Possibly a bug? Have you tried contacting the cmake people for their advice?

@kcat
Copy link
Contributor

kcat commented Sep 10, 2017

It seems policies CMP0003 and CMP0020 were being unset upon entering the macro, so I turned them back on. I'm not sure why this is

Were they even being set in the first place? git grep -i cmake_policy turns up nothing.

@AnyOldName3
Copy link
Member Author

I looked it up, and apparently, macros and functions have the same policies set as when they were defined, not when they were called. At some point we probably have a cmake_minimum_required which sets a bunch of policies, but not at the point at which we import the macros.

Before doing this, I did print out their values both inside and outside my new macro, and they were definitely set outside of it. It still isn't the cleanest thing imaginable.

In order to make this cleaner, the only real solutions are to:

  • Change the point at which OpenMW macros are imported (such that the right policies are set). This might break a bunch of the other macros if they aren't imported before they're needed.

  • Make the macro accept a list of policies as an argument. This breaks the concept of having the macro be a drop-in replacement for add_executable.

As hacks go, the current one seems to be the neatest possible.

@ghost
Copy link

ghost commented Sep 10, 2017

I think it'd be reasonable to set the cmake_minimum_version (and hence policy) at the start of cmake file, before importing macros.

@AnyOldName3
Copy link
Member Author

I'll take a look at doing that. Right now, it's set quite a way down the top-level CMake file, but I think I can rearrange it without breaking anything.

@ghost ghost merged commit bd667c3 into OpenMW:master Sep 11, 2017
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants