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

State of cmake files #86

Closed
mrbean-bremen opened this issue Jan 4, 2023 · 14 comments
Closed

State of cmake files #86

mrbean-bremen opened this issue Jan 4, 2023 · 14 comments

Comments

@mrbean-bremen
Copy link

Not an issue, but a question:

I would like to re-add cmake support to (original) PythonQt (additionally to qmake support that seems still to be needed by some users). I have noticed that you already have done this, and that you also support current Qt 5 versions (despite the note in the top-level CMakeLists.txt). Some earlier version of your changes had been integrated some time in the past, and been removed again a few years ago.

My questions are:

  • what is the state here - is it ready to use (I guess patched-10 is where to look)?
  • do you want to make a PR for this, or should I just copy your files (and adapt if needed) and make a PR on my own?

Thanks

@pieper
Copy link
Member

pieper commented Jan 4, 2023

We are currently using the patched-9 branch, at least as the default that's used when building Slicer. It's build routinely on windows, mac, and linux so it should be in good shape.

Our goal for all forks like this is to keep them as close to upstream as possible, and eliminate the fork if we are able to use the upstream version instead so I think from a CTK perspective we'd love to see native cmake support in PythonQt. Everything has been relatively stable for the past several years but with the impending switch to Qt6 it will probably need to be revisited.

I think that if you are up for making a fresh PR it could be good. I'm not sure but there may be easier ways to do things with modern cmake practices.

@jcfr or @nolden may have comments.

@mrbean-bremen
Copy link
Author

Thanks @pieper - I will go ahead then and use your patched-9 branch as the base for a PR, but I will wait a bit for other suggestions. This so far is for Qt 5 - with Qt 6, there will certainly be major changes.

Our goal for all forks like this is to keep them as close to upstream as possible, and eliminate the fork if we are able to use the upstream version instead

What would that entitle, besides the cmake support?

@lassoan
Copy link
Member

lassoan commented Jan 4, 2023

I would not recommend a fork either, as we'll add Qt6 and i18n support in the next couple of months.

Note that Qt has switched to CMake (see https://www.qt.io/blog/qt-and-cmake-the-past-the-present-and-the-future), so I would not spend time with adding qmake support.

@pieper
Copy link
Member

pieper commented Jan 4, 2023

What would that entitle, besides the cmake support?

In this case I can't think of anything other than cmake support. For some projects like VTK we sometimes have fixes that aren't merged upstream.

If I recall correctly we don't have cmake build support for the binding generator code since we have always been using the generated_cpp directories from the PythonQt repo. When I have generated them myself I have used qmake, so there is probably some work to be done there. Also it would be nice to have the option to generate the bindings automatically as part of the build process as this would help keep them in sync with the version of Qt being used.

@mrbean-bremen
Copy link
Author

mrbean-bremen commented Jan 4, 2023

I would not recommend a fork either, as we'll add Qt6 and i18n support in the next couple of months.

Thanks - no, this is about adding cmake support to the original repo, the question was just how to do this best given that you have already done that. How are you going to add Qt6 support - are you going to use shiboken for the generator?

Note that Qt has switched to CMake (see https://www.qt.io/blog/qt-and-cmake-the-past-the-present-and-the-future), so I would not spend time with adding qmake support.

Yes, that is one reason, why we want to switch to cmake. qmake support is already there and will just not be removed for Qt 5 - what will happen with qt 6 is another matter.

@mrbean-bremen
Copy link
Author

Also it would be nice to have the option to generate the bindings automatically as part of the build process as this would help keep them in sync with the version of Qt being used.

Yes, I agree, though there still is some work to do if you really want to support a version due to added or changed API (See also my comment here).

@nolden
Copy link
Member

nolden commented Jan 5, 2023

@jcfr or @nolden may have comments.

Thanks for mentioning me, unfortunately (?) I haven't touched Qt or any CMake files for a while now ;)

@iltommi
Copy link

iltommi commented Jan 5, 2023

my 2 cents: If I recall correctly, the cmake and qmake produce slightly different libraries.
If you activate the QtAll option the cmake version creates a single library whereas the qmake produces two separated libraries.

@jcfr
Copy link
Member

jcfr commented Jan 5, 2023

If you activate the QtAll option the cmake version creates a single library whereas the qmake produces two separated libraries.

We can definitely add (or help add) missing features to generate the expected artifacts.

To get started, in the next hour or so I will push a pull-request in the upstream project. This will give proper credit and re-introduce CMake support.

@mrbean-bremen
Copy link
Author

I would not recommend a fork either, as we'll add Qt6 and i18n support in the next couple of months.

@lassoan - To clarify, as I may have misunderstood: is this about adding Qt6 support to PythonQt, or is this about Slicer? And if this is about PythonQt, are going to replace the parser with Shiboken, or do something else?

I'm asking, because we also plan to do this (our current plan is to use Shiboken), but probably a bit later. Just to make sure that we won't duplicate the work...

@mrbean-bremen
Copy link
Author

To get started, in the next hour or so I will push a pull-request in the upstream project.

@jcfr - is this still on your list? Otherwise we probably would do it ourselves (though not sure about when, certainly not in the next hour 😁)

@mrbean-bremen
Copy link
Author

Closing - we will add cmake support for Qt6.

@jcfr
Copy link
Member

jcfr commented Jul 27, 2023

we will add cmake support for Qt6.

This is great.

push a pull-request in the upstream project. This will give proper credit and re-introduce CMake support.

As a side note, to bootstrap the effort, I still plan on doing this. I just didn't have a chance to get to it.

@mrbean-bremen
Copy link
Author

As a side note, to bootstrap the effort, I still plan on doing this. I just didn't have a chance to get to it.

Good to hear!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants