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

Unable to read messages from localized versions of utilities. #317

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SokoloffA
Copy link

Fix: Unable to read messages from localized versions of utilities.
If the utilities output localized messages, these messages are unreadable. For example, instead

ERROR: getBinaryRPaths: "objdump: 'not_exists_file': Нет такого файла\n"

we see

ERROR: getBinaryRPaths: "objdump: 'not_exists_file': \xD0\x9D\xD0\xB5\xD1\x82 \xD1\x82\xD0\xB0\xD0\xBA\xD0\xBE\xD0\xB3\xD0\xBE \xD1\x84\xD0\xB0\xD0\xB9\xD0\xBB\xD0\xB0\n"

Fix: Empty error message.
We've already taken all STDERR in line:

if (strip.readAllStandardError().contains("Not enough room for program headers"))

So, the next line will not print the error message

LogError() << "Error stripping" << QFileInfo(resolvedPath).completeBaseName() << ":" << QString::fromLocal8Bit(strip.readAllStandardError());

If the utilities output localized messages, these messages are unreadable. For example, instead
ERROR: getBinaryRPaths: "objdump: 'not_exists_file': Нет такого файла\n"
we see
ERROR: getBinaryRPaths: "objdump: 'not_exists_file': \xD0\x9D\xD0\xB5\xD1\x82 \xD1\x82\xD0\xB0\xD0\xBA\xD0\xBE\xD0\xB3\xD0\xBE \xD1\x84\xD0\xB0\xD0\xB9\xD0\xBB\xD0\xB0\n"
We've already taken all STDERR in line:
if (strip.readAllStandardError().contains("Not enough room for program headers"))

So, the next line will not print the error message
LogError() << "Error stripping" << QFileInfo(resolvedPath).completeBaseName() << ":" << QString::fromLocal8Bit(strip.readAllStandardError());
@probonopd
Copy link
Owner

probonopd commented Sep 18, 2018

Thanks @SokoloffA good catch. However I think we should use a different solution: Instead of trying to parse non-English output, we should set environment variables so that the utilities called by linuxdeployqt run in English and hence produce known-good output. This will be more predictable and robust.

Do the equivalent of the following bash commands:

export LANG=C
export LC_CTYPE="C"
export LC_NUMERIC="C"
export LC_TIME="C"
export LC_COLLATE="C"
export LC_MONETARY="C"
export LC_MESSAGES="C"
export LC_ALL=""

Can you please verify that this works for you, and potentially implement this in your PR (possibly in addition to the UTF8 stuff you added)? Thank you very much.

@SokoloffA
Copy link
Author

Just changing the locale is not enough.

  1. LANG=C does not work. If the path has non English letters:
export LANG=C
export LC_CTYPE="${LANG}"
export LC_NUMERIC="${LANG}"
export LC_TIME="${LANG}"
export LC_COLLATE="${LANG}"
export LC_MONETARY="${LANG}"
export LC_MESSAGES="${LANG}"
export LC_ALL=""

linuxdeployqt русский/usr/share/applications/flacon.desktop -always-overwrite -appimage -bundle-non-qt-libs

linuxdeployqt 4 (commit c6c6898), build <local dev build> built on 2018-09-19 20:27:42 UTC
Desktop file as first argument: "??????????????/usr/share/applications/flacon.desktop"
ERROR: Desktop file in first argument does not exist!

Most Linux systems use UTF-8, so LANG=en_US.utf8 gives a better result:

export LANG=en_US.utf8
#export LANG=C
export LC_CTYPE="${LANG}"
export LC_NUMERIC="${LANG}"
export LC_TIME="${LANG}"
export LC_COLLATE="${LANG}"
export LC_MONETARY="${LANG}"
export LC_MESSAGES="${LANG}"
export LC_ALL=""

linuxdeployqt русский/usr/share/applications/flacon.desktop -always-overwrite -appimage -bundle-non-qt-libs

linuxdeployqt 4 (commit c6c6898), build <local dev build> built on 2018-09-19 20:27:42 UTC
Desktop file as first argument: "русский/usr/share/applications/flacon.desktop"
desktopExecEntry: "flacon"
desktopIconEntry: "flacon"
Found binary from desktop file: "/home/sokoloff/myPrograms/linuxdeployqt/master/build/русский/usr/bin/flacon"
FHS-like mode with PREFIX, fhsPrefix: "/home/sokoloff/myPrograms/linuxdeployqt/master/build/русский/usr"
app-binary: "/home/sokoloff/myPrograms/linuxdeployqt/master/build/русский/usr/bin/flacon"
appDirPath: "/home/sokoloff/myPrograms/linuxdeployqt/master/build/русский"
relativeBinPath: "usr/bin/flacon"
...
Embedding ELF...
Marking the AppImage as executable...
Cannot guess update information since $TRAVIS_REPO_SLUG is missing
Success

Please consider submitting your AppImage to AppImageHub, the crowd-sourced
central directory of available AppImages, by opening a pull request
at https://github.com/AppImage/appimage.github.io

But the error messages are still unreadable:

linuxdeployqt русский/usr/share/applications/flacon.desktop -always-overwrite -executable=not_exists_file -appimage -bundle-non-qt-libs

...
ERROR: findDependencyInfo: "ldd: ./not_exists_file: \xD0\x9D\xD0\xB5\xD1\x82 \xD1\x82\xD0\xB0\xD0\xBA\xD0\xBE\xD0\xB3\xD0\xBE \xD1\x84\xD0\xB0\xD0\xB9\xD0\xBB\xD0\xB0 \xD0\xB8\xD0\xBB\xD0\xB8 \xD0\xBA\xD0\xB0\xD1\x82\xD0\xB0\xD0\xBB\xD0\xBE\xD0\xB3\xD0\xB0\n"
ERROR: getBinaryRPaths: "objdump: 'not_exists_file': \xD0\x9D\xD0\xB5\xD1\x82 \xD1\x82\xD0\xB0\xD0\xBA\xD0\xBE\xD0\xB3\xD0\xBE \xD1\x84\xD0\xB0\xD0\xB9\xD0\xBB\xD0\xB0\n"

So we should to use QString::fromLocal8Bit in any case.

@probonopd
Copy link
Owner

Why is "Linux" always so complicated... how can we set everything to International English?

@SokoloffA
Copy link
Author

how can we set everything to International English?

I have a fashionable solution. We have to send the text to the google-translate :)


Seriously, as I understand you're worried about the code like:

output.contains("statically linked") 

We can set the locale for QProcess, but in any case the user may have non English letters in the file names. So we hould to set the locale for process and use fromLocal8Bit too.

@TheAssassin
Copy link
Collaborator

The obvious approach is to set LC_ALL to C before calling subprocesses that must be machine read. That's how https://github.com/linuxdeploy/linuxdeploy works.

@probonopd
Copy link
Owner

That's what I meant to do, only to find that in typical "Linux" fashion there is not one but multiple similarly-named variables to set the language... and seemingly picked the wrong example. So @SokoloffA can you confirm that it works with @TheAssassin's solution? Thanks.

@TheAssassin
Copy link
Collaborator

You can easily test which variable works for which tool: just env LC_ALL=C mytool. Especially basic tools like ldd behave consistently across all distros.

@SokoloffA
Copy link
Author

Strange result. In my distribution $LS_ALL does not overwrite the $LANGUAGE variable.

$ export | grep 'LC_\|LANG'
declare -x LANG="ru_RU.UTF-8"
declare -x LANGUAGE="ru_RU.UTF-8:ru"
declare -x LC_ADDRESS="ru_RU.UTF-8"
declare -x LC_COLLATE="ru_RU.UTF-8"
declare -x LC_CTYPE="ru_RU.UTF-8"
declare -x LC_IDENTIFICATION="ru_RU.UTF-8"
declare -x LC_MEASUREMENT="ru_RU.UTF-8"
declare -x LC_MESSAGES="ru_RU.UTF-8"
declare -x LC_MONETARY="ru_RU.UTF-8"
declare -x LC_NAME="ru_RU.UTF-8"
declare -x LC_NUMERIC="ru_RU.UTF-8"
declare -x LC_PAPER="ru_RU.UTF-8"
declare -x LC_TELEPHONE="ru_RU.UTF-8"
declare -x LC_TIME="ru_RU.UTF-8"

$ export LC_ALL=en_US.UTF-8
$ tools/linuxdeployqt/linuxdeployqt squashfs-root/usr/share/applications/flacon.desktop -executable=not_exists_file

ERROR: findDependencyInfo: "ldd: ./not_exists_file: \xD0\x9D\xD0\xB5\xD1\x82 \xD1\x82\xD0\xB0\xD0\xBA\xD0\xBE\xD0\xB3\xD0\xBE \xD1\x84\xD0\xB0\xD0\xB9\xD0\xBB\xD0\xB0 \xD0\xB8\xD0\xBB\xD0\xB8 \xD0\xBA\xD0\xB0\xD1\x82\xD0\xB0\xD0\xBB\xD0\xBE\xD0\xB3\xD0\xB0\n"
$ export LANGUAGE=en_US.UTF-8
$ tools/linuxdeployqt/linuxdeployqt squashfs-root/usr/share/applications/flacon.desktop -executable=not_exists_file

ERROR: findDependencyInfo: "ldd: ./not_exists_file: No such file or directory\n"

But when I added the string ldd.setEnvironment(QStringList() << "LC_ALL=en_US.UTF-8"); to the code, the program print a message in English.

$  export | grep 'LC_\|LANG'
declare -x LANG="ru_RU.UTF-8"
declare -x LANGUAGE="ru_RU.UTF-8:ru"
declare -x LC_ADDRESS="ru_RU.UTF-8"
declare -x LC_COLLATE="ru_RU.UTF-8"
declare -x LC_CTYPE="ru_RU.UTF-8"
declare -x LC_IDENTIFICATION="ru_RU.UTF-8"
declare -x LC_MEASUREMENT="ru_RU.UTF-8"
declare -x LC_MESSAGES="ru_RU.UTF-8"
declare -x LC_MONETARY="ru_RU.UTF-8"
declare -x LC_NAME="ru_RU.UTF-8"
declare -x LC_NUMERIC="ru_RU.UTF-8"
declare -x LC_PAPER="ru_RU.UTF-8"
declare -x LC_TELEPHONE="ru_RU.UTF-8"
declare -x LC_TIME="ru_RU.UTF-8"

$ tools/linuxdeployqt/linuxdeployqt squashfs-root/usr/share/applications/flacon.desktop -executable=not_exists_file

ERROR: findDependencyInfo: "ldd: ./not_exists_file: No such file or directory\n"

In any case, we cannot use the "C" locale. If program use the "C" locale, I cannot use Russian filenames.

$ LC_ALL=C tools/linuxdeployqt/linuxdeployqt русский/usr/share/applications/flacon.desktop 
linuxdeployqt 4 (commit c6c6898), build <local dev build> built on 2018-09-19 20:27:42 UTC
Desktop file as first argument: "??????????????/usr/share/applications/flacon.desktop"
ERROR: Desktop file in first argument does not exist!

@TheAssassin
Copy link
Collaborator

The issue with that is that en_US.UTF-8 won't be available on every computer. Unless you/we find a solution for that, C is the better trade-off.

@bjorn
Copy link
Contributor

bjorn commented Nov 5, 2020

I don't really understand why this PR resulted in a locale discussion. The issue seems to be about supporting file names with special characters, which as per results shown by @SokoloffA clearly can't be solved by forcing a C locale.

Instead, the proposed patch seems to be to be the right solution. Using QString::fromLocal8Bit is required to turn the QByteArray coming from standard output/error to a QString while taking into account the system encoding.

Edit: An exception to this is when standard output/error is passed on to LogError() directly, in which case it should simply be passed on as-is (I see this PR also applies QString::fromLocal8Bit in many such cases).

@probonopd
Copy link
Owner

I don't really understand why this PR resulted in a locale discussion. The issue seems to be about supporting file names with special characters, which as per results shown by @SokoloffA clearly can't be solved by forcing a C locale.

Makes sense, sorry if I got it wrong earlier.

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

4 participants