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

python.pri: Fix path search for MinGW on Linux #2765

Merged
merged 1 commit into from Feb 19, 2017

Conversation

@davidebeatrici
Copy link
Member

commented Jan 22, 2017

MinGW triggers the "win32" block, which sets the Python command to "python" without searching for its path.
Now it isn't needed anymore since the script checks if the "which" command is available. If it doesn't the command is set to "python".

python.pri Outdated
# If it isn't available, it will not print anything.
WHICH=$$system(which which 2>/dev/null)
isEmpty(WHICH) {
message("The 'which' command is not available on the system. Unable to search for Python installations. Assuming 'python'... If this is not correct, please point the MUMBLE_PYTHON environment variable at a working Python 2 or Python 3 binary.")

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jan 22, 2017

Member

This message will always show on Windows, since it doesn't have 'which'.
We can't have that.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:python.pri-typo-fix branch 4 times, most recently from 54bcd6f to 51e6253 Feb 2, 2017

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2017

The where command breaks the script if there are multiple Python installations, because it prints all of the paths in different lines.
The CMD doesn't have a simple command to get only the first line of the output, we will probably have to do it from QMake.

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2017

Problem solved, we can use QMake's first function.
Sadly there's another problem: where searches only in the current directory and system's PATH by default. There's a /R option, which allows to specify a path to search in recursively, but it makes QMake slow. No options to stop the tool when a match is found are available.
At this point I propose to use a custom utility like which.exe contained in win-bash. that we can use also when searching for other build tools binaries,

@mkrautz

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

Correct me if I'm wrong, but isn't the only problem with the current python.pri that it doesn't take cross-compiling into consideration?

On Windows, python will always be called python.exe.
When cross-building for Windows on a Unix-like system, using which is fine.

So something like (untested):

# If we're on Qt 4 on win32, or we're building from Windows, use "python" unless
# MUMBLE_PYTHON is specified via env var.
equal(QMAKE_HOST.os, "Windows")|(equal(QMAKE_VERSION_INT_MAJOR, 4):win32) {
    PYTHON = $$(MUMBLE_PYTHON)
    isEmpty(PYTHON) {
        PYTHON=python
    }
} else {
  // Else, query using which.
}
@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2017

Yes, it's the only problem it has. where would have been useful if it had the missing option, because we would have been able to use the build tools without the need of inserting them in PATH.
If on Windows the binary's name is always python.exe, it isn't a problem to always use "python" as command.

python.pri Outdated
# If the MUMBLE_PYTHON environment variable isn't set
isEmpty(PYTHON) {
# If we're on Qt 4 on win32, or we're building from Windows, use "python"
equal(QMAKE_HOST.os, "Windows")|(equal(QMAKE_VERSION_INT_MAJOR, 4):win32) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz Feb 8, 2017

Member

Probably needs a qt.pri include to use the version string.

Also not sure if this even runs. Did you test it?

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Feb 8, 2017

Author Member

python.pri:37: Opening parenthesis without prior test name.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:python.pri-typo-fix branch 4 times, most recently from 0972e02 to a10226e Feb 12, 2017

@mkrautz
Copy link
Member

left a comment

Except for the fact that you removed a newline at the end of the file, it looks good to me.

Can you rebase on current master so you get CI testing? Thanks.

python.pri Outdated
error("Unable to find the system's Python binary. Some scripts invoked during the Mumble build use Python. You can manually specify it via the MUMBLE_PYTHON environment variable (either 2 or 3).")
}
}
}

This comment has been minimized.

Copy link
@mkrautz

mkrautz Feb 13, 2017

Member

Missing newline at the end.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:python.pri-typo-fix branch from a10226e to 43fedc1 Feb 13, 2017

python.pri: Fix path search for MinGW on Linux
MinGW triggers the "win32" block, which sets the Python command to "python" without searching for its path.
Now it isn't needed anymore since the script checks if the "which" command is available and if it doesn't it sets the command to "python".

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:python.pri-typo-fix branch from 43fedc1 to 09cfc1d Feb 13, 2017

@mkrautz mkrautz merged commit af903fa 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
Projects
None yet
2 participants
You can’t perform that action at this time.