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

Fixed build error: AddressInputWidget.h is never moc'ed but the moc file is included during build #165

Merged
merged 2 commits into from
Jul 5, 2024

Conversation

jahorta
Copy link
Contributor

@jahorta jahorta commented Jul 4, 2024

I encountered a build error from a fresh clone of the repository. AddressInputWidget.h is never moc'ed but the compiler is instructed to load moc_AddressInputWidget.h. When I moc AddressInputWidget.h it gives a response that nothing was moc'ed from that file, so I commented out the include for moc_AddressInputWidget.h

…o load moc_AddressInputWidget.h. Include for moc file is now commented out.
@cristian64
Copy link
Collaborator

This was me blindly adding the file to the MSVC project file without noticing that the class does not specify the Q_OBJECT macro; it doesn't use signals or other meta-object features, so the macro is not technically needed.

I think the potential solutions are:

  1. Remove the line from the project file (not only comment it out, but remove it).
  2. Introduce the Q_OBJECT macro in the class, even if it's not technically needed right now.
  3. Definitely drop the legacy MSVC project files to encourage devs to use the CMake-based project file, which is the file in use in the CI from where releases are made.

There is some discussion in #89 on this topic.

I guess my question is, doesn't the CMake file work well with Visual Studio on Windows? Is there any limitation or issue?

@jahorta
Copy link
Contributor Author

jahorta commented Jul 4, 2024

I don't know a lot about CMake, so I might be doing something wrong, but when I try to use the CMakeLists.txt in the Source folder, I get an error:

1> [CMake] CMake Error at C:\Users\jahor\Source\Repos\jahorta\Dolphin-memory-engine\Source\CMakeLists.txt:51 (find_package):
1> [CMake]   By not providing "FindQt6Widgets.cmake" in CMAKE_MODULE_PATH this project
1> [CMake]   has asked CMake to find a package configuration file provided by
1> [CMake]   "Qt6Widgets", but CMake did not find one.
1> [CMake] 
1> [CMake]   Could not find a package configuration file provided by "Qt6Widgets" with
1> [CMake]   any of the following names:
1> [CMake] 
1> [CMake]     Qt6WidgetsConfig.cmake
1> [CMake]     qt6widgets-config.cmake
1> [CMake] 
1> [CMake]   Add the installation prefix of "Qt6Widgets" to CMAKE_PREFIX_PATH or set
1> [CMake]   "Qt6Widgets_DIR" to a directory containing one of the above files.  If
1> [CMake]   "Qt6Widgets" provides a separate development package or SDK, be sure it has
1> [CMake]   been installed.
1> [CMake] 
1> [CMake] 
1> [CMake] -- Configuring incomplete, errors occurred!

This is using Visual Studio 2022 (64-bit) Version 17.10.3.

@cristian64
Copy link
Collaborator

The path to the Qt library needs to be included in CMAKE_PREFIX_PATH. In the command line, it would be something like "-DCMAKE_PREFIX_PATH=..\Externals\Qt\Qt6.5.3\x64", but I can't help much with Visual Studio.

The Qt library for Windows is available in a Git submodule, which I guess you have checked out, or else you would have had problems with the MSVC project too.

@jahorta
Copy link
Contributor Author

jahorta commented Jul 4, 2024

Okay, I was able to add that prefix path in the CMakeLists.txt when building for WIN32. It builds normally with CMake now with Visual Studio on Windows. Should I add that commit here?

@cristian64
Copy link
Collaborator

I'm not sure about it. In principle, one should be to configure the project to use their own Qt library or version; simply hardcoding the path may not be the correct thing to do.

@dreamsyntax
Copy link
Collaborator

We are talking windows. We ship a specific submodule. It is reasonable to hard code to use the provided submodule.

Why does the dolphin project not drop vcproj?
I'm less inclined to drop it because they don't.

@cristian64
Copy link
Collaborator

We are talking windows. We ship a specific submodule. It is reasonable to hard code to use the provided submodule.

Normally we'd check first whether Qt can be found; if not, the path to the Externals is added for a second attempt.

Why does the dolphin project not drop vcproj? I'm less inclined to drop it because they don't.

Good question. With regards to the issue in question, we can simple remove the line for AddressInputWidget and discuss it at any other time.

@jahorta
Copy link
Contributor Author

jahorta commented Jul 4, 2024

Normally we'd check first whether Qt can be found; if not, the path to the Externals is added for a second attempt.

I figured out how to make it first check whether Qt is already installed and revert to Externals for a second attempt for windows only using an if else block and it builds fine on Visual Studio, so I could add that commit instead.

@dreamsyntax
Copy link
Collaborator

Normally we'd check first whether Qt can be found; if not, the path to the Externals is added for a second attempt.

Is this typical in C++ projects? I'm fine with it if yes. It seems a bit strange to me, since I could potentially build with an unintended Qt version locally without realizing it if I installed Qt for other projects.

@cristian64
Copy link
Collaborator

I figured out how to make it first check whether Qt is already installed and revert to Externals for a second attempt for windows only using an if else block and it builds fine on Visual Studio, so I could add that commit instead.

I think we are after two distinct PRs here.

  • One PR (this one) to fix the MSVC project
  • One PR to improve the CMake file so that it can find the external Qt

@cristian64
Copy link
Collaborator

cristian64 commented Jul 4, 2024

Is this typical in C++ projects? I'm fine with it if yes. It seems a bit strange to me, since I could potentially build with an unintended Qt version locally without realizing it if I installed Qt for other projects.

For example, in Dolphin, although it provides many external libraries, it usually tries to use the system's (or whatever is defined in the environment) first. Only when they cannot be found does it fallback to using the external libraries. It adds some information messages during the configuration step, announcing the use of the external library when one could not be found. (At least this is the case on Linux.)

@jahorta
Copy link
Contributor Author

jahorta commented Jul 5, 2024

Okay, for this PR I removed the line for loading moc_AddressInputWidget from the project file.

@dreamsyntax dreamsyntax merged commit 9736e67 into aldelaro5:master Jul 5, 2024
cristian64 pushed a commit to cristian64/dolphin-memory-engine that referenced this pull request Jul 6, 2024
…ile is included during build (aldelaro5#165)

* AddressInputWidget.h is never moc'ed but the compiler is instructed to load moc_AddressInputWidget.h. Include for moc file is now commented out.

* Removed the line which looks for a moc_AddressInputWidget.h file from the project file.
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.

3 participants