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

Add detailed build instructions to Readme #235

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

tonka3000
Copy link
Contributor

Add more detailed build instructions to the Readme to avoid some common pitfalls (see #230) when building PythonQt.

For now I left the original build section in the Readme, but in my opinion the instructions can be removed.

Currently I mention of the env-vars I used for building it with conan, maybe some are not necessary in all configurations.

@tonka3000
Copy link
Contributor Author

@mrbean-bremen

Copy link
Contributor

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to say that I have not used qmake with PythonQt, just cmake, so I'm not the best to review this. Maybe @he-hesce can have another look...

### ⚠️ Pregenerated Bindings Warning ⚠️

It is highly recommended to build the Qt bindings yourself and not(!) use the pregenerated ones. To ensure that, delete the pregenerated directory starting with `generated_cpp_*`.
Do not build `PythonQt.pro` directly because it will only use the pregenerated bindings!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it first checks for the path "generated_cpp", assuming that your self-generated sources are there, and only if it does not exist it checks for the other paths (see common.prf).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. No need to remove any pre-generated bindings. The generated_cpp will be used if created by user. I once tried to generate bindings and failed, so I've used 5.6 pre-gen bindings for a long time instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it is a valid point - it is always preferrable to use the bindings generated for your concrete version instead of a pre-generated one for an older version. That generating the bindings did not work reliably before was one reason to use the pre-generated bindings that is now obsolete.
So maybe lessen the tone of the recommendation a bit ("It is recommended to build the Qt bindings yourself and not use the pregenerated ones."), and add a bit of a rational.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've lessen the tone.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not build PythonQt.pro directly because it will only use the pregenerated bindings!

This is partly true, in the sense that while it does build the generator first, it does not generate the bindings, so if you haven't built them before it will indeed use the checked-in ones.
I think you can leave it as is, but ultimately PythonQt.pro shall be adapted to also generate the bindings, maybe depending on some variable (or add the CMakeLists.txt files, or both).

README.md Outdated

### ⚠️ Pregenerated Bindings Warning ⚠️

It is highly recommended to build the Qt bindings yourself and not(!) use the pregenerated ones. To ensure that, delete the pregenerated directory starting with `generated_cpp_*`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add some reasoning, like "to ensure that the bindings are always compatible with your Qt version".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From experience, 5.6 pre-gen bindings worked fine with 5.9, 5.12, and 5.15 albeit you never get the newer constructs and methods of course. 5.15 pre-gen bindings work fine with 5.15.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used it with 5.11 and I got issues see #230

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrbean-bremen I added your mentioned reasion

README.md Outdated
Comment on lines 118 to 119
## Building on Windows with MinGW

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this part is now redundant. You may mention above that you can also build under Windows using MinGW and make.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no experience with Windows so no idea if the above works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrbean-bremen Yes, it is redundant. I now mention that MinGW is an option on windows as you suggest.

@he-hesce
Copy link
Contributor

he-hesce commented Sep 15, 2024

I have to say that I have not used qmake with PythonQt, just cmake, so I'm not the best to review this. Maybe @he-hesce can have another look...

Well, I think those qmake lines will work though we build it differently. We place the pythonqt directory into our libsrc directory and add it to the libsrc.pro file in that directory. Then it gets built as part of the application build. It is then linked at the top-level project file. We do have to hack some of the lib paths in the PythonQt pro files for it to place build libraries where we place our other built libraries but that is extracurricular for our build environment/system. We do build our application including PythonQt using qmake which works fine and hopefully will continue to do so as we need to use and support qmake/qt5.15 for another decade or so (RHEL/Rocky 9).

@tonka3000
Copy link
Contributor Author

@he-hesce The qmake commands work because they are straight out of my conan recipe 😄 . I'm using 99% cmake, PyhonQt is the only project I use qmake for, so am not an expert in qmake. Also need to support 5.11 - 5.15 for now. But hopefully I make the jump to Qt 6 after having upgraded PythonQt to the lastest version.

Copy link
Contributor

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will wait with the merge a couple of days in case somebody else wants to chime in.

@mrbean-bremen mrbean-bremen merged commit e2bdefa into MeVisLab:master Sep 19, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants