MayaPosch / NymphCast

Audio and video casting system with support for custom applications.
http://nyanko.ws/nymphcast.php
BSD 3-Clause "New" or "Revised" License
2.42k stars 81 forks source link

Add CMake support to NymphcastPlayer #78

Closed PureTryOut closed 8 months ago

PureTryOut commented 1 year ago

QMake is slowly being deprecated by the Qt project. From Qt 6.0 and onwards Qt itself is already building with CMake and not with QMake anymore and it's only a matter of time til QMake itself will be removed.

This commit introduces the CMake build system and successfully builds the project for at least Linux.

I haven't managed to test the Windows support as I don't have or use Windows and neither Android support as I fail to build Poco properly for Android...

/home/bart/Documents/Git/nymphcast/poco/Foundation/src/Thread_POSIX.cpp:94:7: error: use of undeclared identifier 'pthread_getname_np'
                if (pthread_getname_np(pthread_self(), name, POCO_MAX_THREAD_NAME_LEN + 1))
                    ^
PureTryOut commented 1 year ago

Seems my editor doesn't like how mainwindow.cpp is formatted... The only relevant line change there is adding #include "mainwindow.moc" at the bottom.

MayaPosch commented 1 year ago

I'm confused by the including of mainwindow.moc in mainwindow.cpp, as it is not necessary with the qmake-based flow I use. Googling around, I can see that some examples supposedly do this, with the rationale being that for Q_Objects defined in source files this is needed due to limitations in the build system. Yet again, this is not something that I have ever had to do.

Is this a limitation of the CMake-based build system? Since I have no plans to migrate NymphCast Player beyond Qt 5.15 LTS for the foreseeable future, I would very much like to keep qmake as the primary build option, with the CMake version as a parallel, secondary option. Assuming no modifications to the source are required, which does seem to be the case here?

PureTryOut commented 1 year ago

To be honest I do not understand the full jist of it either. However this StackOverflow answer seems relevant. https://stackoverflow.com/a/34929627

Basically because there is a class definition using Q_OBJECT in a .cpp file rather than a header file, the compiler can't see the class definition when compiling the moc output (as you can't #include a .cpp file). #include'ing the moc output at the end of the .cpp makes the compiler compile everything together so everyone is happy. From what I understand this isn't necessary if the class definition were done in the header file (as that header file can be #included). A quick test (moving the definition of MediaItem to mainwindow.h) indeed reveals this is true. I was able to remove the #include at the bottom and it compiles fine. That makes me wonder, why is a class definition in the .cpp rather than the header file anyway?

Now why QMake doesn't mind this at all I don't understand. But if that one line fixes it (or we can move the class definition if that is preferable) then I don't really see the problem either?

PureTryOut commented 11 months ago

@MayaPosch do you agree with my previous comment? What are the next steps to get this in?

PureTryOut commented 8 months ago

Any update @MayaPosch? :wink:

MayaPosch commented 8 months ago

Sorry for letting this issue drop off the radar. To clarify, you can absolutely #include a .cpp file, the preprocessor doesn't care about the file extension, it just tapes the target file into the origin file at that location.

I'll have a look at this issue and the outstanding question in more detail over the course of this week.

PureTryOut commented 8 months ago

Sure it's technically possible, but it's something that shouldn't be done. In fact Linus Torvalds had a rant about it recently on the Linux mailing list (although that was about a .c file). It seems QMake is more tolerant and doesn't care where CMake ignores the file completely.

Preferably class definitions are done only in header files, so is there any reason for keeping the definition of MediaItem in the .cpp file?

MayaPosch commented 8 months ago

You're right about the MediaItem class definition, sorry for not catching on sooner. I have moved it to mainwindow.h as suggested. For qmake-based compilation this makes at least no difference.

It should now be possible to revert the changes in this PR to mainwindow.cpp. Only suggestion I'd have for the PR then is QMake isn't marked as 'deprecated', as it isn't marked as such by Qt. In Qt 6 CMake is the preferred build file generator, but looking at the Qt 6.6 notes there are no deprecation marks or plans, meaning one is free to use whatever for new projects.

I'll have a look at the Poco-for-Android issue too. I assume you're using the Poco build project Makefiles for this per the instructions? Did you create an issue ticket for this already?

PureTryOut commented 8 months ago

I know Qt hasn't officially deprecated QMake, I meant "deprecated" here just for this project, personally I'd rather keep the whole thing to 1 build system (which would be CMake) and I see no need to support multiple, so deprecation meant here that I'd remove QMake at some point. But I removed the mentions, now both options are mentioned in the same manner.

I assume you're using the Poco build project Makefiles for this per the instructions?

Uh, it's been almost a year ago since I tried it, I honestly don't know. I find building for Android hard to grasp in general so I doubt I deviated from any official instructions :sweat_smile:

MayaPosch commented 8 months ago

Thank you for the changes :)

As for NCP for Android, I had a friend build the player for Android from scratch, so I'm pretty sure it should work. If you have any issue with it in the future, feel free to post a ticket :)