Always use lrelease binary from QT_INSTALL_BINS. #2852

Merged
merged 1 commit into from Feb 19, 2017

Projects

None yet

2 participants

@davidebeatrici
Member

In MXE's Qt 5 package the binary is called lrelease, unlike QMake's one, which has a prefix.
This resulted in the script using the wrong name.

@Kissaki Kissaki requested a review from mkrautz Feb 16, 2017
lrelease.pri
+# that can be found in the LICENSE file at the root of the
+# Mumble source tree or at <https://www.mumble.info/LICENSE>.
+
+# This .pri files finds a suitable protoc binary on the system.
@mkrautz
mkrautz Feb 16, 2017 Member

The whole comment block is wrong. It talks about protoc.

Just do a simple description of what this pri files does, instead.
No need for a big one like for the other scripts that are more complicated.

@davidebeatrici
davidebeatrici Feb 16, 2017 Member

Ops... copy & paste without looking at the header...

lrelease.pri
+include(qt.pri)
+
+isEmpty(QMAKE_LRELEASE) {
+ isEqual(QT_MAJOR_VERSION, 5) {
@mkrautz
mkrautz Feb 16, 2017 Member

I think this file can simply be:

isEmpty(QMAKE_LRELEASE) {
    QMAKE_LRELEASE = $$[QT_INSTALL_BINS]/lrelease
}

for all Qt versions. Qt 4 has QT_INSTALL_BINS too.

lrelease.pri
+ QMAKE_LRELEASE_PATH = $$dirname(QMAKE_QMAKE)/$$replace(QMAKE_QMAKE_BASE,qmake,lrelease)
+ QMAKE_LRELEASE = $$QMAKE_LRELEASE_PATH
+ }
+}
@mkrautz
mkrautz Feb 16, 2017 Member

No newline at the end of the file.

lrelease.pri
+
+# This .pri file finds the correct lrelease binary name on the system.
+
+include(qt.pri)
@mkrautz
mkrautz Feb 16, 2017 Member

You no longer need qt.pri.

@mkrautz
Member
mkrautz commented Feb 17, 2017

Please squash the commits, and name this something lile: "Always use lrelease binary from QT_INSTALL_BINS."

@mkrautz mkrautz changed the title from Fix lrelease path detection for Qt 5 to Always use lrelease binary from QT_INSTALL_BINS. Feb 19, 2017
@mkrautz
Member
mkrautz commented Feb 19, 2017

Actually, I think the commit summary should be:

Sometimes, Qt packages in Linux distros and the like provide an 'lrelease' binary in $PATH.
That is what we've relied on in mumble.pro previously.

However, that doesn't always work for all Qt installations.

To fix that, this change introduces a new lookup strategy for the 'lrelease' binary.
Instead of relying on it being in the user's default $PATH, we use qmake's QT_INSTALL_BINS
variable. That path is guaranteed to contain an 'lrelease' binary if the correct packages are
installed -- for both Qt 4 and Qt 5.

@davidebeatrici davidebeatrici Always use lrelease binary from QT_INSTALL_BINS.
Sometimes, Qt packages in Linux distros and the like provide an 'lrelease' binary in $PATH.
That is what we've relied on in mumble.pro previously.

However, that doesn't always work for all Qt installations.

To fix that, this change introduces a new lookup strategy for the 'lrelease' binary.
Instead of relying on it being in the user's default $PATH, we use qmake's QT_INSTALL_BINS
variable. That path is guaranteed to contain an 'lrelease' binary if the correct packages are
installed -- for both Qt 4 and Qt 5.
b50ab76
@mkrautz mkrautz merged commit 17bae48 into mumble-voip:master Feb 19, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment