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

Fix Qt5LinguistTools detection/lrelease binary location, use FeatureSummary #614

Merged
merged 1 commit into from Nov 26, 2020

Conversation

a17r
Copy link
Contributor

@a17r a17r commented Jan 11, 2018

If Qt4 is default in qtchooser, cmake was picking up the wrong (qt4-based) lrelease binary.

@MaartenBaert
Copy link
Owner

You are making a number of unrelated changes, can you explain what each of them do?

  • FeatureSummary: Why is this needed?
  • FFMPEG_FOUND: This doesn't seem to be used anywhere.
  • LRELEASE: is using the wrong lrelease binary actually a problem? Because it seems to work fine for me when I use the default qt4-based lrelease even when I'm compiling with qt5.
  • QT_MIN_VERSION: Why version 5.3.1 specifically? It seems strange that SSR is able to support Qt4 but not Qt5 below 5.3.1.

@a17r
Copy link
Contributor Author

a17r commented Jan 12, 2018

FeatureSummary: Why is this needed?

It is just nice to have an overview of enabled/optional features at the end of configure.

FFMPEG_FOUND: This doesn't seem to be used anywhere. You could make it even more useful and verbose by defining SET_PACKAGE_PROPERTIES for optional deps.

FFMPEG_FOUND was necessary because feature_summary(WHAT ALL FATAL_ON_MISSING_REQUIRED_PACKAGES) is checking that status and failed otherwise.

LRELEASE: is using the wrong lrelease binary actually a problem? Because it seems to work fine for me when I use the default qt4-based lrelease even when I'm compiling with qt5.

Then it works by chance, and you should not rely on that. With lrelease being one of those magic Qt binaries that exist in both Qt4 and Qt5, there exist different workarounds among distributions. With Qt5LinguistTools you can ensure the dep is actually there.

QT_MIN_VERSION: Why version 5.3.1 specifically?

<5.3.1 did not provide the Qt5::lrelease target.

@a17r
Copy link
Contributor Author

a17r commented Apr 1, 2018

ping, anything left for me to do here?

@MaartenBaert
Copy link
Owner

I had forgotten about this, I've made some changes to the build system since then including how Qt selects the correct binary (based on the QT_SELECT environment variable). As far as I know this has fixed all issues, so I don't think this is necessary anymore. I prefer to keep the build scripts simple and rely on the built-in Qt package for cmake.

@a17r
Copy link
Contributor Author

a17r commented Apr 15, 2018

Using QT_SELECT is a workaround, not a solid fix. But I'll have a look at the delta.

I prefer to keep the build scripts simple and rely on the built-in Qt package for cmake.

I think my changes are consistent with that statement. Dropping Qt4 support would make it much simpler though, but I'm not suggesting that; I'm happy enough that we were able to keep your package thanks to Qt5 support. :)

@MaartenBaert
Copy link
Owner

QT_SELECT is the method that's recommended by the Qt project itself, so I don't see the problem. It's backward-compatible and it's the easiest way to support both Qt4 and Qt5 without complex build scripts.

I'm trying to re-add some of the other things though, like the Qt version check.

@a17r
Copy link
Contributor Author

a17r commented Nov 23, 2020

QT_SELECT is the method that's recommended by the Qt project itself, so I don't see the problem.

QT_SELECT is only a clutch for the end user to select a default Qt visible in PATH and it relies on qtchooser being present and correctly set up, it is downright invalid for build systems to depend on it. There's nothing 'complex' about doing it properly in the build system, 95% of packages get it right out there or were fixed during Qt4 to Qt5 transition.

qtchooser is being phased out by Qt upstream anyway so you can not rely on that implicit dependency in the future.

Correctly find the Qt5 module that provides the path to Qt5-based lrelease.
Available since >= Qt-5.3.1 which is well below the current minimum version.
gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Nov 23, 2020
…PEND

See also: MaartenBaert/ssr#614

Package-Manager: Portage-3.0.10, Repoman-3.0.2
Signed-off-by: Andreas Sturmlechner <asturm@gentoo.org>
@MaartenBaert MaartenBaert merged commit 6518c47 into MaartenBaert:master Nov 26, 2020
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.

None yet

2 participants