-
Notifications
You must be signed in to change notification settings - Fork 16
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
Fix CMake config to build on Ubuntu 22.04 #117
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We're testing this internally before merging.
@@ -24,7 +24,7 @@ qssc_add_plugin(QSSCPayloadZip QSSC_PAYLOAD_PLUGIN | |||
|
|||
LINK_LIBS | |||
QSSCPayload | |||
libzip::zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change breaks builds on OSX, is it necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will not build in my testing setup because CMake doesn't correctly find the Conan-provided zip library.
The proposed change uses the system library instead, which is installed by default on the image specified above.
If it breaks on OSX, we shouldn't merge it as is, of course.
A) provide a proper Conan/CMake fix to supply the Conan-based libzip
B) branch at this point to use system libzip if available
C) revert this line and soon put it into Linux install instructions
Right now I'm not seeing an obvious way to do A).
B) is ugly.
C) is the easiest but relies on the tough skin of Linux users ;) ... pick that for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering why Conan/Cmake isn't finding the correct library. Until I get an Ubuntu image spun up to test I think C. is easiest, but I really think this should be resolvable.
What conan/cmake command did you use to install/build?
I very much rely on that, only testing with Ubuntu 22.04 here! |
Its weird it doesn't seem like CI has run here, hmm.. |
Add the CXX tag to the main target.
Due to the minuscule size of the change, I've force-pushed instead of reverting. Hope that's OK with you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with OSX x86 and tests passed with a clean build
Closes #104 and #105
Testing was done on a fresh Ubuntu 22.04 VM running on a Ubuntu 22.04 host.
To reproduce, install VirtualBox on the host with:
sudo apt install virtualbox-qt
Then download the Ubuntu ISO from:
https://releases.ubuntu.com/22.04.2/ubuntu-22.04.2-desktop-amd64.iso
Launch the VirtualBox GUI, install the image, inside the VM install the common build tools with:
sudo apt install build-essential git ninja-build python3-pip python3-setuptools-scm sudo pip install --upgrade pip sudo pip install cmake # newer version than repo sudo pip install --upgrade conan==1.59.0 conan profile new default --detect conan profile update settings.compiler.libcxx=libstdc++11 default
Copy the repo into the VM (either by
git clone
with valid credentials or a passthrough directory to the host) andcd
into it.