commontk / CTK

A set of common support code for medical imaging, surgical navigation, and related purposes.
https://commontk.org
Apache License 2.0
827 stars 480 forks source link

Can't build on linux with current slicer instructions #1196

Closed pieper closed 3 months ago

pieper commented 3 months ago

Using the current linux build instructions on a VM with ubuntu 20.04 ctk build fails due to new APIs being used that aren't available in Qt 5.12.8.

$ qmake --version
QMake version 3.1
Using Qt version 5.12.8 in /usr/lib/x86_64-linux-gnu

One example that I fixed by hand:

  QList<QStandardItem*> list = this->findItems(tr("completed"), Qt::MatchContains, Columns::Status);
  //QList<QStandardItem*> list = this->findItems(tr("completed"), Qt::MatchRegularExpression, Columns::Status);

after that, I get this error:

[ 66%] Building CXX object Libs/DICOM/Widgets/CMakeFiles/CTKDICOMWidgets.dir/ctkDICOMVisualBrowserWidget.cpp.o
In file included from /workspaces/Slicer-superbuild/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp:61:
/workspaces/Slicer-superbuild/CTK-build/CTK-build/Libs/DICOM/Widgets/ui_ctkDICOMVisualBrowserWidget.h: In member function ‘void Ui_ctkDICOMVisualBrowserWidget::retranslateUi(QWidget*)’:
/workspaces/Slicer-superbuild/CTK-build/CTK-build/Libs/DICOM/Widgets/ui_ctkDICOMVisualBrowserWidget.h:551:45: error: ‘class ctkCheckableComboBox’ has no member named ‘setPlaceholderText’
  551 |         FilteringModalityCheckableComboBox->setPlaceholderText(QString());
      |                                             ^~~~~~~~~~~~~~~~~~
make[5]: *** [Libs/DICOM/Widgets/CMakeFiles/CTKDICOMWidgets.dir/build.make:569: Libs/DICOM/Widgets/CMakeFiles/CTKDICOMWidgets.dir/ctkDICOMVisualBrowserWidget.cpp.o] Error 1
make[4]: *** [CMakeFiles/Makefile2:693: Libs/DICOM/Widgets/CMakeFiles/CTKDICOMWidgets.dir/all] Error 2
make[3]: *** [Makefile:136: all] Error 2
make[2]: *** [CMakeFiles/CTK.dir/build.make:87: CTK-prefix/src/CTK-stamp/CTK-build] Error 2
make[1]: *** [CMakeFiles/Makefile2:227: CMakeFiles/CTK.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

Commenting this out allows build to work.

It would be good to hold off on using these APIs until they arre available on all build platforms easily.

pieper commented 3 months ago

@Punzo FYI

Punzo commented 3 months ago

hei Steve,

thanks for reporting. I tested indeed only with Qt 5.15.2

I can probably have a look to replace those Qt API next week or maybe in the next next.

I have few general questions: 1) Is there any specific reason why you used Qt version 5.12.8? In the Slicer prerequisites is stated that version 5.15.2 is the recommended one and the usage of 5.15.2 is discouraged: image 2) Do we really would like to back-maintain CTK/Slicer to older version of Qt than 5.15.2? Considering that Qt5 official support ended in May 2023 and we should probably move to Qt6 soonish. 3) If we actually would like to back-maintain CTK/Slicer to older versions of Qt than 5.15.2, then we should make it clear in the development guidelines and state which version should be used (e.g. in https://slicer.readthedocs.io/en/latest/developer_guide/contributing.html)

I raise these issues, because from a developer point of view, having locally multiple Qt installations is not feasible: i.e. to change those two simple lines: 1) I will have to donwload Qt 5.12.8, 2) rebuild Slicer on my ubuntu 23.10. 3) this would use a lot of hard disk space + building time 4) At the same time it is probably not the best to use Qt 5.12.8 as default dev enviroment, as Slicer CI and deployment uses version 5.15.2.

@jcfr @lassoan

jcfr commented 3 months ago

If we actually would like to back-maintain CTK/Slicer to older versions of Qt than 5.15.2

Support for 5.12.8 is done as a best effort and there are no issue waiting for feedback from the community when something break.

I will look into fixing the issue and update the guideline accordingly.

After we release Slicer built against Qt6, I then suggest we release CTK and document that moving forward only support for 5.15 will be maintained.

pieper commented 3 months ago

Hi @Punzo @jcfr -thanks for looking into this - I was testing building Slicer in a github codespace, so I got the default OS (Ubuntu 20.04) and followed the build instructions from Slicer's readthedocs which at least for now I think we should expect to work. With just these couple small exceptions and the related one in Slicer the compilation works with the distribution's Qt, so I think we want to keep supporting this at least until the Qt6 transition.

You are correct that we should look at how to clarify the documentation to be consistent on this.

In terms of testing Davide, it's very easy and free to replicate what I tried. You can just open the Slicer repo in a codespace in your browser. I used an 8 core machine and it built in a few hours and I could run it in the codespace VM using Xvfb with x11vnc. I built Slicer and some other projects and so far I've only used 30 compute hours and they give away 60 compute hours/month free, so it shouldn't be hard for people to test this way. I'm signed up for the $4/month github plan that includes 180 hours. Now that I know this works I think it's a great option for debugging and testing branches.

If there were a major bug or functional difference I'd be fine with dropping support for Qt 5.12 but for just a few small methods I think it's worth keeping things compatible for now.

Punzo commented 3 months ago

In terms of testing Davide, it's very easy and free to replicate what I tried. You can just open the Slicer repo in a codespace in your browser. I used an 8 core machine and it built in a few hours and I could run it in the codespace VM using Xvfb with x11vnc. I built Slicer and some other projects and so far I've only used 30 compute hours and they give away 60 compute hours/month free, so it shouldn't be hard for people to test this way. I'm signed up for the $4/month github plan that includes 180 hours. Now that I know this works I think it's a great option for debugging and testing branches.

Ah that's nice to know (: thanks!

lassoan commented 3 months ago

@pieper It would be nice if you could write a short tutorial (maybe post it on the Slicer forum) about how to use codespaces for this. I also noticed that I see some codespace that I think @jcfr created. Is it possible to share these codespaces? Or maybe the codespace was created automatically when I reviewed his pull request.

jcfr commented 3 months ago

re: codespace

I do not recall creating such codespace (at least not intentionally)

pieper commented 3 months ago

The code space issue is a bit orthogonal to the point of this issue. My point is that if I start with a fresh ubuntu 22.04 image and follow our current readthedocs instructions I get these few build errors - this would have happened to anyone trying a fresh build on that OS. We do have a warning currently that a newer Qt should be used (download from Qt.io instead of using apt-get installed packages) but maybe we should make that the main documented way so we can avoid this issue.

you could write a short tutorial

At this point it's a bit of an experiment to see if github codespace was useful or not, and I'm not yet convinced it's a good fit for Slicer. It's certainly convenient and it did work and I didn't go over my compute hours limit building and testing Slicer, but I did hit my storage limit for the month (20 GB / month) so now I'm locked out from codespace for 3 weeks unless I authorize charges. So until we can demonstrate utility compared to just building inside a local docker or other VM I'm not sure if it's worth investing time in it. Codespaces could be handy for smaller projects but Slicer builds are just a bit to big to be practical I think.

I do not recall creating such codespace

No, I created one in my own github account for testing. I think @lassoan was suggesting maybe we could standardize on a codespace image that people start from for testing builds? I believe we could do that as a template but I'm not sure it's any better than a docker image with a pre-installed build environment.

Punzo commented 3 months ago

The code space issue is a bit orthogonal to the point of this issue. My point is that if I start with a fresh ubuntu 22.04 image and follow our current readthedocs instructions I get these few build errors - this would have happened to anyone trying a fresh build on that OS. We do have a warning currently that a newer Qt should be used (download from Qt.io instead of using apt-get installed packages) but maybe we should make that the main documented way so we can avoid this issue.

btw I did not forget about this. I just got super busy from other higher priority tasks. I will just try to have a debug local build with Qt 5.12 and then I will see to fix the issue. I will try to do it within this week. I will let you know asap (:.

jcfr commented 3 months ago

I pushed a fix in the PR where I am switching CI to GitHub