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

Remove default Shiboken2 executable in qmake proj #1600

Merged
merged 1 commit into from Jun 11, 2019

Conversation

Projects
None yet
2 participants
@Hamled
Copy link
Contributor

commented Jun 11, 2019

The correct executable path should be supplied on the command line when
running qmake, or it will be discovered by using pkg-config.

Detailed description

When qmake is run with Python bindings enabled (CUTTER_ENABLE_PYTHON=true CUTTER_ENABLE_PYTHON_BINDINGS=true) the dependency check for Shiboken2 does not actually happen.

This is because the SHIBOKEN_EXECTUABLE variable referenced in the outer scope of that dependency check is always set in that case.

It looks like the outer scope was added when updating to support cutter-deps on Windows. I think the intention was to skip the check on that platform because pkg-config is not available.

In that case, I think it should work to remove the default value set earlier in the script, and rely on the later check which assigns a value to the variable with pkg-config.

Test plan (required)

I tested this locally with my Archlinux setup. The current Arch package's PKGBUILD does not build Cutter with the Python bindings, and I discovered this issue when building with a customized PKGBUILD.

According to my testing, prior to this change if Shiboken2 is not installed and available through pkg-config you get a build error stemming from the next check (for PySide2) even when that library is installed. With this change, the correct error relating to a missing Shiboken2 dependency is shown.

Note

I am not currently in a position to test this build script change on Windows, MacOS, or any other Linux distro.

I also think that problems with this change wouldn't show up at all in AppVeyor because its build script specifies the value directly. They might in Travis, though.

Remove default Shiboken2 executable in qmake proj
The correct executable path should be supplied on the command line when
running qmake, or it will be discovered by using pkg-config.
@Hamled

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Pinging @thestr4ng3r because they made the commit that is referenced in the above, which added an outer scope to the Shiboken2 package check.

@thestr4ng3r

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

I don't remember the exact reason for this line and CI is green, so lgtm!

@thestr4ng3r thestr4ng3r merged commit d59ea03 into radareorg:master Jun 11, 2019

2 of 5 checks passed

LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Hamled Hamled deleted the Hamled:qmake-shiboken2-dep branch Jun 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.