pothosware / PothosSDR

Pothos SDR windows development environment
https://github.com/pothosware/PothosSDR/wiki
313 stars 48 forks source link

Add GNURadio Qt4 support #49

Closed eduardosm closed 6 years ago

eduardosm commented 6 years ago

This is an initial and almost untested Qt4 support for GNURadio.

I know you said somewhere you didn't want to include Qt support until GNURadio used Qt5, but I am creating the pull request in case you still want to review it.

It has some ugly workarounds, like patching generated makefiles with PowerShell comands...

guruofquality commented 6 years ago

I took a quick look, and so far it looks good. I'm not really against qt4 per say, but it looks like a lot of work, and qt5 was supposed to be "any day now". So I appreciate that and the other dependencies too.

My only other concern is the Qt4 dlls that get packaged, I need to make sure they don't conflict with Qt5 dlls. The main Qt dlls are fine, they have the version number in the name, but we should double check on "${QT5_LIB_PATH}/bin/icu*.dll" and the plugins:

file(GLOB QT5_ICU_DLLS "${QT5_LIB_PATH}/bin/icu*.dll")

install(FILES
    ${QT5_ICU_DLLS}
    "${QT5_LIB_PATH}/bin/libGLESv2.dll"
    "${QT5_LIB_PATH}/bin/libEGL.dll"
    "${QT5_LIB_PATH}/bin/Qt5Core.dll"
    "${QT5_LIB_PATH}/bin/Qt5Gui.dll"
    "${QT5_LIB_PATH}/bin/Qt5Widgets.dll"
    "${QT5_LIB_PATH}/bin/Qt5Concurrent.dll"
    "${QT5_LIB_PATH}/bin/Qt5OpenGL.dll"
    "${QT5_LIB_PATH}/bin/Qt5Svg.dll"
    "${QT5_LIB_PATH}/bin/Qt5PrintSupport.dll"
    "${QT5_LIB_PATH}/bin/Qt5Network.dll"
    DESTINATION bin
)

install(FILES "${QT5_LIB_PATH}/plugins/platforms/qwindows.dll" DESTINATION bin/platforms)
install(FILES "${QT5_LIB_PATH}/plugins/iconengines/qsvgicon.dll" DESTINATION bin/iconengines)
guruofquality commented 6 years ago

Another concern is the QMAKE_CXXFLAGS_RELEASE -= -arch:AVX2` flags. SSE2 seems to always be fine given its age, but I will definitely hear about unknown instructions and crashing given that AVX2 and sadly AVX is not a universally guaranteed support.

eduardosm commented 6 years ago

Right now I'm only packaging QtCore4.dll, QtGui4.dll, QtSvg4.dll and QtOpenGL4.dll, as well as qwt6.dll. There are not icu dlls and I'm not including any qt4 plugin. I'm don't know if gr-qtgui needs anything else at runtime, but I tested some widgets (constellation sink, frequency sink, time sink, number sink), which worked fine. I will do some more testing later, but first I want to make sure everything builds from scratch in a single run without errors.

Rearding AVX2, I thought that it was disabled by default, but it might be good idea to disable it explcitly. Shall QMAKE_CXXFLAGS_RELEASE be added to every qmake command or only when building qt4?

eduardosm commented 6 years ago

By the way, PowerShell scripts are right now using CRLF line ends, shall I convert them to LF?

eduardosm commented 6 years ago

Or it would better to replace them with commands like ${CMAKE_COMMAND} -E env PATH=...?

guruofquality commented 6 years ago

Sorry for the delay, a lot going on. I left some additional review comments, mostly about qwt.

By the way, PowerShell scripts are right now using CRLF line ends, shall I convert them to LF?

No worries, theres no way to keep this consistent anyway. Any line ending is great. :-P

Or it would better to replace them with commands like ${CMAKE_COMMAND} -E env PATH=...?

If you can do something by invoking cmake, and its more or less as simple, thats preferred. But invoking powershell is fine.

eduardosm commented 6 years ago

I left some additional review comments, mostly about qwt.

I can't find them. Maybe I don't have permission to view them?

If you can do something by invoking cmake, and its more or less as simple, thats preferred. But invoking powershell is fine.

I don't know if I can completly avoid invoking powershell, as I'm using some of its features to patch generated Makefiles (theres isn't cmake -E sed ☹), but we can avoid having script files using powershell -Command , as done here: https://github.com/eduardosm/PothosSDR/blob/ce621dafa9dfc73a4e4ca51e1050b493ff7e5a7a/BuildGrQt4Deps.cmake#L82

guruofquality commented 6 years ago

I can't find them. Maybe I don't have permission to view them?

I forgot to click submit. I was trying something new with github

I don't know if I can completly avoid invoking powershell, as I'm using some of its features to patch generated Makefiles (theres isn't cmake -E sed ☹), but we can avoid having script files using

Ahh they are generated, that explains one thing I mentioned. I think the powershell find/replace is perfectly fine. cmake can call another cmake script which can use the string(REPLACE ...) function. But I bet that would be overkill anyway, where this is pretty simple with powershell

eduardosm commented 6 years ago

Regarding qwt, gnuradio uses qwt both in C++ and python code. For python, it uses pyqwt5, which depends on qwt5. However, I missing symbols issues with C++ code using qwt5, so I had to include qwt6 too (they do the same here: https://github.com/gnieboer/GNURadio_Windows_Build_Scripts).

qwt5 is built as a static library (otherwise I had missing symbol errors when building pyqwt5) and qwt6 as a dll. Maybe I can try to build qwt6 as a static library too, so we don't have to worry about its name.

guruofquality commented 6 years ago

I gave you access to this repo if you are interested. Work has been keeping me busy, but I hope to build and upload something this weekend. Dont worry about perfection, its all an iterative process. I'm sure we can do qwt better.

eduardosm commented 6 years ago

Thanks! I'm right now busy at university, so I won't be able to focus on this for some time.

guruofquality commented 6 years ago

no problem. The effort was much appreciated!