-
Notifications
You must be signed in to change notification settings - Fork 816
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
Get rid of FindQt5Keychain.cmake #2603
Conversation
QtKeychain provides Qt5KeychainConfig.cmake and friends nowadays, so no need to have a less reliable and outdated find module on our end. Also this shows that we were including keychain.h in the wrong way and were not using the link target, so both got fixed as well. Signed-off-by: Kevin Ottens <[email protected]>
Now that things are done in a more standard way, let's adjust the AppImage build so that QtKeychain is picked up properly now that our FindQt5Keychain.cmake file is gone. Signed-off-by: Kevin Ottens <[email protected]>
04df1f7
to
0e617d0
Compare
AppImage file: Nextcloud-PR-2603-0e617d020f6354e536687eb5a4423aae862d9e4a-x86_64.AppImage |
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.
Looks good to me
@@ -30,7 +30,7 @@ mkdir build | |||
cd build | |||
cmake -D CMAKE_INSTALL_PREFIX=/usr ../ | |||
make -j4 | |||
make DESTDIR=/app install |
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 looks unrelated?
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 looks unrelated?
Well actually not, /app wasn't in the paths the cmake was looking at to find qtkeychain.
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.
Eventually we also should update the scripts in the client-building repo, brander and mac os.
Exactly, now that it's in we should trigger all on master and see what fails. |
QtKeychain provides Qt5KeychainConfig.cmake and friends nowadays, so no
need to have a less reliable and outdated find module on our end.
Also this shows that we were including keychain.h in the wrong way and
were not using the link target, so both got fixed as well.
Signed-off-by: Kevin Ottens [email protected]