mne-tools / mne-cpp

MNE-CPP: A Framework for Electrophysiology
https://mne-cpp.org/
BSD 3-Clause "New" or "Revised" License
151 stars 137 forks source link

[FIX] Minor bug cmakelists bin directory name #924

Closed juangpc closed 1 year ago

juangpc commented 1 year ago

Not yet ready.

juangpc commented 1 year ago

@gabrielbmotta I don't follow the logic here. If qt-version is 6 something, else if it is android something else? It is probably correct. Just want to make sure. Thanks

https://github.com/juangpc/mne-cpp/blob/e8277db99d2f1fa0e8c23be480229e6a9c348301/src/applications/mne_scan/mne_scan/CMakeLists.txt#L43

gabrielbmotta commented 1 year ago

This syntax is the default formatting used for building Qt apps in cmake, so it technically supports more platforms than we currently do. It can be removed without issue, but like the Qt 6 compatibility, I figured if Qt has provided it for us I would leave it in. Granted, Qt 6 support is a much more useful thing to us than android support, since a move to Qt 6 is something that will need to happen if the project continues with Qt, while a mobile version of our applications isn't even something we've thought about.

Long story short, it is there because we got it "for free." I'm not opposed to it being removed, since the "else" clause is really all we are using now.

juangpc commented 1 year ago

I think you should decide. My take is that even if it comes for free, I'd remove it. But it doesn't bother me too much.

This looks nice, to the point. The code as it is now, seems like it needs some explanation. But again, I think you should decide.

if(${QT_VERSION_MAJOR} GREATER_EQUAL 6)
    qt_add_executable(${PROJECT_NAME} MANUAL_FINALIZATION ${SOURCES} ${HEADERS} ${RESOURCES})
else()
    add_executable(${PROJECT_NAME} ${SOURCES} ${HEADERS} ${RESOURCES})
endif()
gabrielbmotta commented 1 year ago

Yes, I like the version you posted. I would not want to remove the Qt 6 check, but the android one can totally be removed for the sake of simplicity and clarity.