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

Allow specifying custom path to lrelease binary #400

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mplucinski
Copy link

@mplucinski mplucinski commented Dec 20, 2017

This patch allows a user to specify custom path to lrelease tool during configuration phase. It's necessary e.g. in Fedora, where the tool is called "lrelease-qt4". When using the patch, in case if the user specifies LRELEASE_PATH, the search phase is skipped and it's assumed that the user-provided path is correct. If the variable isn't specified, the search should work like before.

@droark
Copy link

droark commented Dec 20, 2017

Hmmm. Is there another way to do this? I don't like the idea of forcing people to add LRELEASE_PATH to their configs. macOS users would have to do it. I can't speak for anyone else but suspect others would run into similar issues.

@goatpig
Copy link
Owner

goatpig commented Dec 21, 2017

lrelease is to package the translation stuff for Qt4. I'm wondering if it's not preferable to just skip the test if lrelease is missing.

@mplucinski
Copy link
Author

mplucinski commented Dec 21, 2017

I'm sorry if I didn't write it clearly, but (AFAIK, I'm not in any way an autotools-expert) with my patch, the search for a binary should work like before, unless the variable is specified. I don't think it'd affect the people for whom the current setup works.

EDIT: I just checked, and it behaves like I described: in case if LRELEASE_PATH isn't specified in the environment, the search works like before, i.e. searches in $PATH for "lrelease".

@droark
Copy link

droark commented Dec 21, 2017

On macOS, this patch fails.

checking for lrelease... no
configure: error: missing lrelease in path, make sure qt4-linguist-tools is installed or specify LRELEASE_PATH

Granted, I am running this from the command line directly, which isn't how Armory is currently built on Macs. (I have a WIP patch to overhaul all that, and could try this patch against it.) I'm sure I could integrate LRELEASE_PATH into the Mac build script. I just don't like the idea of "intrusive" patches like these unless it's proven that, say, my system's an outlier, while virtually everybody else is dandy.

Have you tried this patch on Ubuntu, Debian, and perhaps Fedora? If they're happy, and maybe my WIP patch is happy, I suppose I don't have much of a leg to stand on.

@goatpig
Copy link
Owner

goatpig commented Dec 21, 2017

Maybe amend the PR it to just bypass the use of lrelease if it can't be detected nor is user specified?

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